state: add machine status; refractor unit status

state: add machine status; refractor unit status

Added Machine.Status() and SetStatus(), as with a unit.
Since we're using the same code, a refactoring was in
order: common code for managing statuses moved to a
separate state collection (see status.go); status consts
by the API and state added to state/api/params, so they
can be accessed from both.

Some changes to allWatcher/megawatcher code: because
now Status and StatusInfo are no longer part of the
unit document, you cannot watch for these changes, but
arguably only the juju status command actually needs
to read them, so the API should be handle this eventually.

Drive-by fix: constranits and annotations (and now status)
of the last unit of a service were not removed properly.
Now they are.

https://code.launchpad.net/~dimitern/juju-core/021-state-add-machine-status/+merge/156534

(do not edit description out of merge proposal)

[dimitern, 2013-04-02 12:09:56]

Please take a look.

[TheMue, 2013-04-02 15:12:58]

LGTM, only getting a bit scared by the growing number of docs and collections.

[fwereade, 2013-04-02 15:21:55]

On 2013/04/02 15:12:58, TheMue wrote:
> LGTM, only getting a bit scared by the growing number of docs and collections.

Would you describe your concerns here in more detail please?

[rog, 2013-04-02 15:29:07]

LGTM with the below comments addressed.

FWIW i echo frank's concerns. as discussed online i really don't see a
particular benefit from putting a unit's status in a separate collection - it
has more failure modes (you now need a conditional transaction to set a status)
and requires more round trips in common cases (status for example).

nonetheless given the above reservations, this should work ok - the logic seems
sound enough.

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go#newcode226
cmd/juju/status.go:226: if err != nil {
this doesn't seem right to me. i don't think we should be ignoring the error
here.

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

https://codereview.appspot.com/8256043/diff/1/state/machine.go#newcode463
state/machine.go:463: panic("machine status must not be set to pending")
i prefer panic messages to say what went wrong rather than an instruction as to
what to do right.

for example:
panic("cannot set machine status back to pending")

https://codereview.appspot.com/8256043/diff/1/state/machine.go#newcode465
state/machine.go:465: panic("must set info for machine error status")
panic("machine error status with no info") ?

[fwereade, 2013-04-02 15:33:42]

Looking good, but I'd like to see it again before it's merged please.

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (left):

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go#oldcode224
cmd/juju/status.go:224: func processStatus(r map[string]interface{}, s status,
agentAlive, unitDead bool) {
Should this maybe be processUnitStatus, just to be clear, given the unitDead
param?

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go#newcode227
cmd/juju/status.go:227: return
I don't think we should be discarding errors.

https://codereview.appspot.com/8256043/diff/1/state/api/params/status.go
File state/api/params/status.go (right):

https://codereview.appspot.com/8256043/diff/1/state/api/params/status.go#newc...
state/api/params/status.go:15: // MachineStatus represents the status of the
machine agent.
OK, let's try to recast these. The "agent" stuff is, I think, not a primary
feature of unit or machine status: not everything that can be communicated here
is necessarily about, or even done by, the agent itself. Here are the actual
current meanings of the given statuses, as best I can characterise them:

"pending"   // The unit is not yet participating in the
            // environment.
"installed" // The unit has performed initial setup and is
            // adapting itself to the environment (???).
"started"   // The unit is actively participating in the
            // environment.
"error"     // The unit requires human intervention in
            // order to operate correctly.
"stopped"   // The unit's agent will perform no further
            // action, other than to set the unit to Dead
            // at a suitable moment.
"down"      // The unit agent ought to be signalling
            // activity, but it cannot be detected.

...and in such a form that the definitions can be reused for machine status with
trivial substitutions. Does that look like it's pitched at a sensible level?

https://codereview.appspot.com/8256043/diff/1/state/api/params/status.go#newc...
state/api/params/status.go:22: MachineError   MachineStatus = "error"   // Agent
is waiting in an error state
This is not necessarily true: one of the anticipated meanings of "error" was to
cover provisioning failures. I think the form of words I gave above will cover
it, though.

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

https://codereview.appspot.com/8256043/diff/1/state/machine.go#newcode449
state/machine.go:449: // Status returns the status of the machine's agent.
I think drop the "agent" bit.

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

https://codereview.appspot.com/8256043/diff/1/state/machine_test.go#newcode748
state/machine_test.go:748: c.Assert(failError, PanicMatches, "must set info for
machine error status")
Not sure the panics actually add much value, but I'm not too bothered... at
least it's consistent with unit.

https://codereview.appspot.com/8256043/diff/1/state/machine_test.go#newcode768
state/machine_test.go:768: }
Add some tests for get/set status as lifecycle changes: I think that get should
work until removal, and set until dead. Sane?

https://codereview.appspot.com/8256043/diff/1/state/unit.go
File state/unit.go (left):

https://codereview.appspot.com/8256043/diff/1/state/unit.go#oldcode452
state/unit.go:452: }
If these are the only changes in this file, we've got a pre-existing
status/resolved race. Probably best not to fix it now; please add a bug.

https://codereview.appspot.com/8256043/diff/1/state/unit_test.go
File state/unit_test.go (left):

https://codereview.appspot.com/8256043/diff/1/state/unit_test.go#oldcode145
state/unit_test.go:145: }
Lifecycle status tests, as in machine (some of these probably already exist, but
now we have separate collections we need to take more care ensuring things are
cleaned up properly).

