uniter: skip unknown rel. endpoints

uniter: skip unknown rel. endpoints

Ignore relations with unknown (not implemented by
the charm) when handling upgrades.
The uniter is now requesting all relation events
on upgrade, so they won't be missed.

This also fixes LP bug #1101140 (uniter does not
skip unimplemented relations).

https://code.launchpad.net/~dimitern/juju-core/002-bug-1101140-ignore-unknown-relations/+merge/149921

Requires: https://code.launchpad.net/~dimitern/juju-core/001-upgrade-charm-filter-restart-watchers/+merge/149910

(do not edit description out of merge proposal)

[fwereade, 2013-02-22 10:32:28]

Tests! But look into the prereq first anyway :).

https://codereview.appspot.com/7385049/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7385049/diff/1/worker/uniter/uniter.go#newcode441
worker/uniter/uniter.go:441: sch, _, err := svc.Charm()
This will never catch problems. We know for *sure* that this charm implements
the endpoint... but we need to check the charm that is actually deployed right
here ;). I think you want something a bit more like:

ch, err := corecharm.ReadDir(u.charm.Path())

...but I would also be potentially interested in smarter approaches (like
storing the *charm.Dir on the uniter, if you can think of a way to do so cleanly
and clearly... I don't *expect* you to do this, just a suggestion).

[dimitern, 2013-02-25 10:04:19]

Please take a look.

[fwereade, 2013-02-25 10:31:57]

Thanks -- LGTM assuming the below.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:145: // handle the relations for the new unit
Now the upgrade is complete, we'll need to check all relations again: some might
previously have been skipped (if they involved endpoints only implemented in the
new charm).

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:383: // possibly even panic?
Nope, this is just a straight-up error, I think. We don't have any way of
resolving it, or reporting it, it's true; but panicking won't improve matters
so... ;).

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:389: log.Printf("worker/uniter: ignoring not implemented
relation endpoint %q", ep)
"skipping relation with unknown endpoint"?

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:323: customize: func(c *C, path string, ctx
*context) {
(c, ctx, path) feels neater to me, but YMMV

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:620: // doing upgradeCharm before addRelation (?!)
This test does an add-relation as quickly as possible after an upgrade-charm, in
the hope that the scheduler will deliver the events in the wrong order. The
observed behaviour should be the same in either case.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:636: waitHooks{"db2-relation-joined mysql/0
db2:0"},
May as well stick those steps together.

[dimitern, 2013-02-25 11:00:05]

Please take a look.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:145: // handle the relations for the new unit
On 2013/02/25 10:31:57, fwereade wrote:
> Now the upgrade is complete, we'll need to check all relations again: some
might
> previously have been skipped (if they involved endpoints only implemented in
the
> new charm).

Done.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:383: // possibly even panic?
On 2013/02/25 10:31:57, fwereade wrote:
> Nope, this is just a straight-up error, I think. We don't have any way of
> resolving it, or reporting it, it's true; but panicking won't improve matters
> so... ;).

Done.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:383: // possibly even panic?
On 2013/02/25 10:31:57, fwereade wrote:
> Nope, this is just a straight-up error, I think. We don't have any way of
> resolving it, or reporting it, it's true; but panicking won't improve matters
> so... ;).

Done.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:389: log.Printf("worker/uniter: ignoring not implemented
relation endpoint %q", ep)
On 2013/02/25 10:31:57, fwereade wrote:
> "skipping relation with unknown endpoint"?

Done.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:323: customize: func(c *C, path string, ctx
*context) {
On 2013/02/25 10:31:57, fwereade wrote:
> (c, ctx, path) feels neater to me, but YMMV

Done.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:620: // doing upgradeCharm before addRelation (?!)
On 2013/02/25 10:31:57, fwereade wrote:
> This test does an add-relation as quickly as possible after an upgrade-charm,
in
> the hope that the scheduler will deliver the events in the wrong order. The
> observed behaviour should be the same in either case.

Done.

https://codereview.appspot.com/7385049/diff/7001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:636: waitHooks{"db2-relation-joined mysql/0
db2:0"},
On 2013/02/25 10:31:57, fwereade wrote:
> May as well stick those steps together.

Done.

[TheMue, 2013-02-25 11:20:47]

Mostly LGTM, only minor comments.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer.go#...
worker/uniter/relationer.go:54: // No need to check the r.dir exists yet
IMHO not needed comment, only relates to removed code part.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer.go#...
worker/uniter/relationer.go:124: // We are about to use the dir, ensure it's
there
Punctuation. ;)

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
File worker/uniter/relationer_test.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:102: // Verify PrepareHook created the dir
Punctuation.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:355: // We're no longer doing this in Join, so
we must do it here
Needless comment, it's related to a change.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:409: // Join the relationer; the dir won't be
created until necessary
Ditto.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:318: // TODO: Get these from state, not from disk
// TODO(who): ..., and punctuation.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:619: // after an upgrade-charm, in the hope that
the scheduler will
"in the hope"? Sounds a bit weak to me. Any way to force the delivery in a wrong
order?

[dimitern, 2013-02-25 11:59:08]

*** Submitted:

uniter: skip unknown rel. endpoints

Ignore relations with unknown (not implemented by
the charm) when handling upgrades.
The uniter is now requesting all relation events
on upgrade, so they won't be missed.

This also fixes LP bug #1101140 (uniter does not
skip unimplemented relations).

R=fwereade, TheMue
CC=
https://codereview.appspot.com/7385049

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer.go#...
worker/uniter/relationer.go:54: // No need to check the r.dir exists yet
On 2013/02/25 11:20:47, TheMue wrote:
> IMHO not needed comment, only relates to removed code part.

Done.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer.go#...
worker/uniter/relationer.go:124: // We are about to use the dir, ensure it's
there
On 2013/02/25 11:20:47, TheMue wrote:
> Punctuation. ;)

Done.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
File worker/uniter/relationer_test.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:102: // Verify PrepareHook created the dir
On 2013/02/25 11:20:47, TheMue wrote:
> Punctuation.

Done.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:355: // We're no longer doing this in Join, so
we must do it here
On 2013/02/25 11:20:47, TheMue wrote:
> Needless comment, it's related to a change.

Not exactly, the change breaks this unless dir.Ensure() is run. Updated the
comment to reflect this.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/relationer_tes...
worker/uniter/relationer_test.go:409: // Join the relationer; the dir won't be
created until necessary
On 2013/02/25 11:20:47, TheMue wrote:
> Ditto.

Actually, this is now part of the Join's behavior and it's worth the comment,
even more so, because it changes the test logic.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:318: // TODO: Get these from state, not from disk
On 2013/02/25 11:20:47, TheMue wrote:
> // TODO(who): ..., and punctuation.

Done.

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7385049/diff/2002/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:619: // after an upgrade-charm, in the hope that
the scheduler will
On 2013/02/25 11:20:47, TheMue wrote:
> "in the hope"? Sounds a bit weak to me. Any way to force the delivery in a
wrong
> order?

No ways I can think of to tweak the scheduling at run-time.