[ovirt-devel] vdsm tasks API design discussion

[Adam Litke]

On 05/05/14 12:29 -0400, Saggi Mizrahi wrote:
>I'll try and sum up my responses here:

I would prefer if you responded inline since it helps you respond
specifically to individual points and also helps others follow along
with the conversation.

>About being things being closed for discussion:
>This stuff had been put up for discussion multiple times
>over more that a year. There comes a time when a decision
>has to be made.

Yep, and then you write code, submit patches, and get them reviewed.
If people don't like the way you did it, you have to go back to the
design phase and try again.  I don't care if you had some discussions
with other people a year ago.  I am paying attention now and have some
concerns with your approach.

>Unless I get compelling enough arguments (ie. an actual use
>case) I will have to keep things as they are.
>
>As for task ID format not being enforced:
>This was also up for discussion ages ago.

This is not a valid reason to ignore further discussion.

>We already had issues with useless validations
>causing us problems over time.
>
>As I said, I don't care if the engine enforces
>it to be a UUID. As long as the only argument is
>"I don't want people that interface VDSM to do stupid
>things" my response it still going to be "I am not here
>to make sure other projects are doing their job correctly".

I'm having trouble hearing you shouting from so high up in your infra
silo.  Last I checked we are working on the same project.  I still
haven't heard a good reason why a UUID isn't enough here.  I'm not
asking vdsm to validate a string with the 'uuid' python module.  I am
just asking that it be treated like other uuids in the API (ie. it's
documented as a UUID in the schema or elsewhere and it is followed by
convention).

>So unless I get a valid real world reason to reopen that
>discussion things I'm going leave things as they are.

It seems to me that the discussion is open now whether you like it or
not :)  Just because you disagree with me does not make my argument
invalid.  Maybe others have an opinion on this too.

>I will be adding length limitation to the ID to prevent abuse.

I thought you were against useless validations.

>Now for the meaty bit:
>
>The main reason against having job control is that
>it doesn't work on a clustered message based system.

To be clear I am not asking for job control.  I am asking for a very
simple form of job monitoring.  Vdsm can tell me whether my named
operation is still running.

>You need to keep 3 things in mind:
>1. Messages can get lost (either requests or responses)

Indeed.  If my request for the list of jobs gets lost or times out
I'll simply ask again.  I assume jobs have not changed from running to
stopped during a time of lost connectivity.

>2. Messages can arrive out of order.

Yep, this is handled by the protocol in the form of message ids.
libvirt and qemu are doing this with qmp today.  It's not related to
the reasons I want a list of running jobs that are indexed by
engine-supplied UUIDs.

>3. The same message can arrive multiple times.

Again, fixed by message IDs and unrelated to the meat of our
disagreement.

>3 You can try and minimize in the actual messaging layer
>but 1 and two have to be handled by the actual algorithms.
>
>Lets take a general request.
>Using getAllRunningJobs() I have no way
>of knowing if:
>1. The message is still in transit
>2. The request is already over (thus was cleared)
>3. VDSM crashed.
>4. Response was sent but never arrived.
>5. Any combination of the cases above.

You either got a response or didn't.  If you got a response, handle
it.  If not, then assume that no jobs have transitioned states
(running to stopped) until you get confirmation.  This is how live
merge will be implemented.  Once you know a job does not exist, then
you can handle resolution of it.

>
>That is what is important about having idempotency or
>having things be bound to entities.
>
>Binding things to an entity gives you the information
>you need to resolve those issues.
>
>without that you will find it very hard to know what exactly is
>going on.

I think you're arguing about resolving success or failure which I
believe we agree on.  In the live merge case, we simply wait for proof
from vdsm that the job is no longer running (job UUID absent from vm
stats).  At that point we query the volume chain of the given vm disk
to determine the actual result.

>For setupNetwork() I would just make it idempotent.
>You just send the same command again with the same end state.
>It would be a no-op if it's the current network topology
>is what it's supposed to be.

no-op is a bit generous.  It will still need to check everything to
make sure it is actually consistent.  This probably involves invoking
quite a few external commands to query interfaces, routes, etc.  It
would be nice if engine could wait until a jobID disappeared before
checking a single time if the state is correct.  Otherwise it will be
very busy at each polling interval checking everything over and over.

>You could also have a special field containing the "version"
>of the configuration (I would make it a hash or a UUID and
>not a running number) that you would persist locally on the
>host after you finished configuring since the local host is the
>scope of setupNetworks().

Hmm, interesing.  It would save time and effort on scanning network
properties.  But you are introducing the persistence of task
end-state.  I thought this was something we are trying to avoid.

>It would allow you to not care about any of the error state
>keep sending the same configuration if you think something
>bad happened until you the is what you expect it to be or
>and error response actually manages find it's way back
>to you. By using the same task ID you are guaranteed
>to only have the operation running once at a time.
>
>I don't mind helping anyone with making their algorithms
>work but there is no escaping from the limitations
>listed above. If we want to make oVirt truly scalable
>and robust we have to start thinking about algorithms
>that work despite of errors and not just have error flows.

Agreed.  This is what the ngTasks framework is supposed to achieve for
us.  I think you are conflating the issue of listing active operations
and high level flow design.  If the async operations that make up a
complex flow are themselves idempotent, then we have achieved the
above.  It can be done with or without a vdsm api to list running
jobs.

>Notice I don't even mention different systems of persistence
>and some tasks that you should be able to get state information
>about from more than one host. Some "Jobs" can survive
>a VDSM restart since it's not in VDSM like stuff in gluster or QEmu.

Yep, live merge is one such job.  While we don't persist the job, we
do remember that it was running so we can synchronize our state with
the underlying hypervisor when we restart.

>To make it clear, the task API shouldn't really be that useful.
>Task IDs are just there to match requests to responses internally
>because as I explained, jobs are hard to manage generally in
>such a system.

>This by no way means that if we see a use case emerging that requires
>some sort of infra we would not do it. I just think it would probably
>be tied to some common algorithm or idiom than something truly generic
>used by every API call.

Maybe we are talking about two different things that cannot be
combined.  All I want is a generic way to list ongoing host-level
operations that will be useful for live merge and others.  If all you
want is a protocol syncronization mechanism in the style of QMP then
that is different.  Perhaps we need both.  I'll be happy to keep the
jobID as a formal API parameter and other new APIs that spawn
long-running operations could do the same.  Then whatever token you
want to pass on the wire does not matter to me at all.

>Hope I made things clearer, sorry if I came out a bit rude.
>I'm off, I have my country's birthday to celebrate.

Thanks for participating in the discussion.  In the end we will end
up with superior code than if we had not had this discussion.  Happy
Yom HaAtzmaut!

-- 
Adam Litke