state: add status to unit

state: add status to unit

Unit now has a getter and setter for a status. It
is intended to be used by the UA and in 'juju status'.
See https://bugs.launchpad.net/juju-core/+bug/1033858.

https://code.launchpad.net/~themue/juju-core/go-state-unit-status/+merge/118549

(do not edit description out of merge proposal)

[TheMue, 2012-08-07 12:55:13]

Please take a look.

[TheMue, 2012-08-07 13:11:46]

Please take a look.

[fwereade, 2012-08-07 13:17:49]

LGTM

[niemeyer, 2012-08-07 17:14:58]

https://codereview.appspot.com/6454113/diff/3001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode160
state/unit.go:160: // Status returns the status of the unit.
The whole thing is looking good, but it's missing more context of what this is.
What does "status of the unit" actually mean? What are the set of valid values?
Should these be constants? etc.

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode162
state/unit.go:162: status, err := getConfigString(u.st.zk, u.zkPath(), "status",
"status of unit %q", u)
We've conventionally agreed to break the line down before the description (see
the rest).

[fwereade, 2012-08-08 07:57:00]

https://codereview.appspot.com/6454113/diff/3001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode160
state/unit.go:160: // Status returns the status of the unit.
On 2012/08/07 17:14:58, niemeyer wrote:
> The whole thing is looking good, but it's missing more context of what this
is.
> What does "status of the unit" actually mean? What are the set of valid
values?
> Should these be constants? etc.

How about:

// Status returns the status of the unit's agent. Expected
// results include:
//   pending:   the agent has not started
//   installed: the agent has installed the charm
//   started:   the agent is running and nothing is wrong
//   stopped:   the agent has done everything it ever will
//   *-error:   the agent failed to run the referenced hook
//   down:      the agent is not functioning correctly

I don't *think* these should be constants, because the set of *-error states is
not predictable.

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode169
state/unit.go:169: }
Actually, sorry, I think it would be nice, at this point, to check AgentAlive
and return "down" if it's not (unless the status is "stopped").

[rog, 2012-08-08 09:57:36]

https://codereview.appspot.com/6454113/diff/3001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode160
state/unit.go:160: // Status returns the status of the unit.
On 2012/08/08 07:57:00, fwereade wrote:
> On 2012/08/07 17:14:58, niemeyer wrote:
> > The whole thing is looking good, but it's missing more context of what this
> is.
> > What does "status of the unit" actually mean? What are the set of valid
> values?
> > Should these be constants? etc.
> 
> How about:
> 
> // Status returns the status of the unit's agent. Expected
> // results include:
> //   pending:   the agent has not started
> //   installed: the agent has installed the charm
> //   started:   the agent is running and nothing is wrong
> //   stopped:   the agent has done everything it ever will
> //   *-error:   the agent failed to run the referenced hook
> //   down:      the agent is not functioning correctly
> 
> I don't *think* these should be constants, because the set of *-error states
is
> not predictable.

I might add to those:

// restarting: the agent is restarting after an upgrade.

Also, how about having error as a prefix, so that the actual
error can be held in the status and it's easy to check for error status in
general?

// error <reason>: the agent has stopped executing hooks for the given reason.

[fwereade, 2012-08-08 10:09:51]

On 2012/08/08 09:57:36, rog wrote:
> https://codereview.appspot.com/6454113/diff/3001/state/unit.go
> File state/unit.go (right):
> 
> https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode160
> state/unit.go:160: // Status returns the status of the unit.
> On 2012/08/08 07:57:00, fwereade wrote:
> > On 2012/08/07 17:14:58, niemeyer wrote:
> > > The whole thing is looking good, but it's missing more context of what
this
> > is.
> > > What does "status of the unit" actually mean? What are the set of valid
> > values?
> > > Should these be constants? etc.
> > 
> > How about:
> > 
> > // Status returns the status of the unit's agent. Expected
> > // results include:
> > //   pending:   the agent has not started
> > //   installed: the agent has installed the charm
> > //   started:   the agent is running and nothing is wrong
> > //   stopped:   the agent has done everything it ever will
> > //   *-error:   the agent failed to run the referenced hook
> > //   down:      the agent is not functioning correctly
> > 
> > I don't *think* these should be constants, because the set of *-error states
> is
> > not predictable.
> 
> I might add to those:
> 
> // restarting: the agent is restarting after an upgrade.

-0.5, would you expand on why this is where we should show this?

> Also, how about having error as a prefix, so that the actual
> error can be held in the status and it's easy to check for error status in
> general?
> 
> // error <reason>: the agent has stopped executing hooks for the given reason.

Would SGTM in a green field, but I think we want to keep the states we already
had in python.

[TheMue, 2012-08-08 15:02:48]

Please take a look.

