Code review comment for lp://staging/~hazmat/pyjuju/unit-stop

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst
File source/drafts/stopping-units.rst (right):

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst#newcode4
source/drafts/stopping-units.rst:4: -------------
On 2012/03/21 20:07:53, niemeyer wrote:
> Sorry for nitpicking, but can we please stick to a consistent
convention for
> headers? I believe that's what we have today:

> Top level: ===
> Inner level: ---

done.

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst#newcode30
source/drafts/stopping-units.rst:30: while still allowing for
termination of errant children.
On 2012/03/21 20:07:53, niemeyer wrote:
> This is a very dense paragraph. Can it be simplified to something
like:

> """
> Juju records the desired state in the topology, and preserves that
state while
> the actions to accomplish it are still taking place.
> """

sounds good.

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst#newcode32
source/drafts/stopping-units.rst:32: How it works, what follows is a a
worked example going through a unit
On 2012/03/21 20:07:53, niemeyer wrote:
> "How it works, what follows"?

removed

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst#newcode58
source/drafts/stopping-units.rst:58: actions are recorded in the
topology as a key 'action' value 'destroy'
On 2012/03/21 20:07:53, niemeyer wrote:
> The "action: destroy" terminology feels a bit out of place here. This
is pretty
> much a procedure call, but the topology holds state instead. Something
like
> "destroyed: true" would be more appropriate. It remains true even
after the
> action has taken place.

action here was a really a name for a coordinated state transition, ie.
an implied final state. recording the state instead of the activity
sounds good. i have a minor preference for a generic key over per state
keys, as i can't think of any multi-valued concurrent activities/states
here, but not guessing via constraints in the face of ambiguity does
seem to be the wiser choice.

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst#newcode69
source/drafts/stopping-units.rst:69: - executing stop hook.
On 2012/03/21 20:07:53, niemeyer wrote:
> This should be done after departing from relations.
definitely

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst#newcode72
source/drafts/stopping-units.rst:72: - unit agent updates
/units/unit-x/stop to reflect the action has been performed.
On 2012/03/21 20:07:53, niemeyer wrote:
> That step doesn't seem necessary. We can reuse the existing liveness
mechanism
> that is already supported by the agent. When the agent dies, the
machine will
> know it.

> It also seems like a nice idea to have the stop ZooKeeper node
existing for as
> long as that's the intended state of the unit. We have to need that
node on
> startup for watching it anyway.

i wanted to avoid presence node usage, due its known problems with
session expiration. ie if an agent has a restart in the middle of the
stop process, the machine agent watch may erroneously think the agent is
up. The stop node explicitness feels more robust in this context, and as
you've noted its nesc for the coordination aspect anyways. Its also nice
to clean up the local coordination structure when the coordination is
done.

https://codereview.appspot.com/5847053/diff/5001/source/drafts/stopping-units.rst#newcode94
source/drafts/stopping-units.rst:94: and garbage collects their topology
footprint and zk state.
On 2012/03/21 20:07:53, niemeyer wrote:
> What about garbage collecting on the next action after the decision
that it's
> fine to GC it? I don't think we need a specific process that has the
task of
> GCing the topology.

> In both cases, though, we have an issue: when is it ok to GC it? This
proposal
> is not providing any means for deciding on that, which means the data
becomes
> eternal. That hole indicates postponing that side of the problem may
not be a
> good idea. We can do something simple, but it'd be good implement
cleaning up at
> the same time.

There is some cost associated to doing gc every user action, its n
roundtrips for the client to check. I agree it is simpler for user-gc,
but there are other forms of garbage that could be opportunistically
collected with an agent (ie. old /relations, non topology referenced
service nodes) Its not clear that garbage will be a problem in practice
though outside of the topology, so i'm fine with the user initiated gc
confined to topology intents.

https://codereview.appspot.com/5847053/

« Back to merge proposal