Add a doc for tracking minimum units changes.

Add a doc for tracking minimum units changes.

This branch includes a new minimumUnits doc.
It allows for keeping track of changes on the 
services MinimumUnits field, also introduced
in this branch. 

Also added functions for setting minimum units 
on the service, and for adding units if the 
value is greater than the number of alive units.
The latter (EnsureMinimumUnits) is intended to 
be used, in the future, by a worker reacting 
to changes on the minimumUnits document.

https://code.launchpad.net/~frankban/juju-core/set-minimum-units/+merge/171078

(do not edit description out of merge proposal)

[frankban, 2013-06-24 13:06:39]

Please take a look.

[mue, 2013-06-25 12:59:41]

LGTM to the code, but I'm not yet sure why an own document and collection is
used. Will see how it gets more clear when the watcher is developed and
integrated.

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

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode13
state/minimumunits.go:13: // minimumUnitsDoc allows for keeping track of
relevant changes on the
Hmm, "... allows keeping track ..." or "... is for keeping track ..."? Or am I
wrong here?

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode24
state/minimumunits.go:24: // Since the referred entity type is always the
Service, it is safe here
Comments typically starts with the identifier. // ServiceName ...

[frankban, 2013-06-25 14:59:48]

Please take a look.

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

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode13
state/minimumunits.go:13: // minimumUnitsDoc allows for keeping track of
relevant changes on the
On 2013/06/25 12:59:42, mue wrote:
> Hmm, "... allows keeping track ..." or "... is for keeping track ..."? Or am I
> wrong here?

I believe the sentence was not incorrect in its original form. Anyway, I
simplified it.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode24
state/minimumunits.go:24: // Since the referred entity type is always the
Service, it is safe here
On 2013/06/25 12:59:42, mue wrote:
> Comments typically starts with the identifier. // ServiceName ...

Done.

[frankban, 2013-06-25 16:19:47]

Please take a look.

[rog, 2013-06-25 16:25:33]

great stuff. looks good with quite a few trivial thoughts and two more
substantial questions.

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

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode15
state/minimumunits.go:15: // A new document is created when MinimumUnits is set
to a non zero value.
might it be easier to create the document when the service is created
and delete it when it's removed? that would seem to me to remove quite
a bit of complexity for minimal cost.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode18
state/minimumunits.go:18: // MinimumUnits for a service is increased or a unit
is destroyed.
s/increased/changed/ ?