[TheMue, 2013-04-02 15:39:44]

On 2013/04/02 15:29:07, rog wrote:
> LGTM with the below comments addressed.
> 
> FWIW i echo frank's concerns. as discussed online i really don't see a
> particular benefit from putting a unit's status in a separate collection - it
> has more failure modes (you now need a conditional transaction to set a
status)
> and requires more round trips in common cases (status for example).

Yep, exactly. The data of one entity is spread over multiple docs which isn't
the idea of a database like MongoDB. We have a transaction mechanism, but it's
not based on the DBMS itself. So I prefer to avoid non-atomic operations as far
as possible.

[dimitern, 2013-04-03 09:33:00]

Thanks for the reviews! It really made the CL better after the suggestions!

Will repropose with the changes shortly.

>> FWIW i echo frank's concerns. as discussed online i really don't see a
>> particular benefit from putting a unit's status in a separate collection - it
>> has more failure modes (you now need a conditional transaction to set a
status)
>> and requires more round trips in common cases (status for example).

> Yep, exactly. The data of one entity is spread over multiple docs which isn't
> the idea of a database like MongoDB. We have a transaction mechanism, but it's
> not based on the DBMS itself. So I prefer to avoid non-atomic operations as
far
> as possible.

The benefit is less duplicated code and better logic, IMHO.
There are exactly as many failure points as before (unit status used to
return an error before I changed it recently, now it does again). True
for more round trips, but it's unavoidable with separate collections,
and it's a small price to pay for better code I think.
The operations are as atomic as the can get with txn/mgo, so no trouble
there - there are asserts and ops for both status and unit/machine docs,
and it's a single transaction.
Am I missing something?

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (left):

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go#oldcode224
cmd/juju/status.go:224: func processStatus(r map[string]interface{}, s status,
agentAlive, unitDead bool) {
On 2013/04/02 15:33:42, fwereade wrote:
> Should this maybe be processUnitStatus, just to be clear, given the unitDead
> param?

Done. I also removed the now unnecessary interface status, because units and
machines have different types for status.

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8256043/diff/1/cmd/juju/status.go#newcode226
cmd/juju/status.go:226: if err != nil {
On 2013/04/02 15:29:07, rog wrote:
> this doesn't seem right to me. i don't think we should be ignoring the error
> here.

I moved getting the status out into processUnit, which handles errors correctly.

https://codereview.appspot.com/8256043/diff/1/state/api/params/status.go
File state/api/params/status.go (right):

https://codereview.appspot.com/8256043/diff/1/state/api/params/status.go#newc...
state/api/params/status.go:15: // MachineStatus represents the status of the
machine agent.
On 2013/04/02 15:33:42, fwereade wrote:
> OK, let's try to recast these. The "agent" stuff is, I think, not a primary
> feature of unit or machine status: not everything that can be communicated
here
> is necessarily about, or even done by, the agent itself. Here are the actual
> current meanings of the given statuses, as best I can characterise them:
> 
> "pending"   // The unit is not yet participating in the
>             // environment.
> "installed" // The unit has performed initial setup and is
>             // adapting itself to the environment (???).
> "started"   // The unit is actively participating in the
>             // environment.
> "error"     // The unit requires human intervention in
>             // order to operate correctly.
> "stopped"   // The unit's agent will perform no further
>             // action, other than to set the unit to Dead
>             // at a suitable moment.
> "down"      // The unit agent ought to be signalling
>             // activity, but it cannot be detected.
> 
> ...and in such a form that the definitions can be reused for machine status
with
> trivial substitutions. Does that look like it's pitched at a sensible level?

Thanks for the much better descriptions! Done.

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

https://codereview.appspot.com/8256043/diff/1/state/machine.go#newcode449
state/machine.go:449: // Status returns the status of the machine's agent.
On 2013/04/02 15:33:42, fwereade wrote:
> I think drop the "agent" bit.

Done.

https://codereview.appspot.com/8256043/diff/1/state/machine.go#newcode463
state/machine.go:463: panic("machine status must not be set to pending")
On 2013/04/02 15:29:07, rog wrote:
> i prefer panic messages to say what went wrong rather than an instruction as
to
> what to do right.
> 
> for example:
> panic("cannot set machine status back to pending")

Fair enough, changed.

https://codereview.appspot.com/8256043/diff/1/state/machine.go#newcode465
state/machine.go:465: panic("must set info for machine error status")
On 2013/04/02 15:29:07, rog wrote:
> panic("machine error status with no info") ?

Done.

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

https://codereview.appspot.com/8256043/diff/1/state/machine_test.go#newcode748
state/machine_test.go:748: c.Assert(failError, PanicMatches, "must set info for
machine error status")
On 2013/04/02 15:33:42, fwereade wrote:
> Not sure the panics actually add much value, but I'm not too bothered... at
> least it's consistent with unit.

I prefer to leave them for now, at least until we change the  MA to use the
status and see what needs changing.

https://codereview.appspot.com/8256043/diff/1/state/machine_test.go#newcode768
state/machine_test.go:768: }
On 2013/04/02 15:33:42, fwereade wrote:
> Add some tests for get/set status as lifecycle changes: I think that get
should
> work until removal, and set until dead. Sane?

Good point! Added; also for unit status.

https://codereview.appspot.com/8256043/diff/1/state/unit.go
File state/unit.go (left):

https://codereview.appspot.com/8256043/diff/1/state/unit.go#oldcode452
state/unit.go:452: }
On 2013/04/02 15:33:42, fwereade wrote:
> If these are the only changes in this file, we've got a pre-existing
> status/resolved race. Probably best not to fix it now; please add a bug.

Not sure I'm following you there - can you explain a bit more?

https://codereview.appspot.com/8256043/diff/1/state/unit_test.go
File state/unit_test.go (left):

https://codereview.appspot.com/8256043/diff/1/state/unit_test.go#oldcode145
state/unit_test.go:145: }
On 2013/04/02 15:33:42, fwereade wrote:
> Lifecycle status tests, as in machine (some of these probably already exist,
but
> now we have separate collections we need to take more care ensuring things are
> cleaned up properly).

