On 11/12/14 01:14, Anant Patil wrote:
On 04-Dec-14 10:49, Zane Bitter wrote:
On 01/12/14 02:02, Anant Patil wrote:
On GitHub:https://github.com/anantpatil/heat-convergence-poc

I'm trying to review this code at the moment, and finding some stuff I
don't understand:


This appears to loop through all of the resources *prior* to kicking off
any actual updates to check if the resource will change. This is
impossible to do in general, since a resource may obtain a property
value from an attribute of another resource and there is no way to know
whether an update to said other resource would cause a change in the
attribute value.

In addition, no attempt to catch UpdateReplace is made. Although that
looks like a simple fix, I'm now worried about the level to which this
code has been tested.

We were working on new branch and as we discussed on Skype, we have
handled all these cases. Please have a look at our current branch:

When a new resource is taken for convergence, its children are loaded
and the resource definition is re-parsed. The frozen resource definition
will have all the "get_attr" resolved.

I'm also trying to wrap my head around how resources are cleaned up in
dependency order. If I understand correctly, you store in the
ResourceGraph table the dependencies between various resource names in
the current template (presumably there could also be some left around
from previous templates too?). For each resource name there may be a
number of rows in the Resource table, each with an incrementing version.
As far as I can tell though, there's nowhere that the dependency graph
for _previous_ templates is persisted? So if the dependency order
changes in the template we have no way of knowing the correct order to
clean up in any more? (There's not even a mechanism to associate a
resource version with a particular template, which might be one avenue
by which to recover the dependencies.)

I think this is an important case we need to be able to handle, so I
added a scenario to my test framework to exercise it and discovered that
my implementation was also buggy. Here's the fix:

Thanks for pointing this out Zane. We too had a buggy implementation for
handling inverted dependency. I had a hard look at our algorithm where
we were continuously merging the edges from new template into the edges
from previous updates. It was an optimized way of traversing the graph
in both forward and reverse order with out missing any resources. But,
when the dependencies are inverted,  this wouldn't work.

We have changed our algorithm. The changes in edges are noted down in
DB, only the delta of edges from previous template is calculated and
kept. At any given point of time, the graph table has all the edges from
current template and delta from previous templates. Each edge has
template ID associated with it.

The thing is, the cleanup dependencies aren't really about the template. The real resources really depend on other real resources. You can't delete a Volume before its VolumeAttachment, not because it says so in the template but because it will fail if you try. The template can give us a rough guide in advance to what those dependencies will be, but if that's all we keep then we are discarding information.

There may be multiple versions of a resource corresponding to one template version. Even worse, the actual dependencies of a resource change on a smaller time scale than an entire stack update (this is the reason the current implementation updates the template one resource at a time as we go).

Given that our Resource entries in the DB are in 1:1 correspondence with actual resources (we create a new one whenever we need to replace the underlying resource), I found it makes the most conceptual and practical sense to store the requirements in the resource itself, and update them at the time they actually change in the real world (bonus: introduces no new locking issues and no extra DB writes). I settled on this after a legitimate attempt at trying other options, but they didn't work out: https://github.com/zaneb/heat-convergence-prototype/commit/a62958342e8583f74e2aca90f6239ad457ba984d

For resource clean up, we start from the
first template (template which was completed and updates were made on
top of it, empty template otherwise), and move towards the current
template in the order in which the updates were issued, and for each
template the graph (edges if found for the template) is traversed in
reverse order and resources are cleaned-up.

I'm pretty sure this is backwards - you'll need to clean up newer resources first because they may reference resources from older templates. Also if you have a stubborn old resource that won't delete you don't want that to block cleanups of anything newer.

You're also serialising some of the work unnecessarily because you've discarded the information about dependencies that cross template versions, forcing you to clean up only one template version at a time.

The process ends up with
current template being traversed in reverse order and resources being
cleaned up. All the update-replaced resources from the older templates
(older updates in concurrent updates) are cleaned up in the order in
which they are suppose to be.

Resources are now tied to template, they have a template_id instead of
version. As we traverse the graph, we know which template we are working
on, and can take the relevant action on resource.

For rollback, another update is issued with the last completed template
(it is designed to have an empty template as last completed template by
default). The current template being worked on becomes predecessor for
the new incoming template. In case of rollback, the last completed
template becomes incoming new template, the current becomes the new
template's predecessor and the successor of last completed template will
have no predecessor. All these changes are available in the
'graph-version' branch. (The branch name is a misnomer though!)

I think it is simpler to think about stack and concurrent updates when
we associate resources and edges with template, and stack with current
template and its predecessors (if any).

It doesn't seem simple to me because it's trying to reconstruct reality from a lossy version of history. The simplest way to think about it, in my opinion is this: - When updating resources, respect their dependencies as given in the template - When checking resources to clean up, respect their actual, current real-world dependencies, and check replacement resources before the resources that they replaced. - Don't check a resource for clean up until it has been updated to the latest template.

I also think that we should decouple Resource from Stack. This is really
a hindrance when workers work on individual resources. The resource
should be abstracted enough from stack for the worker to work on the
resource alone. The worker should load the required resource plug-in and
start converging.

I think that's a worthy goal, and it would be really nice if we could load a Resource completely independently of its Stack, and I know this has always been a design goal of yours (hence you're caching the resource definition in the Resource rather than getting it from the template).

