state: upgrade charm for services/units

state: upgrade charm for services/units

This introduces the following changes:
- Service.SetCharm now performs validation on the
configuration when changing the charm to ensure
what's about to be set is compatible with the
current service's charm. Also introduces multiple
settings per service (one for each unique charm URL),
and uses reference counting to manage these and clean
them up.
- Unit.SetCharmURL does more sanity checks before
changing the unit's charm and also maintains the
reference counts of service settings.

Fixes LP bug #1063621.

This CL was reverted before in r943, now reintroduced
and made to work with current trunk.

https://code.launchpad.net/~dimitern/juju-core/015-charm-upgrade-settings-refs/+merge/153853

(do not edit description out of merge proposal)

[dimitern, 2013-03-18 15:55:48]

Please take a look.

[fwereade, 2013-03-18 16:10:18]

Mostly looking good, but WatchServiceConfig needs to look up the correct
settings document (on the upside, it might not need to have that error return in
there).

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

https://codereview.appspot.com/7497047/diff/1/state/conn_test.go#newcode77
state/conn_test.go:77: // AddConfigCharm clones a testing charm, replaces it's
config with
s/it's/its/

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

https://codereview.appspot.com/7497047/diff/1/state/service.go#newcode365
state/service.go:365: // Increment ref count on new charm.
I think "refcount" is a noun, but YMMV.

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

https://codereview.appspot.com/7497047/diff/1/state/service_test.go#newcode78
state/service_test.go:78: {
We should have a test that actually uses endvalues. All the current ones end up
with an empty dict, and we should verify that it's possible to keep values as
well.

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

https://codereview.appspot.com/7497047/diff/1/state/unit.go#newcode514
state/unit.go:514: // Maintain new charm settings refcount.
Update new settings doc refcount?

https://codereview.appspot.com/7497047/diff/1/state/unit.go#newcode531
state/unit.go:531: // Maintain current charm settings refcount.
Update old settings doc refcount?

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

https://codereview.appspot.com/7497047/diff/1/state/watcher.go#newcode1020
state/watcher.go:1020: }
This needs to watch the config doc with the *unit*'s charm URL.

[rog, 2013-03-19 11:54:30]

this all looks fine to me at surface level, but i confess i am finding it very
difficult to envisage the actual set of txn ops that are being produced (and
what we're guarding against/doing), and hence i don't think i'm really qualified
to give a definitive answer.

perhaps you could explain here what the difficulties are that the code is
solving and how it solves it; then i might have some better idea of what to look
for.

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

https://codereview.appspot.com/7497047/diff/1/state/service.go#newcode365
state/service.go:365: // Increment ref count on new charm.
On 2013/03/18 16:10:18, fwereade wrote:
> I think "refcount" is a noun, but YMMV.

i've used "ref count" in most places elsewhere, FWIW.

https://codereview.appspot.com/7497047/diff/1/state/service.go#newcode647
state/service.go:647: // settingsIncRefOp returns an operation that increments
the refcount
let's use either "refcount" or "ref count" consistently.

[dimitern, 2013-03-19 14:37:31]

Please take a look.

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

https://codereview.appspot.com/7497047/diff/1/state/conn_test.go#newcode77
state/conn_test.go:77: // AddConfigCharm clones a testing charm, replaces it's
config with
On 2013/03/18 16:10:18, fwereade wrote:
> s/it's/its/

Nice catch! Done.

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

https://codereview.appspot.com/7497047/diff/1/state/service.go#newcode365
state/service.go:365: // Increment ref count on new charm.
On 2013/03/19 11:54:30, rog wrote:
> On 2013/03/18 16:10:18, fwereade wrote:
> > I think "refcount" is a noun, but YMMV.
> 
> i've used "ref count" in most places elsewhere, FWIW.

Now using ref count everywhere.

https://codereview.appspot.com/7497047/diff/1/state/service.go#newcode647
state/service.go:647: // settingsIncRefOp returns an operation that increments
the refcount
On 2013/03/19 11:54:30, rog wrote:
> let's use either "refcount" or "ref count" consistently.

Changed to "ref count" everywhere.

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

https://codereview.appspot.com/7497047/diff/1/state/service_test.go#newcode78
state/service_test.go:78: {
On 2013/03/18 16:10:18, fwereade wrote:
> We should have a test that actually uses endvalues. All the current ones end
up
> with an empty dict, and we should verify that it's possible to keep values as
> well.

Done.

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

https://codereview.appspot.com/7497047/diff/1/state/unit.go#newcode514
state/unit.go:514: // Maintain new charm settings refcount.
On 2013/03/18 16:10:18, fwereade wrote:
> Update new settings doc refcount?

Done.

https://codereview.appspot.com/7497047/diff/1/state/unit.go#newcode531
state/unit.go:531: // Maintain current charm settings refcount.
On 2013/03/18 16:10:18, fwereade wrote:
> Update old settings doc refcount?

Done.

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

https://codereview.appspot.com/7497047/diff/1/state/watcher.go#newcode1020
state/watcher.go:1020: }
On 2013/03/18 16:10:18, fwereade wrote:
> This needs to watch the config doc with the *unit*'s charm URL.

Done.

[rog, 2013-03-19 17:51:16]

the comments are much better thank you.
a few more suggestions, then LGTM.

https://codereview.appspot.com/7497047/diff/10001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode328
state/service.go:328: // otherwise returns an error). Also returns an assert op
to
s/Also/It also/

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode329
state/service.go:329: // ensure the old settings haven't changed in the mean
time.
s/mean time/meantime/

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode369
state/service.go:369: // Increment the ref count with the new charm.
s/with/for/ ?

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode370
state/service.go:370: // This is the first time we use the new settings).
s/\)//