also, i'd be tempted to s/Minimum/Min/ throughout.
it's an extremely commonly used abbreviation.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode19
state/minimumunits.go:19: // TODO(frankban): the MinimumUnitsWatcher reacts to
changes sending events,
s/sending/by sending/ ?

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode31
state/minimumunits.go:31: // SetMinimumUnits changes the amount of minimum units
required by the service.
s/amount/number/

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode35
state/minimumunits.go:35: return errors.New("minimum units must be a positive
number")
"cannot set a negative minimum number of units on a service" ?

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode48
state/minimumunits.go:48: // attempt. If the referred-to service advanced his
life cycle to a not
s/his/its

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode105
state/minimumunits.go:105: func minimumUnitsUpdateOp(st *State, serviceName
string) txn.Op {
minimumUnitsUpdateRevnoOp

(otherwise it sounds like you're updating the min units number itself)

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode126
state/minimumunits.go:126: // AliveUnitsCount returns the amount of alive units
of the service.
i'm not sure that we want this operation - it's inherently racy.

i think it might be better to have an AddUnitIfNotEnoughUnits
call instead (choose a name to taste) that did an assert on
the minimum unit count. that way if we have one place
adding a unit and a worker trying to increment the number
of units to fit the minimum, the worker won't create too
many units accidentally.

that would mean we'd need to maintain the current unit count though.
hmm.

also, i'm not sure it's worth exporting.

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

https://codereview.appspot.com/10497043/diff/1/state/service.go#newcode41
state/service.go:41: MinimumUnits  int
is this actually used?

https://codereview.appspot.com/10497043/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/10497043/diff/6001/state/open.go#newcode1
state/open.go:1: relations:      db.C("relations"),
huh? what's going on here?

[frankban, 2013-06-26 08:56:00]

Please take a look.

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

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode15
state/minimumunits.go:15: // A new document is created when MinimumUnits is set
to a non zero value.
On 2013/06/25 16:25:33, rog wrote:
> might it be easier to create the document when the service is created
> and delete it when it's removed? that would seem to me to remove quite
> a bit of complexity for minimal cost.

On the other hand, it seems nice to me that we only watch few documents,
essentially one for each service with MinimumUnits > 0. Your suggestion is
reasonable, but then we will need to watch a document for each service in the
env (I guess this is what your "minimal cost" means). The current solution was
decided after a pre-imp call with William, and the complexity introduced to
achieve this seems not too bad to me.
Perhaps we can defer further work to a later branch, having all the machinery
more defined?

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode18
state/minimumunits.go:18: // MinimumUnits for a service is increased or a unit
is destroyed.
On 2013/06/25 16:25:33, rog wrote:
> s/increased/changed/ ?

The Revno is actually increased only when you set more MinimumUnits than before.

> also, i'd be tempted to s/Minimum/Min/ throughout.
> it's an extremely commonly used abbreviation.

So, SetMinUnits, minUnitsDoc, EnsureMinUnits, minUnitsIncreaseOp...?
I am not a fan of the current name for this feature, no problem in changing it
everywhere if you suggest doing so.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode19
state/minimumunits.go:19: // TODO(frankban): the MinimumUnitsWatcher reacts to
changes sending events,
On 2013/06/25 16:25:33, rog wrote:
> s/sending/by sending/ ?

Done.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode31
state/minimumunits.go:31: // SetMinimumUnits changes the amount of minimum units
required by the service.
On 2013/06/25 16:25:33, rog wrote:
> s/amount/number/

Done.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode35
state/minimumunits.go:35: return errors.New("minimum units must be a positive
number")
On 2013/06/25 16:25:33, rog wrote:
> "cannot set a negative minimum number of units on a service" ?

Done.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode48
state/minimumunits.go:48: // attempt. If the referred-to service advanced his
life cycle to a not
On 2013/06/25 16:25:33, rog wrote:
> s/his/its

Done.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode105
state/minimumunits.go:105: func minimumUnitsUpdateOp(st *State, serviceName
string) txn.Op {
On 2013/06/25 16:25:33, rog wrote:
> minimumUnitsUpdateRevnoOp
> 
> (otherwise it sounds like you're updating the min units number itself)

It chose this name so that it was symmetric with the the name of the other
operations. I agree it can generate confusion, changed.

https://codereview.appspot.com/10497043/diff/1/state/minimumunits.go#newcode126
state/minimumunits.go:126: // AliveUnitsCount returns the amount of alive units
of the service.
On 2013/06/25 16:25:33, rog wrote:
> i'm not sure that we want this operation - it's inherently racy.
> i think it might be better to have an AddUnitIfNotEnoughUnits
> call instead (choose a name to taste) that did an assert on
> the minimum unit count. that way if we have one place
> adding a unit and a worker trying to increment the number
> of units to fit the minimum, the worker won't create too
> many units accidentally.
> that would mean we'd need to maintain the current unit count though.
> hmm.

Yes there is a risk to add too many units. AFAICT, even checking the current
number of alive units in the loop, before adding a unit, does not prevent at
least one more unit to be added to the service. However, I am not sure this is
actually a problem, or a problem in the scope of this branch: we are setting
minimum units (we are not setting units), we are telling the system to react
when we destroy units or when we increase that value, so that eventually that
value, the minimum number of alive units, is respected. If we have too many
units at the end of the process, our minimum constraint is still valid, and we
can just start removing undesired ones.

> also, i'm not sure it's worth exporting.

I don't have a strong opinion about this. On the other hand, I like how this
function is tested in the state_test package.

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

https://codereview.appspot.com/10497043/diff/1/state/service.go#newcode41
state/service.go:41: MinimumUnits  int
On 2013/06/25 16:25:33, rog wrote:
> is this actually used?

Yes, it is set by the SetMinimumUnits function receiving the Service.

[fwereade, 2013-06-26 10:59:26]

I'm fine with the new collection and the operations on it, modulo a few tweaks,
but suspicious of AliveUnitCount (and by inference EnsureMinimumUnits).

Can we strip those two methods out of this branch and stack them in a new for
further discussion in detail? I'm not sure there's currently any justification
for exposing AUC, and that we'd be best off with a slightly harsher and more
rigorous change to EUM -- perhaps add another assert to each unit addition that
aborts on any change to the service document, thereby guaranteeing that any
parallel additions will invalidate the txn and cause us to try to go around
again (and possibly just cancel the op).

txnrevno checks are kinda inelegant, but I think it's the only tool we have
available to use here if we're to upgrade live from 1.10 (which we must be able
to do).

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:28: TxnRevno    int64 `bson:"txn-revno"`
I don't think we need this field.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:36: }
I observe without judgment at this point that we don't check what the service's
current minimum units are. Would it be reasonable to infer state directly from
the service's current MinimumUnits value, and so to abort without db hit or
error if it appears to match already? Changes under our feet shouldn't be too
hard to accommodate -- I think that maybe we never need to look directly at the
minimumunits collection.

I *think* that I'm describing the best approach, but I'm open to argument.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:56: return errors.New("service is no longer alive")
I'd prefer to skip the inference-from-index in favour of a really quick explicit
check of isAlive (utility func in life.go iirc) when we hit ErrAborted.

The big problem with index inference is that it assumes too much about the
asserts of any ops added by the functions it calls -- a tweak there for another
purpose can invalidate the logic here. Explicit asserts on abort for the
conditions you know will sink you tend to work a bit better; in the real fail
cases you get a real tailored error, and in the weird cases you usually see
ErrExcessiveContention (which usually in fact signifies that someone screwed up
some asserts somewhere).

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:69: s.doc.MinimumUnits = minimumUnits
FWIW the post-abort check ends up reasonably clear if you put the
local-memory-change in a defer, gated on err == nil, at the top of the method;
and you can then just do:

if err := s.st.runTransaction(ops); err != errAborted {
    return err
}
if alive, err := isAlive(s.st, um, I forget the args); err != nil {
    return err
} else if !alive {
    return fmt.Errorf("service is dying")
}

as described above, putting the abort-handling explicitly just after the abort.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:80: func minimumUnitsInsertOp(st *State, serviceName
string) txn.Op {
If we were inferring state from a *Service, we could combine all these global
funcs, and the logic that calls them, into a single setMinimumUnitsOps call; the
SetMinimumUnits method could then be exclusively concerned with
abort/retry/cancel handling.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go
File state/minimumunits_test.go (right):

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:32: var setMinimumUnitsTests = []struct {
FWIW, I think these tests work rather better in-method:

for i, test := range []struct{
    // fields
}{{
    // test 1
}, {
    // test 2
}} {
    // test runner code
}

...but I'm not actually mandating it, because opinions vary. If you like it
(less namespace pollution, no chance of unrelated code messing with or depending
on test-specific data) feel free to use it.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:39: {
}{{

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:44: {
}, {

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:83: }
}}

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:99: c.Assert(service.MinimumUnits(), Equals, input)
Maybe refresh the service as well, to check both in-memory and db values?

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:112: `.* cannot set a negative minimum number of
units on a service`)
Best to avoid these .* error tests -- they just mask ugly messages which we then
dump on users.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:181: func (s *MinimumUnitsSuite)
TestMinimumUnitsSetDestroyEntities(c *C) {
Blank lines before comments would really help me read this easily.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:192: // the revno has been increased.
There's a case missing here -- this checks remove, because the destroy
short-circuits to remove, but not just setting Dying. Add another with a
preventUnitDestroyRemove please.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:200: c.Assert(s.service.Life(), Not(Equals),
state.Alive)
I can't decide whether this is redundant (because we know that service
destruction sets both db and in-memory life state correctly anyway) or
inadequate (because we're destroying a service with interesting characteristics
that impact its destruction, but we only check in-memory state and not db
state).

I could be convinced either way :).

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:260: {
Compact these please (and consider taking them out of the global namespace
unless they're reused)

https://codereview.appspot.com/10497043/diff/30001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10497043/diff/30001/state/unit.go#newcode234
state/unit.go:234: ops = append(ops, minimumUnitsIncreaseOp(unit.st,
unit.ServiceName()))
This should go in destroyOps, not here.

[frankban, 2013-06-26 15:44:53]

Please take a look.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:28: TxnRevno    int64 `bson:"txn-revno"`
On 2013/06/26 10:59:26, fwereade wrote:
> I don't think we need this field.

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:36: }
On 2013/06/26 10:59:26, fwereade wrote:
> I observe without judgment at this point that we don't check what the
service's
> current minimum units are. Would it be reasonable to infer state directly from
> the service's current MinimumUnits value, and so to abort without db hit or
> error if it appears to match already? Changes under our feet shouldn't be too
> hard to accommodate -- I think that maybe we never need to look directly at
the
> minimumunits collection.
> 
> I *think* that I'm describing the best approach, but I'm open to argument.

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:56: return errors.New("service is no longer alive")
On 2013/06/26 10:59:26, fwereade wrote:
> I'd prefer to skip the inference-from-index in favour of a really quick
explicit
> check of isAlive (utility func in life.go iirc) when we hit ErrAborted.
> 
> The big problem with index inference is that it assumes too much about the
> asserts of any ops added by the functions it calls -- a tweak there for
another
> purpose can invalidate the logic here. Explicit asserts on abort for the
> conditions you know will sink you tend to work a bit better; in the real fail
> cases you get a real tailored error, and in the weird cases you usually see
> ErrExcessiveContention (which usually in fact signifies that someone screwed
up
> some asserts somewhere).

Good point, fixed.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:69: s.doc.MinimumUnits = minimumUnits
On 2013/06/26 10:59:26, fwereade wrote:
> FWIW the post-abort check ends up reasonably clear if you put the
> local-memory-change in a defer, gated on err == nil, at the top of the method;
> and you can then just do:
> 
> if err := s.st.runTransaction(ops); err != errAborted {
>     return err
> }
> if alive, err := isAlive(s.st, um, I forget the args); err != nil {
>     return err
> } else if !alive {
>     return fmt.Errorf("service is dying")
> }
> 
> as described above, putting the abort-handling explicitly just after the
abort.

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits.go#newc...
state/minimumunits.go:80: func minimumUnitsInsertOp(st *State, serviceName
string) txn.Op {
On 2013/06/26 10:59:26, fwereade wrote:
> If we were inferring state from a *Service, we could combine all these global
> funcs, and the logic that calls them, into a single setMinimumUnitsOps call;
the
> SetMinimumUnits method could then be exclusively concerned with
> abort/retry/cancel handling.

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go
File state/minimumunits_test.go (right):

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:32: var setMinimumUnitsTests = []struct {
On 2013/06/26 10:59:26, fwereade wrote:
> FWIW, I think these tests work rather better in-method:
> 
> for i, test := range []struct{
>     // fields
> }{{
>     // test 1
> }, {
>     // test 2
> }} {
>     // test runner code
> }
> 
> ...but I'm not actually mandating it, because opinions vary. If you like it
> (less namespace pollution, no chance of unrelated code messing with or
depending
> on test-specific data) feel free to use it.

Why not? Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:39: {
On 2013/06/26 10:59:26, fwereade wrote:
> }{{

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:44: {
On 2013/06/26 10:59:26, fwereade wrote:
> }, {

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:83: }
On 2013/06/26 10:59:26, fwereade wrote:
> }}

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:99: c.Assert(service.MinimumUnits(), Equals, input)
On 2013/06/26 10:59:26, fwereade wrote:
> Maybe refresh the service as well, to check both in-memory and db values?

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:112: `.* cannot set a negative minimum number of
units on a service`)
On 2013/06/26 10:59:26, fwereade wrote:
> Best to avoid these .* error tests -- they just mask ugly messages which we
then
> dump on users.

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:192: // the revno has been increased.
On 2013/06/26 10:59:26, fwereade wrote:
> There's a case missing here -- this checks remove, because the destroy
> short-circuits to remove, but not just setting Dying. Add another with a
> preventUnitDestroyRemove please.

Done.

https://codereview.appspot.com/10497043/diff/30001/state/minimumunits_test.go...
state/minimumunits_test.go:200: c.Assert(s.service.Life(), Not(Equals),
state.Alive)
On 2013/06/26 10:59:26, fwereade wrote:
> I can't decide whether this is redundant (because we know that service
> destruction sets both db and in-memory life state correctly anyway) or
> inadequate (because we're destroying a service with interesting
characteristics
> that impact its destruction, but we only check in-memory state and not db
> state).
> 
> I could be convinced either way :).

You are right, those life checks don't add value, removed.

https://codereview.appspot.com/10497043/diff/30001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10497043/diff/30001/state/unit.go#newcode234
state/unit.go:234: ops = append(ops, minimumUnitsIncreaseOp(unit.st,
unit.ServiceName()))
On 2013/06/26 10:59:26, fwereade wrote:
> This should go in destroyOps, not here.

Done.

[fwereade, 2013-06-26 23:11:10]

LGTM with suggestions below

https://codereview.appspot.com/10497043/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/10497043/diff/6001/state/open.go#newcode222
state/open.go:222: 
Let's be consistent about min/minimum across the board. Also, pretty cool typo:
minumumunits :).

https://codereview.appspot.com/10497043/diff/37001/state/minimumunits.go
File state/minimumunits.go (right):

https://codereview.appspot.com/10497043/diff/37001/state/minimumunits.go#newc...
state/minimumunits.go:57: } else if err != txn.ErrAborted {
This branch also handles the case above, because nil != txn.ErrAborted :)

https://codereview.appspot.com/10497043/diff/37001/state/minimumunits_test.go
File state/minimumunits_test.go (right):

https://codereview.appspot.com/10497043/diff/37001/state/minimumunits_test.go...
state/minimumunits_test.go:33: var err error
I think I'd drop this and use := below, but I might be missing something?

https://codereview.appspot.com/10497043/diff/37001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/10497043/diff/37001/state/unit.go#newcode278
state/unit.go:278: minUnitsOp := minUnitsIncreaseOp(u.st, u.ServiceName())
Hmm. In this context, I'm wondering if it might make sense to call it a
minUnitsTriggerOp? YMMV.

[frankban, 2013-06-27 08:18:06]

On 2013/06/26 23:11:10, fwereade wrote:
> LGTM with suggestions below

> https://codereview.appspot.com/10497043/diff/6001/state/open.go#newcode222
> state/open.go:222: 
> Let's be consistent about min/minimum across the board. Also, pretty cool
typo:
> minumumunits :).

Hum... This was already fixed in patch set five. Rietveld acts weirdly
sometimes.

>
https://codereview.appspot.com/10497043/diff/37001/state/minimumunits.go#newc...
> state/minimumunits.go:57: } else if err != txn.ErrAborted {
> This branch also handles the case above, because nil != txn.ErrAborted :)

Heh, great, fixed.

>
https://codereview.appspot.com/10497043/diff/37001/state/minimumunits_test.go...
> state/minimumunits_test.go:33: var err error
> I think I'd drop this and use := below, but I might be missing something?

Done.

> https://codereview.appspot.com/10497043/diff/37001/state/unit.go#newcode278
> state/unit.go:278: minUnitsOp := minUnitsIncreaseOp(u.st, u.ServiceName())
> Hmm. In this context, I'm wondering if it might make sense to call it a
> minUnitsTriggerOp? YMMV.

Nice, it's less confusing in that context, changed.

[frankban, 2013-06-27 08:18:49]

Please take a look.