Done. I caught a case when the error was not reported correctly when the unit is
removed, so updated Status() of both unit and machine to do that and tests for
it.
In the process of doing this, I realised status docs are not deleted when the
entity is deleted! So I added removeStatusOp and changed unit/machine removal to
call it.

[dimitern, 2013-04-03 09:39:06]

Please take a look.

[fwereade, 2013-04-03 11:13:57]

On 2013/04/02 15:39:44, TheMue wrote:
> On 2013/04/02 15:29:07, rog wrote:
> > LGTM with the below comments addressed.
> > 
> > FWIW i echo frank's concerns. as discussed online i really don't see a
> > particular benefit from putting a unit's status in a separate collection -
it
> > has more failure modes (you now need a conditional transaction to set a
> status)
> > and requires more round trips in common cases (status for example).

re failure modes, I'm not quite sure what you're getting at. It requires a
conditional txn regardless, if we want it to be correct; are you concerned about
the multi-document nature of the txn?

re round trips: status may be commonly *used*, but I think it's very much a
special case, because it's the only thing that wants to know about the complete
state of a whole environment (and the only thing that wants to know status at
all). AFAICS it'll be trivial to pull a bunch of status docs down in one go when
we need to do so; and the status-collection work in the AllWatcher will
similarly be no different to the annotation-collection work.