Useful comment, thanks.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode375
state/service.go:375: // Decrement the ref count with the current charm.
s/with/for/ ?
also, perhaps, s/current/old/ ?

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode399
state/service.go:399: // And finally, decrement the old settings.
also useful

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode414
state/service.go:414: // Charm URL already set, just update the force flag.
s/,/;/

https://codereview.appspot.com/7497047/diff/10001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode514
state/unit.go:514: // Update new settings doc ref count.
why are we incrementing the ref count?
(i think i know, but i'd like to see the comment reflect that)

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode531
state/unit.go:531: // Update old settings doc ref count.
why are we decrementing the ref count?
(as above)

[fwereade, 2013-03-20 09:11:30]

You're using the wrong charm url in unit.py; but if you're clear about the
problem and the solution I'm happy to call this LGTM with fixes.

https://codereview.appspot.com/7497047/diff/10001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode353
state/service.go:353: // The key consists of the service name and the charm URL.
I think this comment is redundant.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode370
state/service.go:370: // This is the first time we use the new settings).
On 2013/03/19 17:51:16, rog wrote:
> s/\)//
> 
> Useful comment, thanks.

I'm less sure. That's not always the case; consider a revert to a charm version
that had been kept alive by a single upgraded unit in an error state. That's why
we have a branch that does a replace op above.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode375
state/service.go:375: // Decrement the ref count with the current charm.
On 2013/03/19 17:51:17, rog wrote:
> s/with/for/ ?
> also, perhaps, s/current/old/ ?

+1

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode376
state/service.go:376: // If this was the last reference to it, delete it.
s/delete it/it will be removed/

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode399
state/service.go:399: // And finally, decrement the old settings.
On 2013/03/19 17:51:17, rog wrote:
> also useful

Actually, is the any reason not to just build the decOps in line here? It
doesn't seem to be used in the intervening branch, and ++locality.

https://codereview.appspot.com/7497047/diff/10001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode123
state/unit.go:123: svc, err := u.Service()
We shouldn't need to get the service.

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode127
state/unit.go:127: settings, err := readSettings(u.st, svc.settingsKey())
We want to build the settings key from the *unit's* charm URL. This and the
service name are both in the unitDoc; this information lets us use the free
serviceSettingsKey() func to determine a key that is (1) correct and (2) costs
us no unnecessary rountrips.

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode514
state/unit.go:514: // Update new settings doc ref count.
On 2013/03/19 17:51:17, rog wrote:
> why are we incrementing the ref count?
> (i think i know, but i'd like to see the comment reflect that)

This is because services and units can both hold references to settings
documents, and the last unit to upgrade is responsible for clearing up any that
it leaves unused. I'm not totally sure this is the best place to explain this,
though; maybe consider mentioning it explicitly in the settingsRefDoc comment?

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode531
state/unit.go:531: // Update old settings doc ref count.
On 2013/03/19 17:51:17, rog wrote:
> why are we decrementing the ref count?
> (as above)
(as above)

[dimitern, 2013-03-20 09:45:56]

Please take a look.

https://codereview.appspot.com/7497047/diff/10001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode328
state/service.go:328: // otherwise returns an error). Also returns an assert op
to
On 2013/03/19 17:51:16, rog wrote:
> s/Also/It also/

Done.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode329
state/service.go:329: // ensure the old settings haven't changed in the mean
time.
On 2013/03/19 17:51:16, rog wrote:
> s/mean time/meantime/

Done.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode353
state/service.go:353: // The key consists of the service name and the charm URL.
On 2013/03/20 09:11:31, fwereade wrote:
> I think this comment is redundant.
Removed.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode369
state/service.go:369: // Increment the ref count with the new charm.
On 2013/03/19 17:51:16, rog wrote:
> s/with/for/ ?

see below

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode370
state/service.go:370: // This is the first time we use the new settings).
On 2013/03/20 09:11:31, fwereade wrote:
> On 2013/03/19 17:51:16, rog wrote:
> > s/\)//
> > 
> > Useful comment, thanks.
> 
> I'm less sure. That's not always the case; consider a revert to a charm
version
> that had been kept alive by a single upgraded unit in an error state. That's
why
> we have a branch that does a replace op above.

