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.SetCharm does more sanity checks before
changing the unit's charm and also maintains the
reference counts of service settings.

Fixes LP bug #1063621.

https://code.launchpad.net/~dimitern/juju-core/005-upgrade-charm-state-ops/+merge/150750

(do not edit description out of merge proposal)

[dimitern, 2013-02-27 09:51:38]

Please take a look.

[dimitern, 2013-02-27 09:53:04]

Please take a look.

[fwereade, 2013-02-27 11:21:43]

LGTM assuming you're ok with the suggestions below. Apart from the docs, the
only issue is ServiceSettingsRefCount which I think can be made cleaner and
clearer as suggested.

https://codereview.appspot.com/7363060/diff/3001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/7363060/diff/3001/state/export_test.go#newcode31
state/export_test.go:31: func ServiceSettingsRefCount(st *State, serviceName
string, curl *charm.URL) (int, error) {
Drop the loop, and return 0 on mgo.ErrNotFound; otherwise just pass errors
through.

https://codereview.appspot.com/7363060/diff/3001/state/service.go
File state/service.go (left):

https://codereview.appspot.com/7363060/diff/3001/state/service.go#oldcode266
state/service.go:266: 
convertConfig returns the service's most recent config settings as interpreted
by the supplied charm, and a txn operation that asserts there have been no
changes to the source data.

https://codereview.appspot.com/7363060/diff/3001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode292
state/service.go:292: 
changeCharmOps returns the operations necessary to set a service's charm URL to
a new value.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode325
state/service.go:325: // Build and run the transaction.
s/and run//

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode596
state/service.go:596: 
settingsIncRefOp returns an operation that increments the refcount of the
service settings identified by serviceName and curl. If canCreate is false, a
missing document will be treated as an error; otherwise, it will be created with
a refcount of 1.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode619
state/service.go:619: 
settingsDecRefOps returns a list of operations that decrement the refcount of
the service settings identified by serviceName and curl. If the refcount is set
to zero, the appropriate setting and refcount documents will both be deleted.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode646
state/service.go:646: // settingsRefsDoc holds the number of units using the
settings
s/units/units and services/

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode647
state/service.go:647: // document identified by the same service settings id.
s/the same service settings id/this document's id.

[dimitern, 2013-02-27 11:50:42]

Please take a look.

https://codereview.appspot.com/7363060/diff/3001/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/7363060/diff/3001/state/export_test.go#newcode31
state/export_test.go:31: func ServiceSettingsRefCount(st *State, serviceName
string, curl *charm.URL) (int, error) {
On 2013/02/27 11:21:43, fwereade wrote:
> Drop the loop, and return 0 on mgo.ErrNotFound; otherwise just pass errors
> through.

Done.

https://codereview.appspot.com/7363060/diff/3001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode292
state/service.go:292: 
On 2013/02/27 11:21:43, fwereade wrote:
> changeCharmOps returns the operations necessary to set a service's charm URL
to
> a new value.

Done.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode325
state/service.go:325: // Build and run the transaction.
On 2013/02/27 11:21:43, fwereade wrote:
> s/and run//

Done.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode596
state/service.go:596: 
On 2013/02/27 11:21:43, fwereade wrote:
> settingsIncRefOp returns an operation that increments the refcount of the
> service settings identified by serviceName and curl. If canCreate is false, a
> missing document will be treated as an error; otherwise, it will be created
with
> a refcount of 1.

Done.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode619
state/service.go:619: 
On 2013/02/27 11:21:43, fwereade wrote:
> settingsDecRefOps returns a list of operations that decrement the refcount of
> the service settings identified by serviceName and curl. If the refcount is
set
> to zero, the appropriate setting and refcount documents will both be deleted.

Done.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode646
state/service.go:646: // settingsRefsDoc holds the number of units using the
settings
On 2013/02/27 11:21:43, fwereade wrote:
> s/units/units and services/

Done.

https://codereview.appspot.com/7363060/diff/3001/state/service.go#newcode647
state/service.go:647: // document identified by the same service settings id.
On 2013/02/27 11:21:43, fwereade wrote:
> s/the same service settings id/this document's id.

Done.

[rog, 2013-02-27 13:08:43]

LGTM with a few trivial remarks below.

one question though: why do the settings ref counts need to be in a separate
collection from the settings themselves?

https://codereview.appspot.com/7363060/diff/8001/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/7363060/diff/8001/state/conn_test.go#newcode77
state/conn_test.go:77: func (s *ConnSuite) AddConfigCharm(c *C, name, configYaml
string, revision int) *state.Charm {
doc comment, please.

https://codereview.appspot.com/7363060/diff/8001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7363060/diff/8001/state/service.go#newcode281
state/service.go:281: func (s *Service) convertConfig(ch *Charm)
(map[string]interface{}, txn.Op, error) {
a doc comment would be nice here.

https://codereview.appspot.com/7363060/diff/8001/state/service.go#newcode317
state/service.go:317: // Maintain service settings refcounts.
i had to think a bit about those comments. how about:

// Increment ref count on new charm.
call settingsIncRefOp

// Decrement ref count on current charm (possibly
// deleting it).
call settingsDecRefOps

https://codereview.appspot.com/7363060/diff/8001/state/service.go#newcode375
state/service.go:375: if err := s.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
rather than the defer at the head of the function, why not put the test here?

err := s.st.runner.Run(ops, "", nil)
if err == nil {
    s.doc.CharmURL = ch.URL()
    s.doc.ForceCharm = force
    return nil
}
if err != txn.ErrAborted {
    return err
}

if you had several paths returning a nil error, the defer would be worth it, but
i'm not sure it helps in this case.

https://codereview.appspot.com/7363060/diff/8001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7363060/diff/8001/state/state.go#newcode368
state/state.go:368: createSettingsOp(st, svc.settingsKey(),
map[string]interface{}{}),
i think you could probably pass nil here instead of an empty map.

[dimitern, 2013-02-27 13:52:13]

Please take a look.

https://codereview.appspot.com/7363060/diff/8001/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/7363060/diff/8001/state/conn_test.go#newcode77
state/conn_test.go:77: func (s *ConnSuite) AddConfigCharm(c *C, name, configYaml
string, revision int) *state.Charm {
On 2013/02/27 13:08:43, rog wrote:
> doc comment, please.

Done.

https://codereview.appspot.com/7363060/diff/8001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7363060/diff/8001/state/service.go#newcode281
state/service.go:281: func (s *Service) convertConfig(ch *Charm)
(map[string]interface{}, txn.Op, error) {
On 2013/02/27 13:08:43, rog wrote:
> a doc comment would be nice here.

Done.

https://codereview.appspot.com/7363060/diff/8001/state/service.go#newcode317
state/service.go:317: // Maintain service settings refcounts.
On 2013/02/27 13:08:43, rog wrote:
> i had to think a bit about those comments. how about:
> 
> // Increment ref count on new charm.
> call settingsIncRefOp
> 
> // Decrement ref count on current charm (possibly
> // deleting it).
> call settingsDecRefOps

Good idea! Done.

https://codereview.appspot.com/7363060/diff/8001/state/service.go#newcode375
state/service.go:375: if err := s.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
On 2013/02/27 13:08:43, rog wrote:
> rather than the defer at the head of the function, why not put the test here?
> 
> err := s.st.runner.Run(ops, "", nil)
> if err == nil {
>     s.doc.CharmURL = ch.URL()
>     s.doc.ForceCharm = force
>     return nil
> }
> if err != txn.ErrAborted {
>     return err
> }
> 
> if you had several paths returning a nil error, the defer would be worth it,
but
> i'm not sure it helps in this case.

Looking again, I think you're right, OK.

https://codereview.appspot.com/7363060/diff/8001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7363060/diff/8001/state/state.go#newcode368
state/state.go:368: createSettingsOp(st, svc.settingsKey(),
map[string]interface{}{}),
On 2013/02/27 13:08:43, rog wrote:
> i think you could probably pass nil here instead of an empty map.

Done.

[rog, 2013-02-27 13:55:11]

LGTM with one trivial

https://codereview.appspot.com/7363060/diff/10002/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7363060/diff/10002/state/service.go#newcode326
state/service.go:326: decOps, err := settingsDecRefOps(s.st, s.doc.Name,
s.doc.CharmURL) // current charm
you can delete the comment at the end of this line now.

[dimitern, 2013-02-27 13:58: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.SetCharm does more sanity checks before
changing the unit's charm and also maintains the
reference counts of service settings.

Fixes LP bug #1063621.

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

https://codereview.appspot.com/7363060/diff/10002/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7363060/diff/10002/state/service.go#newcode326
state/service.go:326: decOps, err := settingsDecRefOps(s.st, s.doc.Name,
s.doc.CharmURL) // current charm
On 2013/02/27 13:55:12, rog wrote:
> you can delete the comment at the end of this line now.

Forgot that - fixed!