https://codereview.appspot.com/6454113/diff/3001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode160
state/unit.go:160: // Status returns the status of the unit.
On 2012/08/08 07:57:00, fwereade wrote:
> On 2012/08/07 17:14:58, niemeyer wrote:
> > The whole thing is looking good, but it's missing more context of what this
> is.
> > What does "status of the unit" actually mean? What are the set of valid
> values?
> > Should these be constants? etc.
> 
> How about:
> 
> // Status returns the status of the unit's agent. Expected
> // results include:
> //   pending:   the agent has not started
> //   installed: the agent has installed the charm
> //   started:   the agent is running and nothing is wrong
> //   stopped:   the agent has done everything it ever will
> //   *-error:   the agent failed to run the referenced hook
> //   down:      the agent is not functioning correctly
> 
> I don't *think* these should be constants, because the set of *-error states
is
> not predictable.

Done.

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode162
state/unit.go:162: status, err := getConfigString(u.st.zk, u.zkPath(), "status",
"status of unit %q", u)
On 2012/08/07 17:14:58, niemeyer wrote:
> We've conventionally agreed to break the line down before the description (see
> the rest).

Done.

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode169
state/unit.go:169: }
On 2012/08/08 07:57:01, fwereade wrote:
> Actually, sorry, I think it would be nice, at this point, to check AgentAlive
> and return "down" if it's not (unless the status is "stopped").

Done.

[fwereade, 2012-08-08 15:07:51]

LGTM with test fix

https://codereview.appspot.com/6454113/diff/9001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/6454113/diff/9001/state/unit_test.go#newcode52
state/unit_test.go:52: c.Assert(err, IsNil)
p, err := s.unit.SetAgentAlive()
c.Assert(err, IsNil)
defer func(){
    c.Assert(p.Kill(), IsNil)
}()

[fwereade, 2012-08-08 15:09:56]

Whoops, sorry, missed a bit. With *that*, too, LGTM.

https://codereview.appspot.com/6454113/diff/9001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/9001/state/unit.go#newcode176
state/unit.go:176: status = "pending"
return "pending", nil

[TheMue, 2012-08-08 15:22:16]

Please take a look.

https://codereview.appspot.com/6454113/diff/9001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/9001/state/unit.go#newcode176
state/unit.go:176: status = "pending"
On 2012/08/08 15:09:56, fwereade wrote:
> return "pending", nil

Done.

https://codereview.appspot.com/6454113/diff/9001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/6454113/diff/9001/state/unit_test.go#newcode52
state/unit_test.go:52: c.Assert(err, IsNil)
On 2012/08/08 15:07:51, fwereade wrote:
> p, err := s.unit.SetAgentAlive()
> c.Assert(err, IsNil)
> defer func(){
>     c.Assert(p.Kill(), IsNil)
> }()

Done.

[fwereade, 2012-08-08 15:23:42]

LGTM

[niemeyer, 2012-08-13 15:06:27]

https://codereview.appspot.com/6454113/diff/3001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/3001/state/unit.go#newcode160
state/unit.go:160: // Status returns the status of the unit.
On 2012/08/08 07:57:00, fwereade wrote:
> I don't *think* these should be constants, because the set of *-error states
is
> not predictable.

The status not being predictable is quite a bummer. Also, the documentation for
this method looks *very* much like the documentation for a bunch of constants
that ought to be properly declared in the code as such.

Can we fix this now?

What if we had a single "error" status code for errors, and an additional info
field regarding why it's broken?

We've already agreed in the sprint that any hook errors will freeze the unit,
and also agreed to save more details of erroring hooks, so all seems to be going
in a good direction in those terms.

Comments?

[niemeyer, 2012-08-13 15:20:30]

We have a few people off today, so I'm marking this as WIP so we can discuss
this live tomorrow somewhere.

[TheMue, 2012-08-14 16:34:13]

Please take a look.

[fwereade, 2012-08-15 10:10:56]

LGTM, couple of trivials.

https://codereview.appspot.com/6454113/diff/8005/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/8005/state/unit.go#newcode54
state/unit.go:54: // UnitInfo allows to add an additional info, e.g. an error
reason
s/an //

https://codereview.appspot.com/6454113/diff/8005/state/unit.go#newcode185
state/unit.go:185: // Status returns the status of the unit's agent
s/agent/agent./

https://codereview.appspot.com/6454113/diff/8005/state/unit.go#newcode199
state/unit.go:199: // We allways expect an info if status is 'error'.
s/allways/always.