see below

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode370
state/service.go:370: // This is the first time we use the new settings).
On 2013/03/19 17:51:16, rog wrote:
> s/\)//
> 
> Useful comment, thanks.

see below

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode375
state/service.go:375: // Decrement the ref count with the current charm.
On 2013/03/19 17:51:17, rog wrote:
> s/with/for/ ?
> also, perhaps, s/current/old/ ?

Done.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode375
state/service.go:375: // Decrement the ref count with the current charm.
On 2013/03/20 09:11:31, fwereade wrote:
> On 2013/03/19 17:51:17, rog wrote:
> > s/with/for/ ?
> > also, perhaps, s/current/old/ ?
> 
> +1

Changed as discussed.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode376
state/service.go:376: // If this was the last reference to it, delete it.
On 2013/03/20 09:11:31, fwereade wrote:
> s/delete it/it will be removed/

Done.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode399
state/service.go:399: // And finally, decrement the old settings.
On 2013/03/20 09:11:31, fwereade wrote:
> On 2013/03/19 17:51:17, rog wrote:
> > also useful
> 
> Actually, is the any reason not to just build the decOps in line here? It
> doesn't seem to be used in the intervening branch, and ++locality.

I prefer to leave it as is for now.

https://codereview.appspot.com/7497047/diff/10001/state/service.go#newcode414
state/service.go:414: // Charm URL already set, just update the force flag.
On 2013/03/19 17:51:17, rog wrote:
> s/,/;/

Done.

https://codereview.appspot.com/7497047/diff/10001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode123
state/unit.go:123: svc, err := u.Service()
On 2013/03/20 09:11:31, fwereade wrote:
> We shouldn't need to get the service.

Done.

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode127
state/unit.go:127: settings, err := readSettings(u.st, svc.settingsKey())
On 2013/03/20 09:11:31, fwereade wrote:
> We want to build the settings key from the *unit's* charm URL. This and the
> service name are both in the unitDoc; this information lets us use the free
> serviceSettingsKey() func to determine a key that is (1) correct and (2) costs
> us no unnecessary rountrips.

Done.

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode514
state/unit.go:514: // Update new settings doc ref count.
On 2013/03/19 17:51:17, rog wrote:
> why are we incrementing the ref count?
> (i think i know, but i'd like to see the comment reflect that)

Updated the settingsRefDoc comment.

https://codereview.appspot.com/7497047/diff/10001/state/unit.go#newcode531
state/unit.go:531: // Update old settings doc ref count.
On 2013/03/19 17:51:17, rog wrote:
> why are we decrementing the ref count?
> (as above)

see above.

[rog, 2013-03-20 10:42:42]

LGTM with one last question

https://codereview.appspot.com/7497047/diff/26001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode714
state/service.go:714: // settings document identified by this document's id.
Every time a
i find this a useful comment. one thing i hadn't picked up on before: what does
"this document" refer to in this context?

[dimitern, 2013-03-20 10:44:29]

https://codereview.appspot.com/7497047/diff/26001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode714
state/service.go:714: // settings document identified by this document's id.
Every time a
On 2013/03/20 10:42:43, rog wrote:
> i find this a useful comment. one thing i hadn't picked up on before: what
does
> "this document" refer to in this context?
settingsRefsDoc's id and serviceDoc's id are the same, so "this document" refers
to settingsRefsDoc.

[fwereade, 2013-03-20 10:50:37]

Few more suggestions re documentation, but I'll leave it to your judgment.

https://codereview.appspot.com/7497047/diff/26001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode368
state/service.go:368: // The unit adds a reference to the new settings doc with
this.
// Add or create a reference to the new settings doc.

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode373
state/service.go:373: // The service drops its reference to its old settings doc
with this.
// Drop the reference to the old settings doc.

I still think it's clearer to move it down past the if, because the duplicated
comments make it look like we're doing the same thing twice; if we had better
locality it'd be clearer.

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode714
state/service.go:714: // settings document identified by this document's id.
Every time a
On 2013/03/20 10:44:29, dimitern wrote:
> On 2013/03/20 10:42:43, rog wrote:
> > i find this a useful comment. one thing i hadn't picked up on before: what
> does
> > "this document" refer to in this context?
> settingsRefsDoc's id and serviceDoc's id are the same, so "this document"
refers
> to settingsRefsDoc.

s/serviceDoc/settingsDoc/ and the above is correct

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode714
state/service.go:714: // settings document identified by this document's id.
Every time a
On 2013/03/20 10:42:43, rog wrote:
> i find this a useful comment. one thing i hadn't picked up on before: what
does
> "this document" refer to in this context?

s/this/the/ I guess?

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode719
state/service.go:719: // is responsible for deleting the old settings doc.
Honestly, I'm not sure this adds anything (and it misses a detail on service
charm change). I think I'd prefer to stick to the original.

