Re: [Engine-devel] Serial Execution of Async Tasks

From: "Ayal Baron" <abaron(a)redhat.com&gt; To: "Michael Kublin" <mkublin(a)redhat.com&gt; Cc: "Liron Aravot" <laravot(a)redhat.com&gt;, "engine-devel" <engine-devel(a)ovirt.org&gt;, "Eduardo Warszawski" <ewarszaw(a)redhat.com&gt;, "Allon Mureinik" <amureini(a)redhat.com&gt; 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(a)redhat.com&gt; > > To: "Allon Mureinik" <amureini(a)redhat.com&gt;, "Michael Kublin" > > <mkublin(a)redhat.com&gt; > > Cc: "Liron Aravot" <laravot(a)redhat.com&gt;, "engine-devel" > > <engine-devel(a)ovirt.org&gt;, "Eduardo Warszawski" > > <ewarszaw(a)redhat.com&gt; > > 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(a)redhat.com&gt; > > > > To: "Allon Mureinik" <amureini(a)redhat.com&gt; > > > > Cc: "Eduardo Warszawski" <ewarszaw(a)redhat.com&gt;, "Liron > > > > Aravot" > > > > <laravot(a)redhat.com&gt;, "Maor Lipchuk" > > > > <mlipchuk(a)redhat.com&gt;, "engine-devel" > > > > <engine-devel(a)ovirt.org&gt; > > > > 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_Task... > > > > >>>>>>>> -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?

> > > > > 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(a)ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > > >