Merge lp:~themue/juju-core/020-cmd-status-subordinates into lp:~juju/juju-core/trunk

[Frank Mueller]

status: added the display of subordinates

Added the display of subordinated units below their
associated units and "subordinate-to" for the
subordinated services.

https://codereview.appspot.com/8824043/

[William Reade (fwereade)]

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
cmd/juju/status.go:183: }
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
cmd/juju/status.go:271: if unit.IsPrincipal() {
Not required.

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

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode570
cmd/juju/status_test.go:570: setUnitStatus{"logging/1",
params.StatusStarted, ""},
Maybe just have one of these with a non-"" status?

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode622
cmd/juju/status_test.go:622: "units": M{
logging should not show its own units.

https://codereview.appspot.com/8824043/diff/1/cmd/juju/status_test.go#newcode845
cmd/juju/status_test.go:845: func (rsws relateServicesWithScope) step(c
*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/

[William Reade (fwereade)]

LGTM wrt output. Code improvements appreciated but not required.

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

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status.go#newcode214
cmd/juju/status.go:214: delete(servicesMap[serviceName].(statusMap),
"units")
I remain suspicious of this, but the output looks sane, we can worry
about this another day.

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status.go#newcode283
cmd/juju/status.go:283: subTo = subToMap[svcName].([]string)
Why are we doing this over and over again for every unit?

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newcode610
cmd/juju/status_test.go:610: "agent-state-info": "somehow lost in all
those logs",
:)

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newcode780
cmd/juju/status_test.go:780: s, err := ctx.st.Service(sua.serviceName)
Do we have anything for testing non-agent-alive units?

https://codereview.appspot.com/8824043/diff/6001/cmd/juju/status_test.go#newcode842
cmd/juju/status_test.go:842: rel, err := ctx.st.AddRelation(eps...)
Please don't do this. Create the relation separately, and get it here
with Relation.

https://codereview.appspot.com/8824043/

[Dimiter Naydenov (dimitern)]

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://codereview.appspot.com/8824043/diff/12001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/8824043/diff/12001/cmd/juju/status.go#newcode182
cmd/juju/status.go:182: // prinToSubUnitMap will collect mappings from
principal unit names
readability here really could be improved, but i'm not blocking this for
that.

https://codereview.appspot.com/8824043/diff/12001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/8824043/diff/12001/cmd/juju/status_test.go#newcode907
cmd/juju/status_test.go:907:
this is *DEFINITELY WRONG*, yet another copy of exactly the same test
driver function as TestStatusAllFormats and TestRelations. I'll fix this
in a follow up though, not blocking.

https://codereview.appspot.com/8824043/

[William Reade (fwereade)]

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.

https://codereview.appspot.com/8824043/

=== modified file 'cmd/juju/status.go'
--- cmd/juju/status.go	2013-04-16 23:57:53 +0000
+++ cmd/juju/status.go	2013-04-17 15:27:23 +0000
@@ -2,6 +2,7 @@
 
 import (
 	"fmt"
+	"strings"
 
 	"launchpad.net/gnuflag"
 	"launchpad.net/juju-core/cmd"
@@ -178,13 +179,52 @@
 // processServices gathers information about services.
 func processServices(services map[string]*state.Service) (statusMap, error) {
 	servicesMap := make(statusMap)
-	for _, s := range services {
-		servicesMap[s.Name()] = checkError(processService(s))
+	// prinToSubUnitMap will collect mappings from principal unit names
+	// to their subordinate units name.
+	prinToSubUnitMap := make(statusMap)
+	// subToPrinServicesMap will collect mappings from subordinate service
+	// names to a slice of their principal service names.
+	subToPrinServicesMap := make(statusMap)
+	// 1st pass: iterate over the services.
+	for _, service := range services {
+		servicesMap[service.Name()] = checkError(processService(service, prinToSubUnitMap, subToPrinServicesMap))
+	}
+	// 2nd pass: post-process the subordinates.
+	unitsMapByUnitName := func(unitName string) statusMap {
+		serviceName := strings.Split(unitName, "/")[0]
+		unitsMap := servicesMap[serviceName].(statusMap)["units"]
+		return unitsMap.(statusMap)
+	}
+	// Put the subordinate units data below their principal units.
+	for prinUnitName, subUnitNameTmp := range prinToSubUnitMap {
+		subUnitName := subUnitNameTmp.(string)
+		prinUnitsMap := unitsMapByUnitName(prinUnitName)
+		subUnitsMap := unitsMapByUnitName(subUnitName)
+		if prinUnitsMap[prinUnitName].(statusMap)["subordinates"] == nil {
+			prinUnitsMap[prinUnitName].(statusMap)["subordinates"] = make(statusMap)
+		}
+		subordinatesMap := prinUnitsMap[prinUnitName].(statusMap)["subordinates"].(statusMap)
+		subUnitMap := make(statusMap)
+		subUnitMap["agent-state"] = subUnitsMap[subUnitName].(statusMap)["agent-state"]
+		if info, ok := subUnitsMap[subUnitName].(statusMap)["agent-state-info"]; ok {
+			subUnitMap["agent-state-info"] = info
+		}
+		subordinatesMap[subUnitName] = subUnitMap
+		prinUnitsMap[prinUnitName].(statusMap)["subordinates"] = subordinatesMap
+	}
+	// Add the principals to the subordinated services.
+	for serviceName, subToNames := range subToPrinServicesMap {
+		subToSet := set.NewStrings(subToNames.([]string)...)
+		subToValues := subToSet.SortedValues()
+		servicesMap[serviceName].(statusMap)["subordinate-to"] = subToValues
+		// Drop the unit data after it has been merged in the loop above.
+		// It isn't needed in the subordinated service anymore.
+		delete(servicesMap[serviceName].(statusMap), "units")
 	}
 	return servicesMap, nil
 }
 
-func processService(service *state.Service) (statusMap, error) {
+func processService(service *state.Service, prinToSubUnitMap, subToPrinServicesMap statusMap) (statusMap, error) {
 	serviceMap := make(statusMap)
 	ch, _, err := service.Charm()
 	if err != nil {
@@ -193,14 +233,12 @@
 	serviceMap["charm"] = ch.String()
 	serviceMap["exposed"] = service.IsExposed()
 
-	// TODO(dfc) service.IsSubordinate() ?
-
 	units, err := service.AllUnits()
 	if err != nil {
 		return nil, err
 	}
 
-	if u := checkError(processUnits(units)); len(u) > 0 {
+	if u := checkError(processUnits(units, prinToSubUnitMap, subToPrinServicesMap)); len(u) > 0 {
 		serviceMap["units"] = u
 	}
 
@@ -211,15 +249,15 @@
 	return serviceMap, nil
 }
 
-func processUnits(units []*state.Unit) (statusMap, error) {
+func processUnits(units []*state.Unit, prinToSubUnitMap, subToPrinServicesMap statusMap) (statusMap, error) {
 	unitsMap := make(statusMap)
 	for _, unit := range units {
-		unitsMap[unit.Name()] = checkError(processUnit(unit))
+		unitsMap[unit.Name()] = checkError(processUnit(unit, prinToSubUnitMap, subToPrinServicesMap))
 	}
 	return unitsMap, nil
 }
 
-func processUnit(unit *state.Unit) (statusMap, error) {
+func processUnit(unit *state.Unit, prinToSubUnitMap, subToPrinServicesMap statusMap) (statusMap, error) {
 	unitMap := make(statusMap)
 
 	if addr, ok := unit.PublicAddress(); ok {
@@ -244,6 +282,16 @@
 	}
 	processStatus(unitMap, status, info, agentAlive, unitDead)
 
+	for _, subName := range unit.SubordinateNames() {
+		prinToSubUnitMap[unit.Name()] = subName
+		subNameParts := strings.Split(subName, "/")
+		svcName := subNameParts[0]
+		if subToPrinServicesMap[svcName] == nil {
+			subToPrinServicesMap[svcName] = []string{}
+		}
+		subToPrinServicesMap[svcName] = append(subToPrinServicesMap[svcName].([]string), unit.ServiceName())
+	}
+
 	return unitMap, nil
 }
 

=== modified file 'cmd/juju/status_test.go'
--- cmd/juju/status_test.go	2013-04-16 07:48:25 +0000
+++ cmd/juju/status_test.go	2013-04-17 15:27:23 +0000
@@ -532,6 +532,109 @@
 	),
 }
 
+var subordinatesTests = []testCase{
+	test(
+		"one service with one subordinate service",
+		addMachine{"0", state.JobManageEnviron},
+		startAliveMachine{"0"},
+		setMachineStatus{"0", params.StatusStarted, ""},
+		addCharm{"wordpress"},
+		addCharm{"mysql"},
+		addCharm{"logging"},
+
+		addService{"wordpress", "wordpress"},
+		setServiceExposed{"wordpress", true},
+		addMachine{"1", state.JobHostUnits},
+		startAliveMachine{"1"},
+		setMachineStatus{"1", params.StatusStarted, ""},
+		addAliveUnit{"wordpress", "1"},
+		setUnitStatus{"wordpress/0", params.StatusStarted, ""},
+
+		addService{"mysql", "mysql"},
+		setServiceExposed{"mysql", true},
+		addMachine{"2", state.JobHostUnits},
+		startAliveMachine{"2"},
+		setMachineStatus{"2", params.StatusStarted, ""},
+		addAliveUnit{"mysql", "2"},
+		setUnitStatus{"mysql/0", params.StatusStarted, ""},
+
+		addService{"logging", "logging"},
+		setServiceExposed{"logging", true},
+
+		relateServices{"wordpress", "mysql"},
+		relateServices{"wordpress", "logging"},
+		relateServices{"mysql", "logging"},
+
+		addSubordinate{"wordpress/0", "logging"},
+		addSubordinate{"mysql/0", "logging"},
+
+		setUnitsAlive{"logging"},
+		setUnitStatus{"logging/0", params.StatusStarted, ""},
+		setUnitStatus{"logging/1", params.StatusError, "somehow lost in all those logs"},
+
+		expect{
+			"multiples related peer units",
+			M{
+				"machines": M{
+					"0": machine0,
+					"1": machine1,
+					"2": machine2,
+				},
+				"services": M{
+					"wordpress": M{
+						"charm":   "local:series/wordpress-3",
+						"exposed": true,
+						"units": M{
+							"wordpress/0": M{
+								"machine":     "1",
+								"agent-state": "started",
+								"subordinates": M{
+									"logging/0": M{
+										"agent-state": "started",
+									},
+								},
+							},
+						},
+						"relations": M{
+							"db":          L{"mysql"},
+							"logging-dir": L{"logging"},
+						},
+					},
+					"mysql": M{
+						"charm":   "local:series/mysql-1",
+						"exposed": true,
+						"units": M{
+							"mysql/0": M{
+								"machine":     "2",
+								"agent-state": "started",
+								"subordinates": M{
+									"logging/1": M{
+										"agent-state":      "error",
+										"agent-state-info": "somehow lost in all those logs",
+									},
+								},
+							},
+						},
+						"relations": M{
+							"server":    L{"wordpress"},
+							"juju-info": L{"logging"},
+						},
+					},
+					"logging": M{
+						"charm":   "local:series/logging-1",
+						"exposed": true,
+						"relations": M{
+							"logging-directory": L{"wordpress"},
+							"info":              L{"mysql"},
+						},
+						"subordinate-to": L{"mysql", "wordpress"},
+					},
+				},
+			},
+		},
+	),
+}
+
 // TODO(dfc) test failing components by destructively mutating the state under the hood
 
 type addMachine struct {
@@ -672,6 +775,28 @@
 	ctx.pingers[u.Name()] = pinger
 }
 
+type setUnitsAlive struct {
+	serviceName string
+}
+
+func (sua setUnitsAlive) step(c *C, ctx *context) {
+	s, err := ctx.st.Service(sua.serviceName)
+	c.Assert(err, IsNil)
+	us, err := s.AllUnits()
+	c.Assert(err, IsNil)
+	for _, u := range us {
+		pinger, err := u.SetAgentAlive()
+		c.Assert(err, IsNil)
+		ctx.st.StartSync()
+		err = u.WaitAgentAlive(200 * time.Millisecond)
+		c.Assert(err, IsNil)
+		agentAlive, err := u.AgentAlive()
+		c.Assert(err, IsNil)
+		c.Assert(agentAlive, Equals, true)
+		ctx.pingers[u.Name()] = pinger
+	}
+}
+
 type setUnitStatus struct {
 	unitName   string
 	status     params.Status
@@ -707,6 +832,24 @@
 	c.Assert(err, IsNil)
 }
 
+type addSubordinate struct {
+	prinUnit   string
+	subService string
+}
+
+func (as addSubordinate) step(c *C, ctx *context) {
+	u, err := ctx.st.Unit(as.prinUnit)
+	c.Assert(err, IsNil)
+	eps, err := ctx.st.InferEndpoints([]string{u.ServiceName(), as.subService})
+	c.Assert(err, IsNil)
+	rel, err := ctx.st.EndpointsRelation(eps...)
+	c.Assert(err, IsNil)
+	ru, err := rel.Unit(u)
+	c.Assert(err, IsNil)
+	err = ru.EnterScope(nil)
+	c.Assert(err, IsNil)
+}
+
 type expect struct {
 	what   string
 	output M
@@ -761,3 +904,15 @@
 		}()
 	}
 }
+
+func (s *StatusSuite) TestSubordinates(c *C) {
+	for i, t := range subordinatesTests {
+		c.Log("test %d: %s", i, t.summary)
+		func() {
+			// Prepare context and run all steps to setup.
+			ctx := s.newContext()
+			defer s.resetContext(c, ctx)
+			ctx.run(c, t.steps)
+		}()
+	}
+}