re benefits: consider the usage patterns of the various fields in play. The txn
package gives us some useful tools for atomically updating subsections of a
document, which is fantastic, but the watchers only work at the document level:
every write to a document notifies every watcher of that document, and all of
those then need to pull down the document and do some work to interpret the
change; we would not be wise to discount the delayed minimum cost of a write,
which is that of sending a notification and probably a full document out to each
of N observers.

This STM to be another case -- just like relation scopes and settings, and
service settings and units, and everything annotations, et cetera -- in which an
entity has some associated data that is only of interest to a few specific
observers and is therefore stored in a different doc. And as a bonus we get some
measure of separation of concerns in the code as well, as noted by dimiter, even
if it's not necessarily well reflected in the state API.

> Yep, exactly. The data of one entity is spread over multiple docs which isn't
> the idea of a database like MongoDB. We have a transaction mechanism, but it's
> not based on the DBMS itself. So I prefer to avoid non-atomic operations as
far
> as possible.

I would agree that MongoDB does in general lend itself to less-normalized
representations of data than I might choose for, say, Postgres, for a host of
reasons; but I don't think that implies we should always prefer to put related
data together without considering the nature of the relationship and how it
needs to be used.

I agree that the answer is rarely clear-cut, and accept that we won't always
make the right decisions; but at the moment I don't think the action of
splitting docs (or for that matter merging them) should be considered
intrinsically problematic. That'll change soon enough, though...

[fwereade, 2013-04-03 11:42:34]

I'd like a chat about the following, and agreement on which if any we're going
to address and when.

https://codereview.appspot.com/8256043/diff/11001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8256043/diff/11001/cmd/juju/status.go#newcode224
cmd/juju/status.go:224: func processUnitStatus(r map[string]interface{}, status
params.UnitStatus, info string, agentAlive, unitDead bool) {
Given the param list, it might be best to just inline this. But follow your
heart.

https://codereview.appspot.com/8256043/diff/11001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8256043/diff/11001/juju/conn.go#newcode322
juju/conn.go:322: return fmt.Errorf("unit %q is not in an error state", unit)
status/resolved race exemplified here: the state could change before we set
resolved. Already existed, not actually a very big deal -- in fact I just found
a TODO for it in SetResolved :).

https://codereview.appspot.com/8256043/diff/11001/state/api/params/status.go
File state/api/params/status.go (right):

https://codereview.appspot.com/8256043/diff/11001/state/api/params/status.go#...
state/api/params/status.go:8: UnitPending UnitStatus = "pending"
Please add blank lines between the definitions and the succeeding doc comments.

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

https://codereview.appspot.com/8256043/diff/11001/state/machine.go#newcode300
state/machine.go:300: removeStatusOp(m.st, m),
I don't really like the inconsistency here. IMO we should either be using
globalKeyer either a lot, or not at all... using it in just a few places doesn't
feel worth it.

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

https://codereview.appspot.com/8256043/diff/11001/state/machine_test.go#newco...
state/machine_test.go:768: 
Please split the test case here.

https://codereview.appspot.com/8256043/diff/11001/state/machine_test.go#newco...
state/machine_test.go:779: // When the machine is dead, it should fail.
Dead and removed are different; please check both. I'm pretty sure sets should
stop working when Dead, and gets when removed.

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

