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

27 Sep 2012


      ----- Original Message -----
...
On 09/27/2012 01:11 AM, Ayal Baron wrote:
...
----- Original Message -----
...
----- Original Message -----
...
From: "Ayal Baron" <abaron@redhat.com>
To: "Michael Kublin" <mkublin@redhat.com>
Cc: "Liron Aravot" <laravot@redhat.com>, "engine-devel"
<engine-devel@ovirt.org>, "Eduardo Warszawski"
<ewarszaw@redhat.com>, "Allon Mureinik" <amureini@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@redhat.com>
To: "Allon Mureinik" <amureini@redhat.com>, "Michael Kublin"
<mkublin@redhat.com>
Cc: "Liron Aravot" <laravot@redhat.com>, "engine-devel"
<engine-devel@ovirt.org>, "Eduardo Warszawski"
<ewarszaw@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@redhat.com>
>> To: "Allon Mureinik" <amureini@redhat.com>
>> Cc: "Eduardo Warszawski" <ewarszaw@redhat.com>, "Liron
>> Aravot"
>> <laravot@redhat.com>, "Maor Lipchuk"
>> <mlipchuk@redhat.com>, "engine-devel"
>> <engine-devel@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_D...
>>>>>>>>>> -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@ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
_______________________________________________
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel