[Engine-devel] Serial Execution of Async Tasks
Ayal Baron
abaron at redhat.com
Thu Sep 27 07:17:17 UTC 2012
----- Original Message -----
>
>
> On 09/27/2012 01:11 AM, Ayal Baron wrote:
> >
> >
> > ----- 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.
> IMHO, I think the fact ALL commands have the ability to create VDSM
> async tasks (even nowadays, regardless of the serial execution
> suggestion) - is bad.
> IMHO, A command should use AsyncTaskManager to create/manage tasks
Fine by me, but that's a whole different ball game and has nothing to do with the current feature.
> >>>
> >>> 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.
>
> Who knows if storage will be the only case. See previous comment on
> where we should consider having task MGMT code.
I agree it should be split out of the commands altogether, as most of the code in commands should (in fact I see no justification for the command pattern in most places in the code at all).
But that is a much bigger change and requires a ton of refactoring.
>
>
> >>>
> >>>> 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
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> > _______________________________________________
> > 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