Merge lp:~themue/juju-core/go-state-unit-status into lp:~juju/juju-core/trunk

Proposed by Frank Mueller
Status: Merged
Approved by: Gustavo Niemeyer
Approved revision: no longer in the source branch.
Merged at revision: 391
Proposed branch: lp:~themue/juju-core/go-state-unit-status
Merge into: lp:~juju/juju-core/trunk
Diff against target: 125 lines (+97/-0)
2 files modified
state/unit.go (+62/-0)
state/unit_test.go (+35/-0)
To merge this branch: bzr merge lp:~themue/juju-core/go-state-unit-status
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+118549@code.launchpad.net

Description of the change

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://codereview.appspot.com/6454113/

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :
Revision history for this message
Gustavo Niemeyer (niemeyer) 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.
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).

https://codereview.appspot.com/6454113/

Revision history for this message
William Reade (fwereade) 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/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").

https://codereview.appspot.com/6454113/

Revision history for this message
Roger Peppe (rogpeppe) 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.

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.

https://codereview.appspot.com/6454113/

Revision history for this message
William Reade (fwereade) wrote :

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.

https://codereview.appspot.com/6454113/

Revision history for this message
William Reade (fwereade) wrote :

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)
}()

https://codereview.appspot.com/6454113/

Revision history for this message
William Reade (fwereade) wrote :

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

https://codereview.appspot.com/6454113/

Revision history for this message
William Reade (fwereade) wrote :
Revision history for this message
Gustavo Niemeyer (niemeyer) 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:
> 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?

https://codereview.appspot.com/6454113/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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

https://codereview.appspot.com/6454113/

Revision history for this message
William Reade (fwereade) 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:
> > 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?
>
> https://codereview.appspot.com/6454113/

SGTM -- I hadn't been sure we'd actually agreed to save more hook info in the short term. We should be careful to keep existing fields in juju status output consistent with python.

Revision history for this message
William Reade (fwereade) wrote :

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.

https://codereview.appspot.com/6454113/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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.

https://codereview.appspot.com/6454113/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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/

https://codereview.appspot.com/6454113/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'state/unit.go'
--- state/unit.go 2012-08-15 13:03:27 +0000
+++ state/unit.go 2012-08-15 15:48:21 +0000
@@ -33,6 +33,18 @@
33 AssignUnused AssignmentPolicy = "unused"33 AssignUnused AssignmentPolicy = "unused"
34)34)
3535
36// UnitStatus represents the status of the unit agent.
37type UnitStatus string
38
39const (
40 UnitPending UnitStatus = "pending" // Agent hasn't started
41 UnitInstalled UnitStatus = "installed" // Agent has run the installed hook
42 UnitStarted UnitStatus = "started" // Agent is running properly
43 UnitStopped UnitStatus = "stopped" // Agent has stopped running on request
44 UnitError UnitStatus = "error" // Agent is waiting in an error state
45 UnitDown UnitStatus = "down" // Agent is down or not communicating
46)
47
36// NeedsUpgrade describes if a unit needs an48// NeedsUpgrade describes if a unit needs an
37// upgrade and if this is forced.49// upgrade and if this is forced.
38type NeedsUpgrade struct {50type NeedsUpgrade struct {
@@ -157,6 +169,56 @@
157 "private address of unit %q", u)169 "private address of unit %q", u)
158}170}
159171
172// Status returns the status of the unit's agent.
173func (u *Unit) Status() (UnitStatus, string, error) {
174 cn, err := readConfigNode(u.st.zk, u.zkPath())
175 if err != nil {
176 return "", "", err
177 }
178 raw, found := cn.Get("status")
179 if !found {
180 return UnitPending, "", nil
181 }
182 status := UnitStatus(raw.(string))
183 switch status {
184 case UnitError:
185 // We always expect an info if status is 'error'.
186 raw, found = cn.Get("status-info")
187 if !found {
188 panic("no status-info for unit error found")
189 }
190 return status, raw.(string), nil
191 case UnitStopped:
192 return UnitStopped, "", nil
193 }
194 alive, err := u.AgentAlive()
195 if err != nil {
196 return "", "", err
197 }
198 if !alive {
199 status = UnitDown
200 }
201 return status, "", nil
202}
203
204// SetStatus sets the status of the unit.
205func (u *Unit) SetStatus(status UnitStatus, info string) error {
206 if status == UnitPending {
207 panic("unit status must not be set to pending")
208 }
209 cn, err := readConfigNode(u.st.zk, u.zkPath())
210 if err != nil {
211 return err
212 }
213 cn.Set("status", status)
214 cn.Set("status-info", info)
215 _, err = cn.Write()
216 if err != nil {
217 return fmt.Errorf("cannot set status of %q: %v", u, err)
218 }
219 return nil
220}
221
160// CharmURL returns the charm URL this unit is supposed222// CharmURL returns the charm URL this unit is supposed
161// to use.223// to use.
162func (u *Unit) CharmURL() (url *charm.URL, err error) {224func (u *Unit) CharmURL() (url *charm.URL, err error) {
163225
=== modified file 'state/unit_test.go'
--- state/unit_test.go 2012-07-26 13:52:05 +0000
+++ state/unit_test.go 2012-08-15 15:48:21 +0000
@@ -44,6 +44,41 @@
44 c.Assert(address, Equals, "example.local")44 c.Assert(address, Equals, "example.local")
45}45}
4646
47func (s *UnitSuite) TestGetSetStatus(c *C) {
48 fail := func() { s.unit.SetStatus(state.UnitPending, "") }
49 c.Assert(fail, PanicMatches, "unit status must not be set to pending")
50
51 status, info, err := s.unit.Status()
52 c.Assert(err, IsNil)
53 c.Assert(status, Equals, state.UnitPending)
54 c.Assert(info, Equals, "")
55
56 err = s.unit.SetStatus(state.UnitStarted, "")
57 c.Assert(err, IsNil)
58 status, info, err = s.unit.Status()
59 c.Assert(err, IsNil)
60 c.Assert(status, Equals, state.UnitDown)
61 c.Assert(info, Equals, "")
62
63 p, err := s.unit.SetAgentAlive()
64 c.Assert(err, IsNil)
65 defer func() {
66 c.Assert(p.Kill(), IsNil)
67 }()
68
69 status, info, err = s.unit.Status()
70 c.Assert(err, IsNil)
71 c.Assert(status, Equals, state.UnitStarted)
72 c.Assert(info, Equals, "")
73
74 err = s.unit.SetStatus(state.UnitError, "test-hook failed")
75 c.Assert(err, IsNil)
76 status, info, err = s.unit.Status()
77 c.Assert(err, IsNil)
78 c.Assert(status, Equals, state.UnitError)
79 c.Assert(info, Equals, "test-hook failed")
80}
81
47func (s *UnitSuite) TestUnitCharm(c *C) {82func (s *UnitSuite) TestUnitCharm(c *C) {
48 testcurl, err := s.unit.CharmURL()83 testcurl, err := s.unit.CharmURL()
49 c.Assert(err, IsNil)84 c.Assert(err, IsNil)

Subscribers

People subscribed via source and target branches