https://codereview.appspot.com/8256043/diff/11001/state/service.go#newcode611
state/service.go:611: ops = append(ops, removeStatusOp(s.st, u))
This op should come *after* the unit remove op, where the annotations remove op
does -- at any given nanosecond, existence of the unit doc should imply
existence of the status doc (and the annotations doc).

(and the constraints doc. Bah. While I'm slapping my own wrist, would you either
fix or TODO that please?)

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

https://codereview.appspot.com/8256043/diff/11001/state/status.go#newcode10
state/status.go:10: // globalKeyer represents the interfave, used to get a
global key of a
s/interfave/interface/

...but, actually, please drop this and just pass the result of globalKey()
directly into the *Op funcs. It's not worth introducing this if we're not using
it widely.

https://codereview.appspot.com/8256043/diff/11001/state/status.go#newcode30
state/status.go:30: }
I think that either the status docs should go alongside the methods that use
them, or the methods that use them should migrate here.

I prefer the second solution, but for consistency's sake please use the first:
we can do large-scale reorganizations another day.

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

https://codereview.appspot.com/8256043/diff/11001/state/unit.go#newcode434
state/unit.go:434: return params.UnitPending, "", nil
I'm not sure that lazy creation of status is a very good idea, on balance. With
perspective, it seems like complexity for little gain.

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

https://codereview.appspot.com/8256043/diff/11001/state/unit_test.go#newcode148
state/unit_test.go:148: 
Split test case please.

[dimitern, 2013-04-03 14:29:26]

Please take a look.

https://codereview.appspot.com/8256043/diff/11001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8256043/diff/11001/cmd/juju/status.go#newcode224
cmd/juju/status.go:224: func processUnitStatus(r map[string]interface{}, status
params.UnitStatus, info string, agentAlive, unitDead bool) {
On 2013/04/03 11:42:34, fwereade wrote:
> Given the param list, it might be best to just inline this. But follow your
> heart.

Inlined into processUnit, as discussed.

https://codereview.appspot.com/8256043/diff/11001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8256043/diff/11001/juju/conn.go#newcode322
juju/conn.go:322: return fmt.Errorf("unit %q is not in an error state", unit)
On 2013/04/03 11:42:34, fwereade wrote:
> status/resolved race exemplified here: the state could change before we set
> resolved. Already existed, not actually a very big deal -- in fact I just
found
> a TODO for it in SetResolved :).

As agreed, I'll leave this as is for now.

https://codereview.appspot.com/8256043/diff/11001/state/api/params/status.go
File state/api/params/status.go (right):

https://codereview.appspot.com/8256043/diff/11001/state/api/params/status.go#...
state/api/params/status.go:8: UnitPending UnitStatus = "pending"
On 2013/04/03 11:42:34, fwereade wrote:
> Please add blank lines between the definitions and the succeeding doc
comments.

Done.

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

https://codereview.appspot.com/8256043/diff/11001/state/machine.go#newcode300
state/machine.go:300: removeStatusOp(m.st, m),
On 2013/04/03 11:42:34, fwereade wrote:
> I don't really like the inconsistency here. IMO we should either be using
> globalKeyer either a lot, or not at all... using it in just a few places
doesn't
> feel worth it.

As discussed, I'm removing globalKeyer until there is a better logic behind it.

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

https://codereview.appspot.com/8256043/diff/11001/state/machine_test.go#newco...
state/machine_test.go:768: 
On 2013/04/03 11:42:34, fwereade wrote:
> Please split the test case here.

Done.

https://codereview.appspot.com/8256043/diff/11001/state/machine_test.go#newco...
state/machine_test.go:779: // When the machine is dead, it should fail.
On 2013/04/03 11:42:34, fwereade wrote:
> Dead and removed are different; please check both. I'm pretty sure sets should
> stop working when Dead, and gets when removed.

Done.

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