https://codereview.appspot.com/6454113/diff/8005/state/unit.go#newcode220
state/unit.go:220: func (u *Unit) SetStatus(status UnitStatus, info string)
error {
Maybe a check that we're not setting "pending", which is guaranteed crack.

[TheMue, 2012-08-15 11:02:03]

Please take a look.

https://codereview.appspot.com/6454113/diff/8005/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/8005/state/unit.go#newcode54
state/unit.go:54: // UnitInfo allows to add an additional info, e.g. an error
reason
On 2012/08/15 10:10:56, fwereade wrote:
> s/an //

Done.

https://codereview.appspot.com/6454113/diff/8005/state/unit.go#newcode185
state/unit.go:185: // Status returns the status of the unit's agent
On 2012/08/15 10:10:56, fwereade wrote:
> s/agent/agent./

Done.

https://codereview.appspot.com/6454113/diff/8005/state/unit.go#newcode199
state/unit.go:199: // We allways expect an info if status is 'error'.
On 2012/08/15 10:10:56, fwereade wrote:
> s/allways/always.

Done.

[niemeyer, 2012-08-15 14:53:41]

Not there yet.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode36
state/unit.go:36: // UnitStatus represents the status of the unit's agent.
s/'s//

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode39
state/unit.go:39: const (
As discussed on IRC with William:

http://paste.ubuntu.com/1148913/

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode54
state/unit.go:54: // UnitInfo allows to add additional info, e.g. an error
reason
This seems like a better plan:

Aug 14 11:53:31 <fwereade_>     TheMue, so, agreed, I think:
SetStatus(UnitStatus, string) error; Status() (UnitStatus, string, error); ?

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode200
state/unit.go:200: info, err := getConfigString(u.st.zk, u.zkPath(),
"status-info",
What if the status changed since you last looked?

Grab the config node once and use it rather than reloading it every time you
feel the need to look at its content.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode229
state/unit.go:229: return setConfigString(u.st.zk, u.zkPath(), "status-info",
info,
Ditto.

[TheMue, 2012-08-15 15:46:54]

Please take a look.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode36
state/unit.go:36: // UnitStatus represents the status of the unit's agent.
On 2012/08/15 14:53:41, niemeyer wrote:
> s/'s//

Done.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode39
state/unit.go:39: const (
On 2012/08/15 14:53:41, niemeyer wrote:
> As discussed on IRC with William:
> 
> http://paste.ubuntu.com/1148913/

Done.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode54
state/unit.go:54: // UnitInfo allows to add additional info, e.g. an error
reason
On 2012/08/15 14:53:41, niemeyer wrote:
> This seems like a better plan:
> 
> Aug 14 11:53:31 <fwereade_>     TheMue, so, agreed, I think:
> SetStatus(UnitStatus, string) error; Status() (UnitStatus, string, error); ?

Done.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode200
state/unit.go:200: info, err := getConfigString(u.st.zk, u.zkPath(),
"status-info",
On 2012/08/15 14:53:41, niemeyer wrote:
> What if the status changed since you last looked?
> 
> Grab the config node once and use it rather than reloading it every time you
> feel the need to look at its content.

Done.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode229
state/unit.go:229: return setConfigString(u.st.zk, u.zkPath(), "status-info",
info,
On 2012/08/15 14:53:41, niemeyer wrote:
> Ditto.

Done.

[niemeyer, 2012-08-16 00:14:49]

LGTM with trivials:

https://codereview.appspot.com/6454113/diff/9004/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode173
state/unit.go:173: func (u *Unit) Status() (UnitStatus, string, error) {
(s UnitStatus, info string, err error)

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode176
state/unit.go:176: return "", "", err
return "", "", fmt.Errorf("cannot read status of unit %q: %v", u, err)

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode188
state/unit.go:188: panic("no status-info for unit error found")
"no status-info found for unit error"

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode217
state/unit.go:217: return fmt.Errorf("cannot set status of %q: %v", u, err)
s/of/of unit/

[TheMue, 2012-08-16 09:29:24]

*** Submitted:

state: add status to unit

Unit now has a getter and setter for a status. It
is intended to be used by the UA and in 'juju status'.
See https://bugs.launchpad.net/juju-core/+bug/1033858.

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

https://codereview.appspot.com/6454113/diff/9004/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode173
state/unit.go:173: func (u *Unit) Status() (UnitStatus, string, error) {
On 2012/08/16 00:14:50, niemeyer wrote:
> (s UnitStatus, info string, err error)

Done.

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode176
state/unit.go:176: return "", "", err
On 2012/08/16 00:14:50, niemeyer wrote:
> return "", "", fmt.Errorf("cannot read status of unit %q: %v", u, err)

Done.

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode188
state/unit.go:188: panic("no status-info for unit error found")
On 2012/08/16 00:14:50, niemeyer wrote:
> "no status-info found for unit error"

Done.

https://codereview.appspot.com/6454113/diff/9004/state/unit.go#newcode217
state/unit.go:217: return fmt.Errorf("cannot set status of %q: %v", u, err)
On 2012/08/16 00:14:50, niemeyer wrote:
> s/of/of unit/

Done.