https://codereview.appspot.com/7497047/diff/26001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7497047/diff/26001/state/unit.go#newcode510
state/unit.go:510: // Update new settings doc ref count.
// Add a reference to the service settings for the new charm.

https://codereview.appspot.com/7497047/diff/26001/state/unit.go#newcode527
state/unit.go:527: // Update old settings doc ref count.
// Drop the reference to the old charm.

[rog, 2013-03-20 11:22:13]

https://codereview.appspot.com/7497047/diff/26001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode719
state/service.go:719: // is responsible for deleting the old settings doc.
On 2013/03/20 10:50:37, fwereade wrote:
> Honestly, I'm not sure this adds anything (and it misses a detail on service
> charm change). I think I'd prefer to stick to the original.

it might not add anything for you, but it's useful for me (and i'd presume for
other people coming to this code from cold).

if there's a missing detail, let's add it.

[dimitern, 2013-03-20 11:48:49]

*** Submitted:

state: upgrade charm for services/units

This introduces the following changes:
- Service.SetCharm now performs validation on the
configuration when changing the charm to ensure
what's about to be set is compatible with the
current service's charm. Also introduces multiple
settings per service (one for each unique charm URL),
and uses reference counting to manage these and clean
them up.
- Unit.SetCharmURL does more sanity checks before
changing the unit's charm and also maintains the
reference counts of service settings.

Fixes LP bug #1063621.

This CL was reverted before in r943, now reintroduced
and made to work with current trunk.

R=fwereade, rog
CC=
https://codereview.appspot.com/7497047

https://codereview.appspot.com/7497047/diff/26001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode368
state/service.go:368: // The unit adds a reference to the new settings doc with
this.
On 2013/03/20 10:50:37, fwereade wrote:
> // Add or create a reference to the new settings doc.

Done.

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode373
state/service.go:373: // The service drops its reference to its old settings doc
with this.
On 2013/03/20 10:50:37, fwereade wrote:
> // Drop the reference to the old settings doc.
> 
> I still think it's clearer to move it down past the if, because the duplicated
> comments make it look like we're doing the same thing twice; if we had better
> locality it'd be clearer.

Done.

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode714
state/service.go:714: // settings document identified by this document's id.
Every time a
On 2013/03/20 10:50:37, fwereade wrote:
> On 2013/03/20 10:42:43, rog wrote:
> > i find this a useful comment. one thing i hadn't picked up on before: what
> does
> > "this document" refer to in this context?
> 
> s/this/the/ I guess?

Done.

https://codereview.appspot.com/7497047/diff/26001/state/service.go#newcode719
state/service.go:719: // is responsible for deleting the old settings doc.
On 2013/03/20 11:22:13, rog wrote:
> On 2013/03/20 10:50:37, fwereade wrote:
> > Honestly, I'm not sure this adds anything (and it misses a detail on service
> > charm change). I think I'd prefer to stick to the original.
> 
> it might not add anything for you, but it's useful for me (and i'd presume for
> other people coming to this code from cold).
> 
> if there's a missing detail, let's add it.
> 

Finally, changed as agreed online.

https://codereview.appspot.com/7497047/diff/26001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7497047/diff/26001/state/unit.go#newcode510
state/unit.go:510: // Update new settings doc ref count.
On 2013/03/20 10:50:37, fwereade wrote:
> // Add a reference to the service settings for the new charm.

Done.

https://codereview.appspot.com/7497047/diff/26001/state/unit.go#newcode527
state/unit.go:527: // Update old settings doc ref count.
On 2013/03/20 10:50:37, fwereade wrote:
> // Drop the reference to the old charm.

Done.