Merge lp:~themue/juju-core/020-cmd-status-subordinates into lp:~juju/juju-core/trunk
- 020-cmd-status-subordinates
- Merge into trunk
Status: | Rejected |
---|---|
Rejected by: | William Reade |
Proposed branch: | lp:~themue/juju-core/020-cmd-status-subordinates |
Merge into: | lp:~juju/juju-core/trunk |
Diff against target: |
303 lines (+212/-9) 2 files modified
cmd/juju/status.go (+57/-9) cmd/juju/status_test.go (+155/-0) |
To merge this branch: | bzr merge lp:~themue/juju-core/020-cmd-status-subordinates |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+159339@code.launchpad.net |
Commit message
Description of the change
status: added the display of subordinates
Added the display of subordinated units below their
associated units and "subordinate-to" for the
subordinated services.
William Reade (fwereade) wrote : | # |
William Reade (fwereade) wrote : | # |
LGTM wrt output. Code improvements appreciated but not required.
https:/
File cmd/juju/status.go (right):
https:/
cmd/juju/
"units")
I remain suspicious of this, but the output looks sane, we can worry
about this another day.
https:/
cmd/juju/
Why are we doing this over and over again for every unit?
https:/
File cmd/juju/
https:/
cmd/juju/
those logs",
:)
https:/
cmd/juju/
Do we have anything for testing non-agent-alive units?
https:/
cmd/juju/
Please don't do this. Create the relation separately, and get it here
with Relation.
Dimiter Naydenov (dimitern) wrote : | # |
Somewhat grudgy LGTM, although I don't like how the tests and
readability are both deteriorating. The output is fine, that's what's
important and both issues above can be fixed in a follow up.
https:/
File cmd/juju/status.go (right):
https:/
cmd/juju/
principal unit names
readability here really could be improved, but i'm not blocking this for
that.
https:/
File cmd/juju/
https:/
cmd/juju/
this is *DEFINITELY WRONG*, yet another copy of exactly the same test
driver function as TestStatusAllFo
in a follow up though, not blocking.
William Reade (fwereade) wrote : | # |
Frank, sorry: rogpeppe got carried away and proposed a branch that
cleans up the structure so much it's worth the duplicated effort IMO...
so NOT LGTM. Your tests live on, though, and are much appreciated.
Preview Diff
1 | === modified file 'cmd/juju/status.go' | |||
2 | --- cmd/juju/status.go 2013-04-16 23:57:53 +0000 | |||
3 | +++ cmd/juju/status.go 2013-04-17 15:27:23 +0000 | |||
4 | @@ -2,6 +2,7 @@ | |||
5 | 2 | 2 | ||
6 | 3 | import ( | 3 | import ( |
7 | 4 | "fmt" | 4 | "fmt" |
8 | 5 | "strings" | ||
9 | 5 | 6 | ||
10 | 6 | "launchpad.net/gnuflag" | 7 | "launchpad.net/gnuflag" |
11 | 7 | "launchpad.net/juju-core/cmd" | 8 | "launchpad.net/juju-core/cmd" |
12 | @@ -178,13 +179,52 @@ | |||
13 | 178 | // processServices gathers information about services. | 179 | // processServices gathers information about services. |
14 | 179 | func processServices(services map[string]*state.Service) (statusMap, error) { | 180 | func processServices(services map[string]*state.Service) (statusMap, error) { |
15 | 180 | servicesMap := make(statusMap) | 181 | servicesMap := make(statusMap) |
18 | 181 | for _, s := range services { | 182 | // prinToSubUnitMap will collect mappings from principal unit names |
19 | 182 | servicesMap[s.Name()] = checkError(processService(s)) | 183 | // to their subordinate units name. |
20 | 184 | prinToSubUnitMap := make(statusMap) | ||
21 | 185 | // subToPrinServicesMap will collect mappings from subordinate service | ||
22 | 186 | // names to a slice of their principal service names. | ||
23 | 187 | subToPrinServicesMap := make(statusMap) | ||
24 | 188 | // 1st pass: iterate over the services. | ||
25 | 189 | for _, service := range services { | ||
26 | 190 | servicesMap[service.Name()] = checkError(processService(service, prinToSubUnitMap, subToPrinServicesMap)) | ||
27 | 191 | } | ||
28 | 192 | // 2nd pass: post-process the subordinates. | ||
29 | 193 | unitsMapByUnitName := func(unitName string) statusMap { | ||
30 | 194 | serviceName := strings.Split(unitName, "/")[0] | ||
31 | 195 | unitsMap := servicesMap[serviceName].(statusMap)["units"] | ||
32 | 196 | return unitsMap.(statusMap) | ||
33 | 197 | } | ||
34 | 198 | // Put the subordinate units data below their principal units. | ||
35 | 199 | for prinUnitName, subUnitNameTmp := range prinToSubUnitMap { | ||
36 | 200 | subUnitName := subUnitNameTmp.(string) | ||
37 | 201 | prinUnitsMap := unitsMapByUnitName(prinUnitName) | ||
38 | 202 | subUnitsMap := unitsMapByUnitName(subUnitName) | ||
39 | 203 | if prinUnitsMap[prinUnitName].(statusMap)["subordinates"] == nil { | ||
40 | 204 | prinUnitsMap[prinUnitName].(statusMap)["subordinates"] = make(statusMap) | ||
41 | 205 | } | ||
42 | 206 | subordinatesMap := prinUnitsMap[prinUnitName].(statusMap)["subordinates"].(statusMap) | ||
43 | 207 | subUnitMap := make(statusMap) | ||
44 | 208 | subUnitMap["agent-state"] = subUnitsMap[subUnitName].(statusMap)["agent-state"] | ||
45 | 209 | if info, ok := subUnitsMap[subUnitName].(statusMap)["agent-state-info"]; ok { | ||
46 | 210 | subUnitMap["agent-state-info"] = info | ||
47 | 211 | } | ||
48 | 212 | subordinatesMap[subUnitName] = subUnitMap | ||
49 | 213 | prinUnitsMap[prinUnitName].(statusMap)["subordinates"] = subordinatesMap | ||
50 | 214 | } | ||
51 | 215 | // Add the principals to the subordinated services. | ||
52 | 216 | for serviceName, subToNames := range subToPrinServicesMap { | ||
53 | 217 | subToSet := set.NewStrings(subToNames.([]string)...) | ||
54 | 218 | subToValues := subToSet.SortedValues() | ||
55 | 219 | servicesMap[serviceName].(statusMap)["subordinate-to"] = subToValues | ||
56 | 220 | // Drop the unit data after it has been merged in the loop above. | ||
57 | 221 | // It isn't needed in the subordinated service anymore. | ||
58 | 222 | delete(servicesMap[serviceName].(statusMap), "units") | ||
59 | 183 | } | 223 | } |
60 | 184 | return servicesMap, nil | 224 | return servicesMap, nil |
61 | 185 | } | 225 | } |
62 | 186 | 226 | ||
64 | 187 | func processService(service *state.Service) (statusMap, error) { | 227 | func processService(service *state.Service, prinToSubUnitMap, subToPrinServicesMap statusMap) (statusMap, error) { |
65 | 188 | serviceMap := make(statusMap) | 228 | serviceMap := make(statusMap) |
66 | 189 | ch, _, err := service.Charm() | 229 | ch, _, err := service.Charm() |
67 | 190 | if err != nil { | 230 | if err != nil { |
68 | @@ -193,14 +233,12 @@ | |||
69 | 193 | serviceMap["charm"] = ch.String() | 233 | serviceMap["charm"] = ch.String() |
70 | 194 | serviceMap["exposed"] = service.IsExposed() | 234 | serviceMap["exposed"] = service.IsExposed() |
71 | 195 | 235 | ||
72 | 196 | // TODO(dfc) service.IsSubordinate() ? | ||
73 | 197 | |||
74 | 198 | units, err := service.AllUnits() | 236 | units, err := service.AllUnits() |
75 | 199 | if err != nil { | 237 | if err != nil { |
76 | 200 | return nil, err | 238 | return nil, err |
77 | 201 | } | 239 | } |
78 | 202 | 240 | ||
80 | 203 | if u := checkError(processUnits(units)); len(u) > 0 { | 241 | if u := checkError(processUnits(units, prinToSubUnitMap, subToPrinServicesMap)); len(u) > 0 { |
81 | 204 | serviceMap["units"] = u | 242 | serviceMap["units"] = u |
82 | 205 | } | 243 | } |
83 | 206 | 244 | ||
84 | @@ -211,15 +249,15 @@ | |||
85 | 211 | return serviceMap, nil | 249 | return serviceMap, nil |
86 | 212 | } | 250 | } |
87 | 213 | 251 | ||
89 | 214 | func processUnits(units []*state.Unit) (statusMap, error) { | 252 | func processUnits(units []*state.Unit, prinToSubUnitMap, subToPrinServicesMap statusMap) (statusMap, error) { |
90 | 215 | unitsMap := make(statusMap) | 253 | unitsMap := make(statusMap) |
91 | 216 | for _, unit := range units { | 254 | for _, unit := range units { |
93 | 217 | unitsMap[unit.Name()] = checkError(processUnit(unit)) | 255 | unitsMap[unit.Name()] = checkError(processUnit(unit, prinToSubUnitMap, subToPrinServicesMap)) |
94 | 218 | } | 256 | } |
95 | 219 | return unitsMap, nil | 257 | return unitsMap, nil |
96 | 220 | } | 258 | } |
97 | 221 | 259 | ||
99 | 222 | func processUnit(unit *state.Unit) (statusMap, error) { | 260 | func processUnit(unit *state.Unit, prinToSubUnitMap, subToPrinServicesMap statusMap) (statusMap, error) { |
100 | 223 | unitMap := make(statusMap) | 261 | unitMap := make(statusMap) |
101 | 224 | 262 | ||
102 | 225 | if addr, ok := unit.PublicAddress(); ok { | 263 | if addr, ok := unit.PublicAddress(); ok { |
103 | @@ -244,6 +282,16 @@ | |||
104 | 244 | } | 282 | } |
105 | 245 | processStatus(unitMap, status, info, agentAlive, unitDead) | 283 | processStatus(unitMap, status, info, agentAlive, unitDead) |
106 | 246 | 284 | ||
107 | 285 | for _, subName := range unit.SubordinateNames() { | ||
108 | 286 | prinToSubUnitMap[unit.Name()] = subName | ||
109 | 287 | subNameParts := strings.Split(subName, "/") | ||
110 | 288 | svcName := subNameParts[0] | ||
111 | 289 | if subToPrinServicesMap[svcName] == nil { | ||
112 | 290 | subToPrinServicesMap[svcName] = []string{} | ||
113 | 291 | } | ||
114 | 292 | subToPrinServicesMap[svcName] = append(subToPrinServicesMap[svcName].([]string), unit.ServiceName()) | ||
115 | 293 | } | ||
116 | 294 | |||
117 | 247 | return unitMap, nil | 295 | return unitMap, nil |
118 | 248 | } | 296 | } |
119 | 249 | 297 | ||
120 | 250 | 298 | ||
121 | === modified file 'cmd/juju/status_test.go' | |||
122 | --- cmd/juju/status_test.go 2013-04-16 07:48:25 +0000 | |||
123 | +++ cmd/juju/status_test.go 2013-04-17 15:27:23 +0000 | |||
124 | @@ -532,6 +532,109 @@ | |||
125 | 532 | ), | 532 | ), |
126 | 533 | } | 533 | } |
127 | 534 | 534 | ||
128 | 535 | var subordinatesTests = []testCase{ | ||
129 | 536 | test( | ||
130 | 537 | "one service with one subordinate service", | ||
131 | 538 | addMachine{"0", state.JobManageEnviron}, | ||
132 | 539 | startAliveMachine{"0"}, | ||
133 | 540 | setMachineStatus{"0", params.StatusStarted, ""}, | ||
134 | 541 | addCharm{"wordpress"}, | ||
135 | 542 | addCharm{"mysql"}, | ||
136 | 543 | addCharm{"logging"}, | ||
137 | 544 | |||
138 | 545 | addService{"wordpress", "wordpress"}, | ||
139 | 546 | setServiceExposed{"wordpress", true}, | ||
140 | 547 | addMachine{"1", state.JobHostUnits}, | ||
141 | 548 | startAliveMachine{"1"}, | ||
142 | 549 | setMachineStatus{"1", params.StatusStarted, ""}, | ||
143 | 550 | addAliveUnit{"wordpress", "1"}, | ||
144 | 551 | setUnitStatus{"wordpress/0", params.StatusStarted, ""}, | ||
145 | 552 | |||
146 | 553 | addService{"mysql", "mysql"}, | ||
147 | 554 | setServiceExposed{"mysql", true}, | ||
148 | 555 | addMachine{"2", state.JobHostUnits}, | ||
149 | 556 | startAliveMachine{"2"}, | ||
150 | 557 | setMachineStatus{"2", params.StatusStarted, ""}, | ||
151 | 558 | addAliveUnit{"mysql", "2"}, | ||
152 | 559 | setUnitStatus{"mysql/0", params.StatusStarted, ""}, | ||
153 | 560 | |||
154 | 561 | addService{"logging", "logging"}, | ||
155 | 562 | setServiceExposed{"logging", true}, | ||
156 | 563 | |||
157 | 564 | relateServices{"wordpress", "mysql"}, | ||
158 | 565 | relateServices{"wordpress", "logging"}, | ||
159 | 566 | relateServices{"mysql", "logging"}, | ||
160 | 567 | |||
161 | 568 | addSubordinate{"wordpress/0", "logging"}, | ||
162 | 569 | addSubordinate{"mysql/0", "logging"}, | ||
163 | 570 | |||
164 | 571 | setUnitsAlive{"logging"}, | ||
165 | 572 | setUnitStatus{"logging/0", params.StatusStarted, ""}, | ||
166 | 573 | setUnitStatus{"logging/1", params.StatusError, "somehow lost in all those logs"}, | ||
167 | 574 | |||
168 | 575 | expect{ | ||
169 | 576 | "multiples related peer units", | ||
170 | 577 | M{ | ||
171 | 578 | "machines": M{ | ||
172 | 579 | "0": machine0, | ||
173 | 580 | "1": machine1, | ||
174 | 581 | "2": machine2, | ||
175 | 582 | }, | ||
176 | 583 | "services": M{ | ||
177 | 584 | "wordpress": M{ | ||
178 | 585 | "charm": "local:series/wordpress-3", | ||
179 | 586 | "exposed": true, | ||
180 | 587 | "units": M{ | ||
181 | 588 | "wordpress/0": M{ | ||
182 | 589 | "machine": "1", | ||
183 | 590 | "agent-state": "started", | ||
184 | 591 | "subordinates": M{ | ||
185 | 592 | "logging/0": M{ | ||
186 | 593 | "agent-state": "started", | ||
187 | 594 | }, | ||
188 | 595 | }, | ||
189 | 596 | }, | ||
190 | 597 | }, | ||
191 | 598 | "relations": M{ | ||
192 | 599 | "db": L{"mysql"}, | ||
193 | 600 | "logging-dir": L{"logging"}, | ||
194 | 601 | }, | ||
195 | 602 | }, | ||
196 | 603 | "mysql": M{ | ||
197 | 604 | "charm": "local:series/mysql-1", | ||
198 | 605 | "exposed": true, | ||
199 | 606 | "units": M{ | ||
200 | 607 | "mysql/0": M{ | ||
201 | 608 | "machine": "2", | ||
202 | 609 | "agent-state": "started", | ||
203 | 610 | "subordinates": M{ | ||
204 | 611 | "logging/1": M{ | ||
205 | 612 | "agent-state": "error", | ||
206 | 613 | "agent-state-info": "somehow lost in all those logs", | ||
207 | 614 | }, | ||
208 | 615 | }, | ||
209 | 616 | }, | ||
210 | 617 | }, | ||
211 | 618 | "relations": M{ | ||
212 | 619 | "server": L{"wordpress"}, | ||
213 | 620 | "juju-info": L{"logging"}, | ||
214 | 621 | }, | ||
215 | 622 | }, | ||
216 | 623 | "logging": M{ | ||
217 | 624 | "charm": "local:series/logging-1", | ||
218 | 625 | "exposed": true, | ||
219 | 626 | "relations": M{ | ||
220 | 627 | "logging-directory": L{"wordpress"}, | ||
221 | 628 | "info": L{"mysql"}, | ||
222 | 629 | }, | ||
223 | 630 | "subordinate-to": L{"mysql", "wordpress"}, | ||
224 | 631 | }, | ||
225 | 632 | }, | ||
226 | 633 | }, | ||
227 | 634 | }, | ||
228 | 635 | ), | ||
229 | 636 | } | ||
230 | 637 | |||
231 | 535 | // TODO(dfc) test failing components by destructively mutating the state under the hood | 638 | // TODO(dfc) test failing components by destructively mutating the state under the hood |
232 | 536 | 639 | ||
233 | 537 | type addMachine struct { | 640 | type addMachine struct { |
234 | @@ -672,6 +775,28 @@ | |||
235 | 672 | ctx.pingers[u.Name()] = pinger | 775 | ctx.pingers[u.Name()] = pinger |
236 | 673 | } | 776 | } |
237 | 674 | 777 | ||
238 | 778 | type setUnitsAlive struct { | ||
239 | 779 | serviceName string | ||
240 | 780 | } | ||
241 | 781 | |||
242 | 782 | func (sua setUnitsAlive) step(c *C, ctx *context) { | ||
243 | 783 | s, err := ctx.st.Service(sua.serviceName) | ||
244 | 784 | c.Assert(err, IsNil) | ||
245 | 785 | us, err := s.AllUnits() | ||
246 | 786 | c.Assert(err, IsNil) | ||
247 | 787 | for _, u := range us { | ||
248 | 788 | pinger, err := u.SetAgentAlive() | ||
249 | 789 | c.Assert(err, IsNil) | ||
250 | 790 | ctx.st.StartSync() | ||
251 | 791 | err = u.WaitAgentAlive(200 * time.Millisecond) | ||
252 | 792 | c.Assert(err, IsNil) | ||
253 | 793 | agentAlive, err := u.AgentAlive() | ||
254 | 794 | c.Assert(err, IsNil) | ||
255 | 795 | c.Assert(agentAlive, Equals, true) | ||
256 | 796 | ctx.pingers[u.Name()] = pinger | ||
257 | 797 | } | ||
258 | 798 | } | ||
259 | 799 | |||
260 | 675 | type setUnitStatus struct { | 800 | type setUnitStatus struct { |
261 | 676 | unitName string | 801 | unitName string |
262 | 677 | status params.Status | 802 | status params.Status |
263 | @@ -707,6 +832,24 @@ | |||
264 | 707 | c.Assert(err, IsNil) | 832 | c.Assert(err, IsNil) |
265 | 708 | } | 833 | } |
266 | 709 | 834 | ||
267 | 835 | type addSubordinate struct { | ||
268 | 836 | prinUnit string | ||
269 | 837 | subService string | ||
270 | 838 | } | ||
271 | 839 | |||
272 | 840 | func (as addSubordinate) step(c *C, ctx *context) { | ||
273 | 841 | u, err := ctx.st.Unit(as.prinUnit) | ||
274 | 842 | c.Assert(err, IsNil) | ||
275 | 843 | eps, err := ctx.st.InferEndpoints([]string{u.ServiceName(), as.subService}) | ||
276 | 844 | c.Assert(err, IsNil) | ||
277 | 845 | rel, err := ctx.st.EndpointsRelation(eps...) | ||
278 | 846 | c.Assert(err, IsNil) | ||
279 | 847 | ru, err := rel.Unit(u) | ||
280 | 848 | c.Assert(err, IsNil) | ||
281 | 849 | err = ru.EnterScope(nil) | ||
282 | 850 | c.Assert(err, IsNil) | ||
283 | 851 | } | ||
284 | 852 | |||
285 | 710 | type expect struct { | 853 | type expect struct { |
286 | 711 | what string | 854 | what string |
287 | 712 | output M | 855 | output M |
288 | @@ -761,3 +904,15 @@ | |||
289 | 761 | }() | 904 | }() |
290 | 762 | } | 905 | } |
291 | 763 | } | 906 | } |
292 | 907 | |||
293 | 908 | func (s *StatusSuite) TestSubordinates(c *C) { | ||
294 | 909 | for i, t := range subordinatesTests { | ||
295 | 910 | c.Log("test %d: %s", i, t.summary) | ||
296 | 911 | func() { | ||
297 | 912 | // Prepare context and run all steps to setup. | ||
298 | 913 | ctx := s.newContext() | ||
299 | 914 | defer s.resetContext(c, ctx) | ||
300 | 915 | ctx.run(c, t.steps) | ||
301 | 916 | }() | ||
302 | 917 | } | ||
303 | 918 | } |
Excellent start, but a couple of tweaks needed. Only the ones that
actually affect output are showstoppers though.
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status. go
File cmd/juju/status.go (left):
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status. go#oldcode183 status. go:183: }
cmd/juju/
This all feels more complex than it needs to be, but if it can support
the changes requested in the other file I'm not too bothered today.
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status. go
File cmd/juju/status.go (right):
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status. go#newcode271 status. go:271: if unit.IsPrincipal() {
cmd/juju/
Not required.
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status_ test.go status_ test.go (right):
File cmd/juju/
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status_ test.go# newcode570 status_ test.go: 570: setUnitStatus{ "logging/ 1", StatusStarted, ""},
cmd/juju/
params.
Maybe just have one of these with a non-"" status?
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status_ test.go# newcode622 status_ test.go: 622: "units": M{
cmd/juju/
logging should not show its own units.
https:/ /codereview. appspot. com/8824043/ diff/1/ cmd/juju/ status_ test.go# newcode845 status_ test.go: 845: func (rsws relateServicesW ithScope) step(c
cmd/juju/
*C, ctx *context) {
I think the relateServices should be separate, and there should be a
separate
type addSubordinate struct {
principal string
subService string
}
...that does roughly the same, but using the specific correct unit
rather than just picking the same one every time.
https:/ /codereview. appspot. com/8824043/