[Engine-devel] Serial Execution of Async Tasks

Mon Sep 24 08:10:08 UTC 2012


----- 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.

> > 
> >   
> > > > 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
> > > > 
> > > 
> > 
>