That said, I am convinced it's an unachievable goal, and I believe we should give up on it.

- We'll always need to load _some_ central thing (e.g. to find out if the current traversal is still the valid one), so it might as well be the Stack. - Existing plugin abominations like HARestarter expect a working Stack object to be provided so it can go hunting for other resources.

I think the best we can do is try to make heat.engine.stack.Stack as lazy as possible so that it only does extra work when strictly required, and just accept that the stack will always be loaded from the database.

I am also strongly in favour of treating the idea of caching the unresolved resource definition in the Resource table as a straight performance optimisation that is completely separate to the convergence work. It's going to be inevitably ugly because there is no template-format-independent way to serialise a resource definition (while resource definition objects themselves are designed to be inherently template-format-independent). Once phase 1 is complete we can decide whether it's worth it based on measuring the actual performance improvement.

(Note that we _already_ store the _resolved_ properties of the resource, which is what the observer will be comparing against for phase 2, so there should be no reason for the observer to need to load the stack.)

The READEME.rst is really helpful for bringing up the minimal devstack
and test the PoC. I also has some notes on design.


Zane, I have few questions:
1. Our current implementation is based on notifications from worker so
that the engine can take up next set of tasks. I don't see this in your
case. I think we should be doing this. It gels well with observer
notification mechanism. When the observer comes, it would send a
converge notification. Both, the provisioning of stack and the
continuous observation, happens with notifications (async message
passing). I see that the workers in your case pick up the parent when/if
it is done and schedules it or updates the sync point.

I'm not quite sure what you're asking here, so forgive me if I'm misunderstanding. What I think you're saying is that where my prototype propagates notifications thus:

  worker -> worker
         -> worker

(where -> is an async message)
you would prefer it to do:

  worker -> engine -> worker
                   -> worker

Is that right?

To me the distinction seems somewhat academic, given that we've decided that the engine and the worker will be the same process. I don't see a disadvantage to doing right away stuff that we know needs to be done right away. Obviously we should factor the code out tidily into a separate method where we can _also_ expose it as a notification that can be triggered by the continuous observer.

You mentioned above that you thought the workers should not ever load the Stack, and I think that's probably the reason you favour this approach: the 'worker' would always load just the Resource and the 'engine' (even though they're really the same) would always load just the Stack, right?

However, as I mentioned above, I think we'll want/have to load the Stack in the worker anyway, so eliminating the extra asynchronous call eliminates the performance penalty for having to do so.

2. The dependency graph travels everywhere. IMHO, we can keep the graph
in DB and let the workers work on a resource, and engine decide which
one to be scheduled next by looking at the graph. There wouldn't be a
need for a lock here, in the engine, the DB transactions should take
care of concurrent DB updates. Our new PoC follows this model.

I'm fine with keeping the graph in the DB instead of having it flow with the notifications.

3. The request ID is passed down to check_*_complete. Would the check
method be interrupted if new request arrives? IMHO, the check method
should not get interrupted. It should return back when the resource has
reached a concrete state, either failed or completed.

I agree, it should not be interrupted.

I've started to think of phase 1 and phase 2 like this:

1) Make locks more granular: stack-level lock becomes resource-level
2) Get rid of locks altogether

So in phase 1 we'll lock the resources and like you said, it will return back when it has reached a concrete state. In phase 2 we'll be able to just update the goal state for the resource and the observe/converge process will be able to automagically find the best way to that state regardless of whether it was in the middle of transitioning to another state or not. Or something. But that's for the future :)

4. Lot of synchronization issues which we faced in our PoC cannot be
encountered with the framework. How do we evaluate what happens when
synchronization issues are encountered (like stack lock kind of issues
which we are replacing with DB transaction).

Right, yeah, this is obviously the big known limitation of the simulator. I don't have a better answer other than to Think Very Hard about it.

Designing software means solving for hundreds of constraints - too many for a human to hold in their brain at the same time. The purpose of prototyping is to fix enough of the responses to those constraints in a concrete form to allow reasoning about the remaining ones to become tractable. If you fix solutions for *all* of the constraints, then what you've built is by definition not a prototype but the final product.

One technique available to us is to encapsulate the parts of the algorithm that are subject to synchronisation issues behind abstractions that offer stronger guarantees. Then in order to have confidence in the design we need only satisfy ourselves that we have analysed the guarantees correctly and that a concrete implementation offering those same guarantees is possible. For example, the SyncPoints are shown to work under the assumption that they are not subject to race conditions, and the SyncPoint code is small enough that we can easily see that it can be implemented in an atomic fashion using the same DB primitives already proven to work by StackLock. Therefore we can have a very high confidence (but not proof) that the overall algorithm will work when implemented in the final product.

Having Thought Very Hard about it, I'm as confident as I can be that I'm not relying on any synchronisation properties that can't be implemented using select-for-update on a single database row. There will of course be surprises at implementation time, but I hope that won't be one of them and anticipate that any changes required to the plan will be localised and not wide-ranging.

(This is in contrast BTW to my centralised-graph branch, linked above, where it became very obvious that it would require some sort of external locking - so there is reason to think that this process can reveal architectural problems related to synchronisation where they are present.)


OpenStack-dev mailing list

Reply via email to