[Engine-devel] Serial Execution of Async Tasks

Thu Sep 27 05:41:29 UTC 2012

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

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