https://codereview.appspot.com/8256043/diff/11001/state/service.go#newcode611
state/service.go:611: ops = append(ops, removeStatusOp(s.st, u))
On 2013/04/03 11:42:34, fwereade wrote:
> This op should come *after* the unit remove op, where the annotations remove
op
> does -- at any given nanosecond, existence of the unit doc should imply
> existence of the status doc (and the annotations doc).
> 
> (and the constraints doc. Bah. While I'm slapping my own wrist, would you
either
> fix or TODO that please?)

As discussed, moved constraints and annotations removal here, rather than at the
end, so when the last unit of the service is removed they'll be removed as well.

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

https://codereview.appspot.com/8256043/diff/11001/state/status.go#newcode10
state/status.go:10: // globalKeyer represents the interfave, used to get a
global key of a
On 2013/04/03 11:42:34, fwereade wrote:
> s/interfave/interface/
> 
> ...but, actually, please drop this and just pass the result of globalKey()
> directly into the *Op funcs. It's not worth introducing this if we're not
using
> it widely.

Dropped until better days :)

https://codereview.appspot.com/8256043/diff/11001/state/status.go#newcode30
state/status.go:30: }
On 2013/04/03 11:42:34, fwereade wrote:
> I think that either the status docs should go alongside the methods that use
> them, or the methods that use them should migrate here.
> 
> I prefer the second solution, but for consistency's sake please use the first:
> we can do large-scale reorganizations another day.

If I understood you correctly, I did this by moving the docs in unit/machine.go
respectively.

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

https://codereview.appspot.com/8256043/diff/11001/state/unit.go#newcode434
state/unit.go:434: return params.UnitPending, "", nil
On 2013/04/03 11:42:34, fwereade wrote:
> I'm not sure that lazy creation of status is a very good idea, on balance.
With
> perspective, it seems like complexity for little gain.

Now the creation is explicit, rather than lazy - createStatusOp (as discussed)
added to both addMachineOps and addUnitOps.

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

https://codereview.appspot.com/8256043/diff/11001/state/unit_test.go#newcode148
state/unit_test.go:148: 
On 2013/04/03 11:42:34, fwereade wrote:
> Split test case please.

Done.

https://codereview.appspot.com/8256043/diff/11001/state/unit_test.go#newcode148
state/unit_test.go:148: 
On 2013/04/03 11:42:34, fwereade wrote:
> Split test case please.

Done.

[fwereade, 2013-04-03 14:39:11]

LGTM, just a doc suggestion

https://codereview.appspot.com/8256043/diff/21001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8256043/diff/21001/state/machine.go#newcode65
state/machine.go:65: // There is an implicit _id field here, which mongo
creates, which is the
Mayse something a bit more like:

// The implicit _id field is explicitly specified set to the global key of the
associated machine in the document's creation transaction, but is omitted to
allow direct use of the document in both create and update transaction
operations. 

...and do the same for unit.

[dimitern, 2013-04-03 14:53:33]

*** Submitted:

state: add machine status; refractor unit status

Added Machine.Status() and SetStatus(), as with a unit.
Since we're using the same code, a refactoring was in
order: common code for managing statuses moved to a
separate state collection (see status.go); status consts
by the API and state added to state/api/params, so they
can be accessed from both.

Some changes to allWatcher/megawatcher code: because
now Status and StatusInfo are no longer part of the
unit document, you cannot watch for these changes, but
arguably only the juju status command actually needs
to read them, so the API should be handle this eventually.

Drive-by fix: constranits and annotations (and now status)
of the last unit of a service were not removed properly.
Now they are.

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

https://codereview.appspot.com/8256043/diff/21001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8256043/diff/21001/state/machine.go#newcode65
state/machine.go:65: // There is an implicit _id field here, which mongo
creates, which is the
On 2013/04/03 14:39:11, fwereade wrote:
> Mayse something a bit more like:
> 
> // The implicit _id field is explicitly specified set to the global key of the
> associated machine in the document's creation transaction, but is omitted to
> allow direct use of the document in both create and update transaction
> operations. 
> 
> ...and do the same for unit.

Done.