[Engine-devel] Serial Execution of Async Tasks
Ayal Baron
abaron at redhat.com
Wed Sep 26 23:11:02 UTC 2012
----- Original Message -----
>
>
> ----- Original Message -----
> > From: "Ayal Baron" <abaron at redhat.com>
> > To: "Michael Kublin" <mkublin at redhat.com>
> > Cc: "Liron Aravot" <laravot at redhat.com>, "engine-devel"
> > <engine-devel at ovirt.org>, "Eduardo Warszawski"
> > <ewarszaw at redhat.com>, "Allon Mureinik" <amureini at redhat.com>
> > Sent: Sunday, September 23, 2012 5:27:54 PM
> > Subject: Re: [Engine-devel] Serial Execution of Async Tasks
> >
> >
> >
> > ----- Original Message -----
> > >
> > >
> > > ----- Original Message -----
> > > > From: "Ayal Baron" <abaron at redhat.com>
> > > > To: "Allon Mureinik" <amureini at redhat.com>, "Michael Kublin"
> > > > <mkublin at redhat.com>
> > > > Cc: "Liron Aravot" <laravot at redhat.com>, "engine-devel"
> > > > <engine-devel at ovirt.org>, "Eduardo Warszawski"
> > > > <ewarszaw at redhat.com>
> > > > Sent: Sunday, September 23, 2012 1:10:07 PM
> > > > Subject: Re: [Engine-devel] Serial Execution of Async Tasks
> > > >
> > > >
> > > >
> > > > ----- Original Message -----
> > > > >
> > > > >
> > > > > ----- Original Message -----
> > > > > > From: "Michael Kublin" <mkublin at redhat.com>
> > > > > > To: "Allon Mureinik" <amureini at redhat.com>
> > > > > > Cc: "Eduardo Warszawski" <ewarszaw at redhat.com>, "Liron
> > > > > > Aravot"
> > > > > > <laravot at redhat.com>, "Maor Lipchuk"
> > > > > > <mlipchuk at redhat.com>, "engine-devel"
> > > > > > <engine-devel at ovirt.org>
> > > > > > Sent: Sunday, September 23, 2012 12:41:05 PM
> > > > > > Subject: Re: [Engine-devel] Serial Execution of Async Tasks
> > > > > >
> > > > > >
> > > > > > >>>>>>>>>> Hi guys,
> > > > > > >>>>>>>>>>
> > > > > > >>>>>>>>>> As you may know the engine currently has the
> > > > > > >>>>>>>>>> ability
> > > > > > >>>>>>>>>> to
> > > > > > >>>>>>>>>> fire
> > > > > > >>>>>>>>>> an
> > > > > > >>>>>>>>>> SPM
> > > > > > >>>>>>>>>> task, and be asynchronously be "woken-up" when
> > > > > > >>>>>>>>>> it
> > > > > > >>>>>>>>>> ends.
> > > > > > >>>>>>>>>> This is great, but we found the for the Live
> > > > > > >>>>>>>>>> Storage
> > > > > > >>>>>>>>>> Migration
> > > > > > >>>>>>>>>> feature we need something a bit complex - the
> > > > > > >>>>>>>>>> ability
> > > > > > >>>>>>>>>> to
> > > > > > >>>>>>>>>> have a
> > > > > > >>>>>>>>>> series of async tasks in a single control flow.
> > > > > > >>>>>>>>>>
> > > > > > >>>>>>>>>> Here's my initial design for this, your comments
> > > > > > >>>>>>>>>> and
> > > > > > >>>>>>>>>> criticism
> > > > > > >>>>>>>>>> would
> > > > > > >>>>>>>>>> be welcome:
> > > > > > >>>>>>>>>> http://wiki.ovirt.org/wiki/Features/Serial_Execution_of_Asynchronous_Tasks_Detailed_Design
> > > > > > >>>>>>>> -successful execution -
> > > > > > >>>>>>>> * "CommandBase iterates over its
> > > > > > >>>>>>>> SPMAsyncTaskHandlers"
> > > > > > >>>>>>>> -
> > > > > > >>>>>>>> when?
> > > > > > >>>>>>> This is the new suggested format of
> > > > > > >>>>>>> executeCommand().
> > > > > > >>>>>>> I'll
> > > > > > >>>>>>> clarify
> > > > > > >>>>>>> this too.
> > > > > > >>>>>>>
> > > > > > >>>>>>>> * If the second task is an HSM command (vs. SPM
> > > > > > >>>>>>>> command),
> > > > > > >>>>>>>> I
> > > > > > >>>>>>>> think you
> > > > > > >>>>>>>> should explain in the design how to handle such
> > > > > > >>>>>>>> flows
> > > > > > >>>>>>>> as
> > > > > > >>>>>>>> well.
> > > > > > >>>>>>> HSM commands do not create AsyncTasks, as they do
> > > > > > >>>>>>> today
> > > > > > >>>>>>> -
> > > > > > >>>>>>> I
> > > > > > >>>>>>> will
> > > > > > >>>>>>> clarify this.
> > > > > > >>>>>>>
> > > > > > >>>>>>>> * Why do we need before task? can you give a
> > > > > > >>>>>>>> concrete
> > > > > > >>>>>>>> example
> > > > > > >>>>>>>> of what
> > > > > > >>>>>>>> would you do in such a method.
> > > > > > >>>>>>> Basically, /today/, command look like this:
> > > > > > >>>>>>> executeCommand() {
> > > > > > >>>>>>> doStuffInTheDB();
> > > > > > >>>>>>> runVdsCommand(someCommand);
> > > > > > >>>>>>> }
> > > > > > >>>>>>>
> > > > > > >>>>>>> endSuccessfully() {
> > > > > > >>>>>>> doMoreStuffInTheDB();
> > > > > > >>>>>>> }
> > > > > > >>>>>>>
> > > > > > >>>>>>> endWithFailure() {
> > > > > > >>>>>>> doMoreStuffForFailureInTheDB();
> > > > > > >>>>>>> }
> > > > > > >>>>>>>
> > > > > > >>>>>>> In the new design, the entire doStuffInTheDB()
> > > > > > >>>>>>> should
> > > > > > >>>>>>> be
> > > > > > >>>>>>> moved
> > > > > > >>>>>>> to a
> > > > > > >>>>>>> breforeTask of the (only) SPMAsyncTaskHandler.
> > > > > > >>>>>>>
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> - I see you added SPMAsyncTaskHandler, any reason
> > > > > > >>>>>>>> not
> > > > > > >>>>>>>> to
> > > > > > >>>>>>>> use
> > > > > > >>>>>>>> SPMAsyncTasK to manage it own life-cycle?
> > > > > > >>>>>>> Conserving today's design - The SPMAsyncTaskHandler
> > > > > > >>>>>>> is
> > > > > > >>>>>>> the
> > > > > > >>>>>>> place to
> > > > > > >>>>>>> add additional, non-SPM, logic around the SPM task
> > > > > > >>>>>>> execution,
> > > > > > >>>>>>> like
> > > > > > >>>>>>> CommandBase allows today.
> > > > > > >>>>>>>
> > > > > > >>>>>>>>
> > > > > > >>>>>>>> - In the life-cycle managed by the
> > > > > > >>>>>>>> SPMAsyncTaskHandler
> > > > > > >>>>>>>> there
> > > > > > >>>>>>>> is a
> > > > > > >>>>>>>> step
> > > > > > >>>>>>>> 'createTask - how to create the async task' can
> > > > > > >>>>>>>> you
> > > > > > >>>>>>>> please
> > > > > > >>>>>>>> elaborate
> > > > > > >>>>>>>> what are the options.
> > > > > > >>>>>>> new [any type of async task]
> > > > > >
> > > > > > (I cleaned thread a little.)
> > > > > > The following design and it is implementation
> > > > > > http://gerrit.ovirt.org/#/c/7956/
> > > > > > is bad.
> > > > > > I don't see any reason for creating a new
> > > > > > SPMAsyncTaskHandler
> > > > > > and
> > > > > > especially in the
> > > > > > way as it's done in patch.
> > > > > > The reason are following:
> > > > > > 1. Performance , increased memory footprint, created CYCLIC
> > > > > > REFERENCE.
> > > > > > 2. Readability and robust of code: the code which is
> > > > > > written
> > > > > > as
> > > > > > cyclic references is unreadable
> > > > > > and difficult for debug.
> > > > > > 3. Why I need a generic implementation and changes all over
> > > > > > whole
> > > > > > project because of
> > > > > > series of async commands, for me it is a private case?
> > > >
> > > > What is the private case here exactly?
> > > > Every task can have multiple jobs. We've identified several
> > > > such
> > > > places (e.g. live storage migration, move disk, move vm) and I
> > > > have
> > > > no doubt more will popup.
> > > > As Allon notes below, task handling is done at CommandBase, if
> > > > you
> > > > think task management should be for storage only, you're
> > > > welcome
> > > > to
> > > > push it down to StorageHandlingCommandBase (or get rid of
> > > > inheritance here altogether).
> > > Interesting , regards cyclic reference no response, but who
> > > cares,
> > > it is difficult to answer , that's why better not to response?
> >
> > There is no problem with cyclic references in general, GCs know how
> > to deal with these just fine and in this case it's limited to the
> > command and its handlers.
> > I did not reply, however, as I do not feel strongly about this.
> >
> > > Regards private case:
> > > 1. We have command that not creating any task
> > > 2. We have command that will create a one task.
> > > 3. And we have 3 commands meanwhile which will create more than
> > > one
> > > task.
> > > I think that 3 is private case and not common? (In the future, I
> >
> > once happens
> > twice is a coincidence
> > three times is a method
> >
> > But if you insist on more then it's easy enough. We've discussed
> > many times in the past that we need to change many of the storage
> > verbs to run asynchronously (e.g. createStorageDomain) once this
> > happens then existing flows would have to run multiple async tasks
> > serially.
> >
> > > removed too many
> > > lines of code that were preparation for future that never come)
> >
> > This is not in preparation for the future, it is for a feature
> > we're
> > working on right now (live storage migration) and for fixing move
> > disk on which we have several bugs pending.
> >
> > > The handling done at CommandBase it means that it is influence
> > > all
> > > system.
> >
> > That is how the task management was done. Again, if you feel it
> > should only affect storage flows, feel free to push it down into
> > StorageCommandHandlingBase and then only storage verbs will have
> > task management.
> >
> > > Now regards architecture why I need some handler which will be
> > > inside
> > > a command
> > > and will call for command methods? Please explain.
> >
> > As opposed to what?
> So, u think it is a good design where we are using a "Circular
> references"
> design pattern. If yes, please elaborate.
I'm saying nacking something without suggesting a better alternative is not good practice.
Besides, the design is classic callback pattern (http://en.wikipedia.org/wiki/Callback_%28computer_programming%29) which is quite common and accepted.
Regardless though, take a look at the new patches.
>
> > >
> > >
> > > > > This will occur all over the storage commands (which are the
> > > > > only
> > > > > usages of tasks nowadys).
> > > > > Moreover, async task handling is done at the Commandbase
> > > > > level
> > > > > (see
> > > > > the end* methods) - instead of hacking it in X different
> > > > > places
> > > > > whenever we need it, I'd prefer doing it once, properly.
> > > > > >
> > > > > >
> > > > > _______________________________________________
> > > > > Engine-devel mailing list
> > > > > Engine-devel at ovirt.org
> > > > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > > > >
> > > >
> > >
> >
>
More information about the Engine-devel
mailing list