Merge lp:~themue/juju-core/go-state-lifecycle-relation into lp:~juju/juju-core/trunk

[Frank Mueller]

state: lifecycle implementation started

The branch contains the type definition and some utility
functions for lifecycles in general. A first integration
into relation is done and the tests changed (very naive by
just setting the lifecycle state before removing). So the
tests of state are working while any outer relying code
may fail. This has to be done in a follow-up CL.

https://codereview.appspot.com/6455065/

[Roger Peppe (rogpeppe)]

looks plausible. a few minor comments below.

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode18
state/lifecycle.go:18: func (l Life) checkNext(next Life) error {
this seems a little more involved than it has to be.
perhaps something like this might be better?

func (l Life) checkNext(next Life) error {
 switch {
 case l == next:
  return nil
 case l == Alive && next == Dying:
  return nil
 case l == Dying && next == Dead:
  return nil
 }
 return fmt.Errorf("illegal lifecycle state change from %q to %q", l,
next)
}

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode38
state/lifecycle.go:38: func createLifeNode(zk *zookeeper.Conn, basepath
string) (err error) {
does the life state need to be stored in a separate node? most entities
already have an associated config node, and the life state could be
stored there. maybe there's a good reason for the separation though.

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode40
state/lifecycle.go:40: defer errorContextf(&err, "cannot create
lifecycle state node %q", lifepath)
i wonder if the error context is necessary here, given that the
zookeeper error mentions the path and the create operation.

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode48
state/lifecycle.go:48: defer errorContextf(&err, "cannot write lifecycle
state node %q", lifepath)
ditto

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode56
state/lifecycle.go:56: defer errorContextf(&err, "cannot read lifecycle
state node %q", lifepath)
ditto

https://codereview.appspot.com/6455065/diff/1/state/lifecycle_test.go
File state/lifecycle_test.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle_test.go#newcode21
state/lifecycle_test.go:21: c.Assert(Alive.checkNext(Dying), IsNil)
this seems like a good place for a table-driven test.

https://codereview.appspot.com/6455065/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode101
state/relation.go:101: func (r *Relation) readLife() (err error) {
why is this a different method from Life?

https://codereview.appspot.com/6455065/

[Aram Hăvărneanu (aramh)]

No changes to watchers in this CL?

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode18
state/lifecycle.go:18: func (l Life) checkNext(next Life) error {
s/checkNext/isNextValid/ perhaps?

https://codereview.appspot.com/6455065/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode101
state/relation.go:101: func (r *Relation) readLife() (err error) {
On 2012/07/31 07:48:58, rog wrote:
> why is this a different method from Life?

Because of potential caching in the future, perhaps.

https://codereview.appspot.com/6455065/

[William Reade (fwereade)]

Good start, couple of issues

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode18
state/lifecycle.go:18: func (l Life) checkNext(next Life) error {
validChange?

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode38
state/lifecycle.go:38: func createLifeNode(zk *zookeeper.Conn, basepath
string) (err error) {
On 2012/07/31 07:48:58, rog wrote:
> does the life state need to be stored in a separate node? most
entities already
> have an associated config node, and the life state could be stored
there. maybe
> there's a good reason for the separation though.

ISTM that we'll be watching a *lot* of these, and handling events for
every unrelated change will be somewhat tedious and unnecessary so, or
reflection, I'm +1 on separate nodes.

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode40
state/lifecycle.go:40: defer errorContextf(&err, "cannot create
lifecycle state node %q", lifepath)
On 2012/07/31 07:48:58, rog wrote:
> i wonder if the error context is necessary here, given that the
zookeeper error
> mentions the path and the create operation.

I reckon the extra information has some value, and is more readable
besides.

https://codereview.appspot.com/6455065/diff/1/state/lifecycle_test.go
File state/lifecycle_test.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle_test.go#newcode21
state/lifecycle_test.go:21: c.Assert(Alive.checkNext(Dying), IsNil)
On 2012/07/31 07:48:58, rog wrote:
> this seems like a good place for a table-driven test.
+1

https://codereview.appspot.com/6455065/diff/1/state/lifecycle_test.go#newcode50
state/lifecycle_test.go:50: defer testing.ZkRemoveTree(s.ZkConn,
basepath)
If ZkConnSuite doesn't nuke ZK at the end of each test, surely it
should?

https://codereview.appspot.com/6455065/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode79
state/relation.go:79: life Life
I'm not sure this pulls its weight.

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode119
state/relation.go:119: return writeLifeNode(r.st.zk, r.zkPath(), r.life)
We probably shouldn't alter r.life until we know the write succeeded.

https://codereview.appspot.com/6455065/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6455065/diff/1/state/service.go#newcode211
state/service.go:211: r := &Relation{s.st, key, Alive,
make([]RelationEndpoint, len(tr.Endpoints))}
Shouldn't you get the actual life value rather than just assume it's
alive? TBH this life field seems to be causing more problems than it
solves...

https://codereview.appspot.com/6455065/diff/1/state/service_test.go
File state/service_test.go (left):

https://codereview.appspot.com/6455065/diff/1/state/service_test.go#oldcode443
state/service_test.go:443: err = s.State.RemoveRelation(rel)
I haven't seen any new tests for RemoveRelation -- surely it should
panic if the relation is not Dead?

https://codereview.appspot.com/6455065/diff/1/state/state.go
File state/state.go (righ...

[Aram Hăvărneanu (aramh)]

https://codereview.appspot.com/6455065/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode79
state/relation.go:79: life Life
On 2012/07/31 09:12:33, fwereade wrote:
> I'm not sure this pulls its weight.

Not sure what weight you are referring to, but this is the way it's done
in mstate after a few previous heavier iterations.

https://codereview.appspot.com/6455065/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6455065/diff/1/state/service.go#newcode211
state/service.go:211: r := &Relation{s.st, key, Alive,
make([]RelationEndpoint, len(tr.Endpoints))}
> Shouldn't you get the actual life value rather than just assume it's
> alive? TBH this life field seems to be causing more problems than it
> solves...

I think this is just part of the initial lifecycle bootstrap, after we
modify state client this would change in a subsequent CL.

https://codereview.appspot.com/6455065/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6455065/diff/1/state/state.go#newcode427
state/state.go:427: return fmt.Errorf("not found")
> I suspect this shouldn't be an error any more... we're just cleaning
up,
> we might not be the only concurrent caller, and if it's already gone
> then we're happy.

I thought there's one one agent responsible for calling Remove*, the
single one receiving the Dead event. I think this is an error to be
called more than once. The corrective agent might call this
independently,
but the CE should deal with it failing anyway.

https://codereview.appspot.com/6455065/

[Aram Hăvărneanu (aramh)]

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode9
state/lifecycle.go:9: type Life string
Hmm, this simplifies returning errors, but the integer approach in
mstate makes state change checking trivial because all you need to do is

   if (newLife < life) { ...

https://codereview.appspot.com/6455065/

[William Reade (fwereade)]

On 2012/07/31 09:36:47, aram wrote:
> https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go
> File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode9
> state/lifecycle.go:9: type Life string
> Hmm, this simplifies returning errors, but the integer approach in
mstate makes
> state change checking trivial because all you need to do is

> if (newLife < life) { ...

Does a string cost us so much as to not be worth the clarity?

https://codereview.appspot.com/6455065/

[William Reade (fwereade)]

On 2012/07/31 09:29:17, aram wrote:
> https://codereview.appspot.com/6455065/diff/1/state/relation.go
> File state/relation.go (right):

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode79
> state/relation.go:79: life Life
> On 2012/07/31 09:12:33, fwereade wrote:
> > I'm not sure this pulls its weight.

> Not sure what weight you are referring to, but this is the way it's
done in
> mstate after a few previous heavier iterations.

> https://codereview.appspot.com/6455065/diff/1/state/service.go
> File state/service.go (right):

https://codereview.appspot.com/6455065/diff/1/state/service.go#newcode211
> state/service.go:211: r := &Relation{s.st, key, Alive,
make([]RelationEndpoint,
> len(tr.Endpoints))}
> > Shouldn't you get the actual life value rather than just assume it's
> > alive? TBH this life field seems to be causing more problems than it
> > solves...

> I think this is just part of the initial lifecycle bootstrap, after we
> modify state client this would change in a subsequent CL.

If we just dropped the field and called Life everywhere we needed it,
then we could have code that wasn't actively inconsistent. We could add
the caching as required in a subsequent CL.

> https://codereview.appspot.com/6455065/diff/1/state/state.go
> File state/state.go (right):

https://codereview.appspot.com/6455065/diff/1/state/state.go#newcode427
> state/state.go:427: return fmt.Errorf("not found")
> > I suspect this shouldn't be an error any more... we're just cleaning
up,
> > we might not be the only concurrent caller, and if it's already gone
> > then we're happy.

> I thought there's one one agent responsible for calling Remove*, the
> single one receiving the Dead event. I think this is an error to be
> called more than once. The corrective agent might call this
independently,
> but the CE should deal with it failing anyway.

My point is just: there are concurrent callers of this. The end goal is
that the relation no longer exist. At least one of the callers will see
the relation already gone from the topology; if we treat this as an
error, but the other client failed before it could delete the ZK nodes,
how will we ever be able to collect the relation nodes?

https://codereview.appspot.com/6455065/

[Aram Hăvărneanu (aramh)]

> Does a string cost us so much as to not be worth the clarity?

Actually it was about improving clarity. Here's rog's suggestion:

http://play.golang.org/p/ZLVcLY96Oy

https://codereview.appspot.com/6455065/

[Gustavo Niemeyer (niemeyer)]

First round:

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode18
state/lifecycle.go:18: func (l Life) checkNext(next Life) error {
checkNext seems like a bad approach altogether. We need to know more
than whether a change is allowed to make a decision of what to do (e.g.
Alive => Dying when Dead is fine, but does nothing). I suggest inlining
logic into SetLife.

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode38
state/lifecycle.go:38: func createLifeNode(zk *zookeeper.Conn, basepath
string) (err error) {
On 2012/07/31 09:12:33, fwereade wrote:
> On 2012/07/31 07:48:58, rog wrote:
> > does the life state need to be stored in a separate node? most
entities
> already
> > have an associated config node, and the life state could be stored
there.
> maybe
> > there's a good reason for the separation though.

> ISTM that we'll be watching a *lot* of these, and handling events for
every
> unrelated change will be somewhat tedious and unnecessary so, or
reflection, I'm
> +1 on separate nodes.

I agree with rog here. We have to watch a lot of things in general.
Maybe I just haven't seen the issue clearly yet, but it sounds to me
like separating the nodes will actually create *more* things for
watching, rather than less.

https://codereview.appspot.com/6455065/diff/1/state/lifecycle.go#newcode56
state/lifecycle.go:56: defer errorContextf(&err, "cannot read lifecycle
state node %q", lifepath)
This is all changing given we don't need independent nodes, but FWIW
let's please not sprawl errorContextf more than strictly necessary.
fmt.Errorf works well.

https://codereview.appspot.com/6455065/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode79
state/relation.go:79: life Life
On 2012/07/31 09:29:17, aram wrote:
> On 2012/07/31 09:12:33, fwereade wrote:
> > I'm not sure this pulls its weight.

> Not sure what weight you are referring to, but this is the way it's
done in
> mstate after a few previous heavier iterations.

I believe mstate is different in that the value is mapped straight from
the database. The issue is related to the caching topic we debated at
the end of last week.

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode79
state/relation.go:79: life Life
Frank, have you seen the work by rog on agent tooling? It sounds like we
could have a single implementation of resourceLife that could be used
everywhere.

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode114
state/relation.go:114: if err := r.life.checkNext(life); err != nil {
This doesn't work, as it's testing a memory value. The database could
have shifted several times while we're sitting on our couch evaluating
this. It must be put in a retry change block.

Also, checkNext is a red-herring. You must do a local test here that
takes into account what you want the end result to be. Alive is *never*
acceptable. Dying is *always* fine, but only does anything if it is
currenlty Alive, as someone else could have shifted from A...

[Aram Hăvărneanu (aramh)]

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle.go#newcode96
state/lifecycle.go:96: value, found := config.Get(lifeKey)
Isn't there a race between config.Get() and config.{Set,Write}()?

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle_test.go
File state/lifecycle_test.go (right):

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle_test.go#newcode90
state/lifecycle_test.go:90: c.Assert(func() { lc.SetLife(test.panicLife)
}, PanicMatches, panicMessage)
Did not know about PanicMatches, thanks!

https://codereview.appspot.com/6455065/diff/11001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6455065/diff/11001/state/state.go#newcode419
state/state.go:419: return fmt.Errorf("cannot retrieve lifecacle state
of relation %q: %s", r, err)
s/lifecacle/lifecycle/

https://codereview.appspot.com/6455065/

[Aram Hăvărneanu (aramh)]

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle.go#newcode26
state/lifecycle.go:26: var transitions = [nLife][nLife]bool{
I love shorter names in general, but perhaps validTransitions is a
better name here?

https://codereview.appspot.com/6455065/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/11001/state/lifecycle.go#newcode91
state/lifecycle.go:91: func (lc *lifecycle) SetLife(next Life) error {
The issues described last week are still not handled:

https://codereview.appspot.com/6455065/diff/1/state/relation.go#newcode114
state/relation.go:114: if err := r.life.checkNext(life); err != nil {
This doesn't work, as it's testing a memory value. The database could
have
shifted several times while we're sitting on our couch evaluating this.
It must
be put in a retry change block.

Also, checkNext is a red-herring. You must do a local test here that
takes into
account what you want the end result to be. Alive is *never* acceptable.
Dying
is *always* fine, but only does anything if it is currenlty Alive, as
someone
else could have shifted from Alive to Dying and to Dead concurrently,
and the
Dying effect is guaranteed even then. Setting to Dead is fine unless
it's Alive,
but only does anything if not already Dead.

https://codereview.appspot.com/6455065/diff/11001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6455065/diff/11001/state/relation.go#newcode111
state/relation.go:111: func (r *Relation) Life() Life {
We shouldn't have to declare those methods for every type. Embedding a
reasonable type should be enough. You can use Roger's work as a
reference.

https://codereview.appspot.com/6455065/diff/11001/state/relation.go#newcode125
state/relation.go:125: // Refresh reloads the cached data of the
relation.
// Refresh reloads the state of the lifecycle.

This is somewhat unfortunate since it's caching a specific setting and
ignoring the rest, but I guess it's a good compromise to maintain
compatibility with mstate for the moment.

https://codereview.appspot.com/6455065/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode45
state/lifecycle.go:45: // init initializes the lifecycle with the state
Alive .
Why do we need this? We should be creating the actual nodes with life
set to Alive in the first place, rather than changing them to Alive
after the fact via this method.

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode88
state/lifecycle.go:88: // Attention! The config map is defined by the
ConfigNode.
As we debated ad infinitum, there are other configuration nodes that we
manipulate without ConfigNode, so this isn't really a relevant note.

What might be more useful is:

// Note that there are unknown keys in the node.

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode98
state/lifecycle.go:98: if !validTransitions[current][next] {
Please add a test verifying that SetLife(Dying) when the current state
is Dead works and does nothing. This was mentioned in the very first
review I made and still seems unhandled.

https://codereview.appspot.com/6455065/diff/17001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6455065/diff/17001/state/service.go#newcode212
state/service.go:212: if err := r.Refresh(); err != nil {
This workflow doesn't make sense:

1) r := newRelation
2) r.Refresh
3) r.endpoint = ...

If we're going to be adding Refresh now, the sites that call it should
be sane, otherwise we're corrupting the whole logic in a way that we
won't be able to get out of in the near future. Please ping me online
for us to sort this out.

https://codereview.appspot.com/6455065/diff/17001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6455065/diff/17001/state/state.go#newcode348
state/state.go:348: err = newRelation(s, relationKey).lifecycle.init()
Why writing a second time to the node just for setting life=alive?

https://codereview.appspot.com/6455065/diff/17001/state/state.go#newcode401
state/state.go:401: if err = r.Refresh(); err != nil {
Same case as before.

https://codereview.appspot.com/6455065/

[lp:~themue/juju-core/go-state-lifecycle-relation]

359. By Frank Mueller on 2012-08-21

state: merged trunk to stay up-to-date

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode45
state/lifecycle.go:45: // init initializes the lifecycle with the state
Alive .
On 2012/08/14 12:36:06, TheMue wrote:
> On 2012/08/13 15:48:49, niemeyer wrote:
> > Why do we need this? We should be creating the actual nodes with
life set to
> > Alive in the first place, rather than changing them to Alive after
the fact
> via
> > this method.

> Maybe this comment is misleading. init() is to write the initial Alive
into the
> config node. init() is called immediately after the creation of the
instance, so
> the config node should be empty so far. But I preferred to take care
that it's
> really so.

The comment isn't misleading. The logic itself is wrong. You can't
create a node that has no lifecycle state and then initialize it after
the fact. Meanwhile, someone else will pick it up, and will see wrong
state.

This, specifically, is a relevant race condition:

98 r := newRelation(st, key)
99 if err := r.lifecycle.init(); err != nil {

We've been going back and forth on this, and I'm feeling unproductive
reviewing this over and over about the same problems.

I suggest we stop work on state, put that aside, and focus entirely on
mstate. Let's implement watches and presence, and move forward.

https://codereview.appspot.com/6455065/

[Gustavo Niemeyer (niemeyer)]

On 2012/08/28 17:24:11, TheMue wrote:
> On 2012/08/28 17:15:06, niemeyer wrote:
> > https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go
> > File state/lifecycle.go (right):
> >
> >
https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode45
> > state/lifecycle.go:45: // init initializes the lifecycle with the
state Alive
> .
> > On 2012/08/14 12:36:06, TheMue wrote:
> > > On 2012/08/13 15:48:49, niemeyer wrote:
> > > > Why do we need this? We should be creating the actual nodes with
life set
> to
> > > > Alive in the first place, rather than changing them to Alive
after the
> fact
> > > via
> > > > this method.
> > >
> > > Maybe this comment is misleading. init() is to write the initial
Alive into
> > the
> > > config node. init() is called immediately after the creation of
the
> instance,
> > so
> > > the config node should be empty so far. But I preferred to take
care that
> it's
> > > really so.
> >
> > The comment isn't misleading. The logic itself is wrong. You can't
create a
> node
> > that has no lifecycle state and then initialize it after the fact.
Meanwhile,
> > someone else will pick it up, and will see wrong state.
> >
> > This, specifically, is a relevant race condition:
> >
> > 98 r := newRelation(st, key)
> > 99 if err := r.lifecycle.init(); err != nil {
> >
> > We've been going back and forth on this, and I'm feeling
unproductive
> reviewing
> > this over and over about the same problems.
> >
> > I suggest we stop work on state, put that aside, and focus entirely
on mstate.
> > Let's implement watches and presence, and move forward.

> Take a look at newRelation(), it doesn't create a node, only the
struct with
> lifecycle initialized:

What is this logic doing Frank:

   46 func (lc *lifecycle) init() error {
   47 config, err := readConfigNode(lc.zk, lc.path)
   48 if err != nil {
   49 return err
   50 }
   51 config.Set(lifeKey, Alive)
   52 changes, err := config.Write()
   53 if err != nil {
   54 return err
   55 }

Either:

1) The node was created before without a lifecycle

or

2) An empty node is being read, and is being written down
    just with lifecycle information and nothing else

Both are wrong.

This is feeling very unproductive to me, Frank. This is in review for
several weeks, and the issues that are brought up are repeatedly
receiving replies that indicate lack of care instead of being fixed.

I'm marking the branch as rejected.

https://codereview.appspot.com/6455065/

Unmerged revisions

359. By Frank Mueller on 2012-08-21

state: merged trunk to stay up-to-date

358. By Frank Mueller on 2012-08-16

state: changed lifecycle from SetLife() to Kill() and Die()

357. By Frank Mueller on 2012-08-08

state: changed lifecycle after review, now uses RetryChange and is embedable

356. By Frank Mueller on 2012-08-06

state: merged trunk and worked off the review comments

355. By Frank Mueller on 2012-07-31

state: first lifecycle utils and implementation for relation

354. By Roger Peppe on 2012-07-30

state: implement MachineInfoWatcher

R=niemeyer
CC=
https://codereview.appspot.com/6447054

353. By William Reade on 2012-07-27

add RelationState type...

...and use it to reconcile HookQueue state with initial watcher event on
load.

As part of this, I added a RelationId to HookQueue, and return it in every
HookInfo, so that we can identify what relation the hook is actually meant
to run against.

R=niemeyer
CC=
https://codereview.appspot.com/6441047

352. By Roger Peppe on 2012-07-27

environs/ec2: start agents via agent-specific symbolic links

R=niemeyer
CC=
https://codereview.appspot.com/6454057

351. By William Reade on 2012-07-27

HookQueue now looks more like a watcher

R=niemeyer
CC=
https://codereview.appspot.com/6453043

350. By Roger Peppe on 2012-07-27

worker/machiner: fix test to add new Container method

R=
CC=
https://codereview.appspot.com/6440053

=== added file 'state/lifecycle.go'
--- state/lifecycle.go	1970-01-01 00:00:00 +0000
+++ state/lifecycle.go	2012-08-21 08:50:23 +0000
@@ -0,0 +1,121 @@
+package state
+
+import (
+	"fmt"
+	"launchpad.net/goyaml"
+	"launchpad.net/gozk/zookeeper"
+)
+
+// Life represents the lifecycle state of a state entity.
+type Life string
+
+const (
+	Alive Life = "alive"
+	Dying Life = "dying"
+	Dead  Life = "dead"
+)
+
+const lifeKey = "life"
+
+// lifecycle manages the lifecycle of state entities
+// in the config node.
+type lifecycle struct {
+	zk   *zookeeper.Conn
+	path string
+}
+
+// init initializes the lifecycle in the config node with the state Alive .
+func (lc *lifecycle) init() error {
+	config, err := readConfigNode(lc.zk, lc.path)
+	if err != nil {
+		return err
+	}
+	config.Set(lifeKey, Alive)
+	changes, err := config.Write()
+	if err != nil {
+		return err
+	}
+	// Check if only 'life = Alive' has been added. 
+	if len(changes) != 1 ||
+		changes[0].Type != ItemAdded ||
+		(changes[0].OldValue != nil && changes[0].NewValue != Alive) {
+		return fmt.Errorf("initial lifecycle state cannot be set")
+	}
+	return nil
+}
+
+// Life returns the cached lifecycle state.
+func (lc *lifecycle) Life() (Life, error) {
+	config, err := readConfigNode(lc.zk, lc.path)
+	if err != nil {
+		return "", err
+	}
+	value, found := config.Get(lifeKey)
+	if !found {
+		panic("lifecycle state is not set")
+	}
+	return Life(value.(string)), nil
+}
+
+// Kill changes the lifecycle to 'Dying' if it's 'Alive'
+// or already 'Dying'. No change will happen if the lifecycle
+// is 'Dead'.
+func (lc *lifecycle) Kill() error {
+	kill := func(oldYaml string, stat *zookeeper.Stat) (string, error) {
+		// Note that there are unknown keys in the node.
+		var config map[string]interface{}
+		if err := goyaml.Unmarshal([]byte(oldYaml), &config); err != nil {
+			return "", err
+		}
+		tmpLife, found := config[lifeKey]
+		if !found {
+			panic("lifecycle state is not set")
+		}
+		current := Life(tmpLife.(string))
+		if current == Alive {
+			config[lifeKey] = Dying
+			newYaml, err := goyaml.Marshal(config)
+			if err != nil {
+				return "", err
+			}
+			return string(newYaml), nil
+		}
+		return oldYaml, nil
+	}
+	if err := lc.zk.RetryChange(lc.path, 0, zkPermAll, kill); err != nil {
+		return err
+	}
+	return nil
+}
+
+// Die changes the lifecycle to 'Dead' if it's 'Dying'.
+func (lc *lifecycle) Die() error {
+	die := func(oldYaml string, stat *zookeeper.Stat) (string, error) {
+		// Note that there are unknown keys in the node.
+		var config map[string]interface{}
+		if err := goyaml.Unmarshal([]byte(oldYaml), &config); err != nil {
+			return "", err
+		}
+		tmpLife, found := config[lifeKey]
+		if !found {
+			panic("lifecycle state is not set")
+		}
+		current := Life(tmpLife.(string))
+		switch current {
+		case Alive:
+			panic("lifecycle state is alive, must not die")
+		case Dying:
+			config[lifeKey] = Dead
+			newYaml, err := goyaml.Marshal(config)
+			if err != nil {
+				return "", err
+			}
+			return string(newYaml), nil
+		}
+		return oldYaml, nil
+	}
+	if err := lc.zk.RetryChange(lc.path, 0, zkPermAll, die); err != nil {
+		return err
+	}
+	return nil
+}

=== added file 'state/lifecycle_test.go'
--- state/lifecycle_test.go	1970-01-01 00:00:00 +0000
+++ state/lifecycle_test.go	2012-08-21 08:50:23 +0000
@@ -0,0 +1,72 @@
+package state
+
+import (
+	"fmt"
+	. "launchpad.net/gocheck"
+	"launchpad.net/gozk/zookeeper"
+	"launchpad.net/juju-core/testing"
+)
+
+type LifecycleSuite struct {
+	testing.ZkConnSuite
+	t *topology
+}
+
+var _ = Suite(&LifecycleSuite{})
+
+func (s *LifecycleSuite) TearDownSuite(c *C) {
+	s.ZkConn.Close()
+}
+
+func (s *LifecycleSuite) SetUpTest(c *C) {
+	zkPermAll := zookeeper.WorldACL(zookeeper.PERM_ALL)
+	_, err := s.ZkConn.Create("/r", "", 0, zkPermAll)
+	c.Assert(err, IsNil)
+}
+
+func (s *LifecycleSuite) TearDownTest(c *C) {
+	testing.ZkRemoveTree(s.ZkConn, "/r")
+}
+
+var lifecyleTests = []struct {
+	prepare    func(lc lifecycle) error
+	change     func(lc lifecycle) error
+	panicMatch string
+	life       Life
+}{
+	{nil, nil, "", Alive},
+	{nil, func(lc lifecycle) error { return lc.Kill() }, "", Dying},
+	{nil, func(lc lifecycle) error { return lc.Die() }, "lifecycle state is alive, must not die", ""},
+	{func(lc lifecycle) error { return lc.Kill() }, func(lc lifecycle) error { return lc.Kill() }, "", Dying},
+	{func(lc lifecycle) error { return lc.Kill() }, func(lc lifecycle) error { return lc.Die() }, "", Dead},
+	{func(lc lifecycle) error { lc.Kill(); return lc.Die() }, func(lc lifecycle) error { return lc.Kill() }, "", Dead},
+	{func(lc lifecycle) error { lc.Kill(); return lc.Die() }, func(lc lifecycle) error { return lc.Die() }, "", Dead},
+}
+
+func (s *LifecycleSuite) TestLifecycle(c *C) {
+	for i, test := range lifecyleTests {
+		c.Logf("test %d", i)
+		lc := lifecycle{
+			zk:   s.ZkConn,
+			path: fmt.Sprintf("/r/%d", i),
+		}
+		err := lc.init()
+		c.Assert(err, IsNil)
+		life, err := lc.Life()
+		c.Assert(err, IsNil)
+		c.Assert(life, Equals, Alive)
+		if test.prepare != nil {
+			c.Assert(test.prepare(lc), IsNil)
+		}
+		if test.change != nil {
+			if test.panicMatch != "" {
+				c.Assert(func() { test.change(lc) }, PanicMatches, test.panicMatch)
+			} else {
+				c.Assert(test.change(lc), IsNil)
+				life, err = lc.Life()
+				c.Assert(err, IsNil)
+				c.Assert(life, Equals, test.life)
+			}
+		}
+	}
+}

=== modified file 'state/relation.go'
--- state/relation.go	2012-08-17 12:10:32 +0000
+++ state/relation.go	2012-08-21 08:50:23 +0000
@@ -78,6 +78,28 @@
 	st        *State
 	key       string
 	endpoints []RelationEndpoint
+	lifecycle
+}
+
+func newRelation(st *State, key string) *Relation {
+	r := &Relation{
+		st:        st,
+		key:       key,
+		endpoints: []RelationEndpoint{},
+	}
+	r.lifecycle = lifecycle{
+		zk:   st.zk,
+		path: r.zkConfigPath(),
+	}
+	return r
+}
+
+func createRelation(st *State, key string) (*Relation, error) {
+	r := newRelation(st, key)
+	if err := r.lifecycle.init(); err != nil {
+		return nil, err
+	}
+	return r, nil
 }
 
 func (r *Relation) String() string {
@@ -155,6 +177,16 @@
 	}, nil
 }
 
+// zkPath returns the ZooKeeper base path for the relation.
+func (r *Relation) zkPath() string {
+	return fmt.Sprintf("/relations/%s", r.key)
+}
+
+// zkConfigPath returns the ZooKeeper path for the relation configuration.
+func (r *Relation) zkConfigPath() string {
+	return r.zkPath() + "/config"
+}
+
 // RelationUnit holds information about a single unit in a relation, and
 // allows clients to conveniently access unit-specific functionality.
 type RelationUnit struct {

=== modified file 'state/relation_internal_test.go'
--- state/relation_internal_test.go	2012-07-26 18:40:19 +0000
+++ state/relation_internal_test.go	2012-08-21 08:50:23 +0000
@@ -18,7 +18,7 @@
 		RelationEndpoint{"jeff", "ifce", "group", RolePeer, charm.ScopeGlobal},
 		RelationEndpoint{"mike", "ifce", "group", RolePeer, charm.ScopeGlobal},
 		RelationEndpoint{"bill", "ifce", "group", RolePeer, charm.ScopeGlobal},
-	}}
+	}, lifecycle{nil, ""}}
 	eps, err := r.RelatedEndpoints("mike")
 	c.Assert(err, IsNil)
 	c.Assert(eps, DeepEquals, []RelationEndpoint{

=== modified file 'state/relation_test.go'
--- state/relation_test.go	2012-08-17 09:13:31 +0000
+++ state/relation_test.go	2012-08-21 08:50:23 +0000
@@ -86,12 +86,16 @@
 	assertOneRelation(c, req, 0, reqep, proep)
 
 	// Remove the relation, and check it can't be removed again.
+	err = rel.Kill()
+	c.Assert(err, IsNil)
+	err = rel.Die()
+	c.Assert(err, IsNil)
 	err = s.State.RemoveRelation(rel)
 	c.Assert(err, IsNil)
 	assertNoRelations(c, pro)
 	assertNoRelations(c, req)
 	err = s.State.RemoveRelation(rel)
-	c.Assert(err, ErrorMatches, `cannot remove relation "pro:foo req:bar": not found`)
+	c.Assert(err, ErrorMatches, `cannot remove relation "pro:foo req:bar": relation with key .* not found`)
 
 	// Check that we can add it again if we want to; but this time,
 	// give one of the endpoints container scope and check that both
@@ -120,11 +124,15 @@
 	assertOneRelation(c, peer, 0, peerep)
 
 	// Remove the relation, and check it can't be removed again.
+	err = rel.Kill()
+	c.Assert(err, IsNil)
+	err = rel.Die()
+	c.Assert(err, IsNil)
 	err = s.State.RemoveRelation(rel)
 	c.Assert(err, IsNil)
 	assertNoRelations(c, peer)
 	err = s.State.RemoveRelation(rel)
-	c.Assert(err, ErrorMatches, `cannot remove relation "peer:baz": not found`)
+	c.Assert(err, ErrorMatches, `cannot remove relation "peer:baz": relation with key .* not found`)
 }
 
 func (s *RelationSuite) TestRemoveServiceRemovesRelations(c *C) {
@@ -133,6 +141,10 @@
 	peerep := state.RelationEndpoint{"peer", "ifce", "baz", state.RolePeer, charm.ScopeGlobal}
 	rel, err := s.State.AddRelation(peerep)
 	c.Assert(err, IsNil)
+	err = rel.Kill()
+	c.Assert(err, IsNil)
+	err = rel.Die()
+	c.Assert(err, IsNil)
 	err = s.State.RemoveService(peer)
 	c.Assert(err, IsNil)
 	_, err = s.State.Service("peer")
@@ -140,7 +152,44 @@
 	_, err = s.State.Relation(peerep)
 	c.Assert(err, ErrorMatches, `cannot get relation "peer:baz": relation does not exist`)
 	err = s.State.RemoveRelation(rel)
-	c.Assert(err, ErrorMatches, `cannot remove relation "peer:baz": not found`)
+	c.Assert(err, ErrorMatches, `cannot remove relation "peer:baz": relation with key .* not found`)
+}
+
+func (s *RelationSuite) TestLifecycle(c *C) {
+	peer, err := s.State.AddService("peer", s.charm)
+	c.Assert(err, IsNil)
+	peerep := state.RelationEndpoint{"peer", "ifce", "baz", state.RolePeer, charm.ScopeGlobal}
+	assertNoRelations(c, peer)
+
+	rel, err := s.State.AddRelation(peerep)
+	c.Assert(err, IsNil)
+	life, err := rel.Life()
+	c.Assert(err, IsNil)
+	c.Assert(life, Equals, state.Alive)
+
+	// Check legal next state.
+	err = rel.Kill()
+	c.Assert(err, IsNil)
+	life, err = rel.Life()
+	c.Assert(err, IsNil)
+	c.Assert(life, Equals, state.Dying)
+
+	// Check legal repeated state setting.
+	err = rel.Kill()
+	c.Assert(err, IsNil)
+	life, err = rel.Life()
+	c.Assert(err, IsNil)
+	c.Assert(life, Equals, state.Dying)
+
+	// Check non-dead removal.
+	c.Assert(func() { s.State.RemoveRelation(rel) }, PanicMatches, `relation .* is not dead`)
+
+	// Check final state.
+	err = rel.Die()
+	c.Assert(err, IsNil)
+	life, err = rel.Life()
+	c.Assert(err, IsNil)
+	c.Assert(life, Equals, state.Dead)
 }
 
 func assertNoRelations(c *C, srv *state.Service) {
@@ -247,6 +296,10 @@
 	assertSettings(ru1, map[string]interface{}{})
 
 	// Trash the relation and check we can't get anything any more.
+	err = rel.Kill()
+	c.Assert(err, IsNil)
+	err = rel.Die()
+	c.Assert(err, IsNil)
 	err = s.State.RemoveRelation(rel)
 	c.Assert(err, IsNil)
 	_, err = ru0.ReadSettings("peer/1")

=== modified file 'state/service.go'
--- state/service.go	2012-08-17 12:10:32 +0000
+++ state/service.go	2012-08-21 08:50:23 +0000
@@ -209,8 +209,7 @@
 	}
 	relations := []*Relation{}
 	for key, tr := range trs {
-		r := &Relation{s.st, key, make([]RelationEndpoint, len(tr.Endpoints))}
-		i := 0
+		r := newRelation(s.st, key)
 		for _, tep := range tr.Endpoints {
 			sname := s.name
 			if tep.Service != s.key {
@@ -218,10 +217,9 @@
 					return nil, err
 				}
 			}
-			r.endpoints[i] = RelationEndpoint{
+			r.endpoints = append(r.endpoints, RelationEndpoint{
 				sname, tr.Interface, tep.RelationName, tep.RelationRole, tr.Scope,
-			}
-			i++
+			})
 		}
 		relations = append(relations, r)
 	}

=== modified file 'state/service_test.go'
--- state/service_test.go	2012-07-25 12:25:39 +0000
+++ state/service_test.go	2012-08-21 08:50:23 +0000
@@ -440,6 +440,10 @@
 	assertNoChange()
 
 	// Remove one of the relations; check change.
+	err = rel.Kill()
+	c.Assert(err, IsNil)
+	err = rel.Die()
+	c.Assert(err, IsNil)
 	err = s.State.RemoveRelation(rel)
 	c.Assert(err, IsNil)
 	assertChange(nil, []int{0})

=== modified file 'state/state.go'
--- state/state.go	2012-08-17 12:10:32 +0000
+++ state/state.go	2012-08-21 08:50:23 +0000
@@ -346,6 +346,7 @@
 		return
 	}
 	relationKey = strings.Split(path, "/")[2]
+	_, err = createRelation(s, relationKey)
 	return
 }
 
@@ -397,7 +398,7 @@
 	if err != nil {
 		return nil, err
 	}
-	r = &Relation{s, key, nil}
+	r = newRelation(s, key)
 	for _, tep := range tr.Endpoints {
 		sname, err := t.ServiceName(tep.Service)
 		if err != nil {
@@ -412,14 +413,20 @@
 
 // RemoveRelation removes the supplied relation.
 func (s *State) RemoveRelation(r *Relation) error {
-	err := retryTopologyChange(s.zk, func(t *topology) error {
-		if !t.HasRelation(r.key) {
-			return fmt.Errorf("not found")
-		}
+	life, err := r.Life()
+	if err != nil {
+		return fmt.Errorf("cannot remove relation %q: %s", r, err)
+	}
+	if life != Dead {
+		panic(fmt.Sprintf("relation %q is not dead", r))
+	}
+	err = retryTopologyChange(s.zk, func(t *topology) error {
 		return t.RemoveRelation(r.key)
 	})
 	if err != nil {
 		return fmt.Errorf("cannot remove relation %q: %s", r, err)
 	}
+	// TODO(mue) Check removal of ZK tree for relation.
+	// It  effects the reusage of keys.
 	return nil
 }