Merge lp:~themue/juju-core/go-state-unit-status into lp:~juju/juju-core/trunk
- go-state-unit-status
- Merge into trunk
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
The Go Language Gophers | Pending | ||
Review via email: mp+118549@code.launchpad.net |
Commit message
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:/
William Reade (fwereade) wrote : | # |
Gustavo Niemeyer (niemeyer) wrote : | # |
https:/
File state/unit.go (right):
https:/
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:/
state/unit.go:162: status, err := getConfigString
"status", "status of unit %q", u)
We've conventionally agreed to break the line down before the
description (see the rest).
William Reade (fwereade) wrote : | # |
https:/
File state/unit.go (right):
https:/
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:/
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").
Roger Peppe (rogpeppe) wrote : | # |
https:/
File state/unit.go (right):
https:/
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.
William Reade (fwereade) wrote : | # |
On 2012/08/08 09:57:36, rog wrote:
> https:/
> File state/unit.go (right):
https:/
> 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.
William Reade (fwereade) wrote : | # |
LGTM with test fix
https:/
File state/unit_test.go (right):
https:/
state/unit_
p, err := s.unit.
c.Assert(err, IsNil)
defer func(){
c.
}()
William Reade (fwereade) wrote : | # |
Whoops, sorry, missed a bit. With *that*, too, LGTM.
https:/
File state/unit.go (right):
https:/
state/unit.go:176: status = "pending"
return "pending", nil
William Reade (fwereade) wrote : | # |
Gustavo Niemeyer (niemeyer) wrote : | # |
https:/
File state/unit.go (right):
https:/
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?
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.
William Reade (fwereade) wrote : | # |
>
> https:/
> File state/unit.go (right):
>
> https:/
> 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:/
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.
William Reade (fwereade) wrote : | # |
LGTM, couple of trivials.
https:/
File state/unit.go (right):
https:/
state/unit.go:54: // UnitInfo allows to add an additional info, e.g. an
error reason
s/an //
https:/
state/unit.go:185: // Status returns the status of the unit's agent
s/agent/agent./
https:/
state/unit.go:199: // We allways expect an info if status is 'error'.
s/allways/always.
https:/
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.
Gustavo Niemeyer (niemeyer) wrote : | # |
Not there yet.
https:/
File state/unit.go (right):
https:/
state/unit.go:36: // UnitStatus represents the status of the unit's
agent.
s/'s//
https:/
state/unit.go:39: const (
As discussed on IRC with William:
http://
https:/
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(
error); ?
https:/
state/unit.go:200: info, err := getConfigString
"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:/
state/unit.go:229: return setConfigString
"status-info", info,
Ditto.
Gustavo Niemeyer (niemeyer) wrote : | # |
LGTM with trivials:
https:/
File state/unit.go (right):
https:/
state/unit.go:173: func (u *Unit) Status() (UnitStatus, string, error) {
(s UnitStatus, info string, err error)
https:/
state/unit.go:176: return "", "", err
return "", "", fmt.Errorf("cannot read status of unit %q: %v", u, err)
https:/
state/unit.go:188: panic("no status-info for unit error found")
"no status-info found for unit error"
https:/
state/unit.go:217: return fmt.Errorf("cannot set status of %q: %v", u,
err)
s/of/of unit/
Preview Diff
1 | === modified file 'state/unit.go' |
2 | --- state/unit.go 2012-08-15 13:03:27 +0000 |
3 | +++ state/unit.go 2012-08-15 15:48:21 +0000 |
4 | @@ -33,6 +33,18 @@ |
5 | AssignUnused AssignmentPolicy = "unused" |
6 | ) |
7 | |
8 | +// UnitStatus represents the status of the unit agent. |
9 | +type UnitStatus string |
10 | + |
11 | +const ( |
12 | + UnitPending UnitStatus = "pending" // Agent hasn't started |
13 | + UnitInstalled UnitStatus = "installed" // Agent has run the installed hook |
14 | + UnitStarted UnitStatus = "started" // Agent is running properly |
15 | + UnitStopped UnitStatus = "stopped" // Agent has stopped running on request |
16 | + UnitError UnitStatus = "error" // Agent is waiting in an error state |
17 | + UnitDown UnitStatus = "down" // Agent is down or not communicating |
18 | +) |
19 | + |
20 | // NeedsUpgrade describes if a unit needs an |
21 | // upgrade and if this is forced. |
22 | type NeedsUpgrade struct { |
23 | @@ -157,6 +169,56 @@ |
24 | "private address of unit %q", u) |
25 | } |
26 | |
27 | +// Status returns the status of the unit's agent. |
28 | +func (u *Unit) Status() (UnitStatus, string, error) { |
29 | + cn, err := readConfigNode(u.st.zk, u.zkPath()) |
30 | + if err != nil { |
31 | + return "", "", err |
32 | + } |
33 | + raw, found := cn.Get("status") |
34 | + if !found { |
35 | + return UnitPending, "", nil |
36 | + } |
37 | + status := UnitStatus(raw.(string)) |
38 | + switch status { |
39 | + case UnitError: |
40 | + // We always expect an info if status is 'error'. |
41 | + raw, found = cn.Get("status-info") |
42 | + if !found { |
43 | + panic("no status-info for unit error found") |
44 | + } |
45 | + return status, raw.(string), nil |
46 | + case UnitStopped: |
47 | + return UnitStopped, "", nil |
48 | + } |
49 | + alive, err := u.AgentAlive() |
50 | + if err != nil { |
51 | + return "", "", err |
52 | + } |
53 | + if !alive { |
54 | + status = UnitDown |
55 | + } |
56 | + return status, "", nil |
57 | +} |
58 | + |
59 | +// SetStatus sets the status of the unit. |
60 | +func (u *Unit) SetStatus(status UnitStatus, info string) error { |
61 | + if status == UnitPending { |
62 | + panic("unit status must not be set to pending") |
63 | + } |
64 | + cn, err := readConfigNode(u.st.zk, u.zkPath()) |
65 | + if err != nil { |
66 | + return err |
67 | + } |
68 | + cn.Set("status", status) |
69 | + cn.Set("status-info", info) |
70 | + _, err = cn.Write() |
71 | + if err != nil { |
72 | + return fmt.Errorf("cannot set status of %q: %v", u, err) |
73 | + } |
74 | + return nil |
75 | +} |
76 | + |
77 | // CharmURL returns the charm URL this unit is supposed |
78 | // to use. |
79 | func (u *Unit) CharmURL() (url *charm.URL, err error) { |
80 | |
81 | === modified file 'state/unit_test.go' |
82 | --- state/unit_test.go 2012-07-26 13:52:05 +0000 |
83 | +++ state/unit_test.go 2012-08-15 15:48:21 +0000 |
84 | @@ -44,6 +44,41 @@ |
85 | c.Assert(address, Equals, "example.local") |
86 | } |
87 | |
88 | +func (s *UnitSuite) TestGetSetStatus(c *C) { |
89 | + fail := func() { s.unit.SetStatus(state.UnitPending, "") } |
90 | + c.Assert(fail, PanicMatches, "unit status must not be set to pending") |
91 | + |
92 | + status, info, err := s.unit.Status() |
93 | + c.Assert(err, IsNil) |
94 | + c.Assert(status, Equals, state.UnitPending) |
95 | + c.Assert(info, Equals, "") |
96 | + |
97 | + err = s.unit.SetStatus(state.UnitStarted, "") |
98 | + c.Assert(err, IsNil) |
99 | + status, info, err = s.unit.Status() |
100 | + c.Assert(err, IsNil) |
101 | + c.Assert(status, Equals, state.UnitDown) |
102 | + c.Assert(info, Equals, "") |
103 | + |
104 | + p, err := s.unit.SetAgentAlive() |
105 | + c.Assert(err, IsNil) |
106 | + defer func() { |
107 | + c.Assert(p.Kill(), IsNil) |
108 | + }() |
109 | + |
110 | + status, info, err = s.unit.Status() |
111 | + c.Assert(err, IsNil) |
112 | + c.Assert(status, Equals, state.UnitStarted) |
113 | + c.Assert(info, Equals, "") |
114 | + |
115 | + err = s.unit.SetStatus(state.UnitError, "test-hook failed") |
116 | + c.Assert(err, IsNil) |
117 | + status, info, err = s.unit.Status() |
118 | + c.Assert(err, IsNil) |
119 | + c.Assert(status, Equals, state.UnitError) |
120 | + c.Assert(info, Equals, "test-hook failed") |
121 | +} |
122 | + |
123 | func (s *UnitSuite) TestUnitCharm(c *C) { |
124 | testcurl, err := s.unit.CharmURL() |
125 | c.Assert(err, IsNil) |
LGTM
https:/ /codereview. appspot. com/6454113/