Merge lp:~dave-cheney/juju-core/089-state-unit-assigned-machine into lp:~juju/juju-core/trunk

[Dave Cheney]

state: unit: AssignedMachine returns a machine

This proposal alters AssignedMachine (and it's signature) to return a
*state.Machine rather than an id. All the non test consumers of this
method are actually after a *Machine, not the id anyway, so this saves them
the call to state.Machine(). The test code was altered for the new signature
but is operation is unaffected.

https://codereview.appspot.com/6499071/

[Gustavo Niemeyer (niemeyer)]

The CL change doesn't seem to reflect the CL description.

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

https://codereview.appspot.com/6499071/diff/1/cmd/juju/status.go#newcode212
cmd/juju/status.go:212: if m, err := unit.AssignedMachine(); err == nil
{
This seems to contradict the CL description. We used to have a machine
id, and now we will fetch a machine value from the database once more
for every single unit we have.

https://codereview.appspot.com/6499071/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6499071/diff/1/worker/firewaller/firewaller.go#newcode113
worker/firewaller/firewaller.go:113: if fw.machineds[machine.Id()] ==
nil {
Once again, real code that cares only about the id is now fetching the
machine just to pick its id.

https://codereview.appspot.com/6499071/

[Roger Peppe (rogpeppe)]

I'm +1 on this CL. It's easy to get a machine id from a Machine, but
getting a Machine from an id requires an additional round trip.

https://codereview.appspot.com/6499071/

[Gustavo Niemeyer (niemeyer)]

On 2012/09/06 12:04:41, rog wrote:
> I'm +1 on this CL. It's easy to get a machine id from a Machine, but
getting a
> Machine from an id requires an additional round trip.

Thus we should *always* be paying for the additional roundtrip to the
database, even when the two only non-test locations involved don't
actually want anything but the machine id? I don't understand.

This seems like a totally straightforward decision, btw. It concerns me
a bit that we're even discussing it. I'll soon be stepping down as core
reviewer and will not be overseeing all such changes anymore.

https://codereview.appspot.com/6499071/

Unmerged revisions

457. By Dave Cheney on 2012-09-04

AssignedMachine

456. By Roger Peppe on 2012-09-03

juju: always connect to State.

All uses of juju.Conn other than Bootstrap and Destroy require a state
connection, so make State an exported field of Conn and connect to it
when creating the Conn.

The few places that call Bootstrap can easily call Environ.Bootstrap
instead.

Also add environs.NewFromName to make it easier to open an
environment, rename juju.NewConn to juju.NewConnFromName, and add
juju.NewConnFromEnviron to join the dots.

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6488077

455. By Roger Peppe on 2012-08-31

worker: new package to factor out some common code from workers

Also simplify workers a little.

R=niemeyer
CC=
https://codereview.appspot.com/6501072

454. By Roger Peppe on 2012-08-31

cmd/jujud: fix race in test

We should not call OpenPort until the new instance id has
hit the state.

We also fix a bug in firewaller where it called InstanceId even
when it needed to do nothing, and take the opportunity
to make the firewaller tests a little more concise now that we
have JujuConnSuite.

R=TheMue, dfc
CC=
https://codereview.appspot.com/6499056

453. By Roger Peppe on 2012-08-31

environs: add BootstrapConfig

Also add flags to environs.FindTools and environs.BestTools,
LoggingSuite to JujuSuite, and make the Config in cloudinit
a *config.Config.

R=niemeyer, dfc
CC=
https://codereview.appspot.com/6500052

452. By Frank Mueller on 2012-08-30

state: changed environ config to be config.Config

state.EnvironConfig() and the according watcher now
return environs/config.Config. Additionally the new
function state.SetEnvironConfig() replaces the current
configuration. Depending code - mostly tests - has
been changed.

R=niemeyer
CC=
https://codereview.appspot.com/6493055

451. By William Reade on 2012-08-30

add charm.GitDir

Hopefully uncontroversial; implementing this should help me focus on the
actual charm.Manager implementation more easily.

R=niemeyer
CC=
https://codereview.appspot.com/6493059

450. By Roger Peppe on 2012-08-30

environs/config: add support for agent version.

Also adjust documentation for schema.Omit slightly
and add a test.

R=niemeyer
CC=
https://codereview.appspot.com/6499055

449. By Roger Peppe on 2012-08-30

environs: change BestTools so it does not choose later versions

As discussed on-line, when upgrading we want to
choose the latest version with number <= the proposed
version.

R=niemeyer
CC=
https://codereview.appspot.com/6495054

448. By Dave Cheney on 2012-08-30

environs/dummy: fix build

I thought I ran tests before submitting, but clearly I did not.

R=
CC=
https://codereview.appspot.com/6501063

=== modified file 'cmd/juju/status.go'
--- cmd/juju/status.go	2012-09-03 17:35:44 +0000
+++ cmd/juju/status.go	2012-09-04 06:33:21 +0000
@@ -209,9 +209,9 @@
 		r["public-address"] = addr
 	}
 
-	if id, err := unit.AssignedMachineId(); err == nil {
+	if m, err := unit.AssignedMachine(); err == nil {
 		// TODO(dfc) we could make this nicer, ie machine/0
-		r["machine"] = id
+		r["machine"] = m.Id()
 	}
 
 	processVersion(r, unit)

=== modified file 'cmd/jujud/provisioning_test.go'
--- cmd/jujud/provisioning_test.go	2012-08-30 14:50:43 +0000
+++ cmd/jujud/provisioning_test.go	2012-09-04 06:33:21 +0000
@@ -59,9 +59,7 @@
 	c.Check(opRecvTimeout(c, op, dummy.OpStartInstance{}), NotNil)
 
 	// Wait for the instance id to show up in the state.
-	id, err := units[0].AssignedMachineId()
-	c.Assert(err, IsNil)
-	m, err := s.State.Machine(id)
+	m, err := units[0].AssignedMachine()
 	c.Assert(err, IsNil)
 	w := m.Watch()
 	for _ = range w.Changes() {

=== modified file 'juju/deploy_test.go'
--- juju/deploy_test.go	2012-09-03 17:35:44 +0000
+++ juju/deploy_test.go	2012-09-04 06:33:21 +0000
@@ -172,9 +172,9 @@
 	c.Assert(err, IsNil)
 	c.Assert(units, HasLen, 2)
 
-	id0, err := units[0].AssignedMachineId()
-	c.Assert(err, IsNil)
-	id1, err := units[1].AssignedMachineId()
-	c.Assert(err, IsNil)
-	c.Assert(id0, Not(Equals), id1)
+	m0, err := units[0].AssignedMachine()
+	c.Assert(err, IsNil)
+	m1, err := units[1].AssignedMachine()
+	c.Assert(err, IsNil)
+	c.Assert(m0.Id(), Not(Equals), m1.Id())
 }

=== modified file 'state/assign_test.go'
--- state/assign_test.go	2012-08-15 11:51:56 +0000
+++ state/assign_test.go	2012-09-04 06:33:21 +0000
@@ -39,7 +39,7 @@
 	c.Assert(err, IsNil)
 
 	// Check that the unit has no machine assigned.
-	_, err = s.unit.AssignedMachineId()
+	_, err = s.unit.AssignedMachine()
 	c.Assert(err, ErrorMatches, `cannot get machine id of unit "wordpress/0": unit not assigned to machine`)
 }
 
@@ -63,9 +63,9 @@
 	err = s.unit.AssignToMachine(machineTwo)
 	c.Assert(err, ErrorMatches, `cannot assign unit "wordpress/0" to machine 2: unit already assigned to machine 1`)
 
-	machineId, err := s.unit.AssignedMachineId()
+	machine, err := s.unit.AssignedMachine()
 	c.Assert(err, IsNil)
-	c.Assert(machineId, Equals, 1)
+	c.Assert(machine.Id(), Equals, 1)
 }
 
 func (s *AssignSuite) TestUnassignUnitFromMachineWithChangingState(c *C) {
@@ -76,7 +76,7 @@
 
 	err = s.unit.UnassignFromMachine()
 	c.Assert(err, ErrorMatches, `cannot unassign unit "wordpress/0" from machine: environment state has changed`)
-	_, err = s.unit.AssignedMachineId()
+	_, err = s.unit.AssignedMachine()
 	c.Assert(err, ErrorMatches, `cannot get machine id of unit "wordpress/0": environment state has changed`)
 
 	err = s.State.RemoveService(s.service)
@@ -84,7 +84,7 @@
 
 	err = s.unit.UnassignFromMachine()
 	c.Assert(err, ErrorMatches, `cannot unassign unit "wordpress/0" from machine: environment state has changed`)
-	_, err = s.unit.AssignedMachineId()
+	_, err = s.unit.AssignedMachine()
 	c.Assert(err, ErrorMatches, `cannot get machine id of unit "wordpress/0": environment state has changed`)
 }
 
@@ -164,19 +164,19 @@
 	err = s.unit.AssignToMachine(m1)
 	c.Assert(err, IsNil)
 
-	id, err := log1Unit.AssignedMachineId()
+	m, err := log1Unit.AssignedMachine()
 	c.Assert(err, IsNil)
-	c.Check(id, Equals, m1.Id())
-	id, err = log2Unit.AssignedMachineId()
-	c.Check(id, Equals, m1.Id())
+	c.Check(m.Id(), Equals, m1.Id())
+	m, err = log2Unit.AssignedMachine()
+	c.Check(m.Id(), Equals, m1.Id())
 
 	// Check that unassigning the principal unassigns the
 	// subordinates too.
 	err = s.unit.UnassignFromMachine()
 	c.Assert(err, IsNil)
-	_, err = log1Unit.AssignedMachineId()
+	_, err = log1Unit.AssignedMachine()
 	c.Assert(err, ErrorMatches, `cannot get machine id of unit "logging1/0": unit not assigned to machine`)
-	_, err = log2Unit.AssignedMachineId()
+	_, err = log2Unit.AssignedMachine()
 	c.Assert(err, ErrorMatches, `cannot get machine id of unit "logging2/0": unit not assigned to machine`)
 }
 
@@ -184,16 +184,16 @@
 	// Check nonsensical policy
 	fail := func() { s.State.AssignUnit(s.unit, state.AssignmentPolicy("random")) }
 	c.Assert(fail, PanicMatches, `unknown unit assignment policy: "random"`)
-	_, err := s.unit.AssignedMachineId()
+	_, err := s.unit.AssignedMachine()
 	c.Assert(err, NotNil)
 	assertMachineCount(c, s.State, 1)
 
 	// Check local placement
 	err = s.State.AssignUnit(s.unit, state.AssignLocal)
 	c.Assert(err, IsNil)
-	mid, err := s.unit.AssignedMachineId()
+	m, err := s.unit.AssignedMachine()
 	c.Assert(err, IsNil)
-	c.Assert(mid, Equals, 0)
+	c.Assert(m.Id(), Equals, 0)
 	assertMachineCount(c, s.State, 1)
 
 	// Check unassigned placement with no unused machines
@@ -201,9 +201,9 @@
 	c.Assert(err, IsNil)
 	err = s.State.AssignUnit(unit1, state.AssignUnused)
 	c.Assert(err, IsNil)
-	mid, err = unit1.AssignedMachineId()
+	m, err = unit1.AssignedMachine()
 	c.Assert(err, IsNil)
-	c.Assert(mid, Equals, 1)
+	c.Assert(m.Id(), Equals, 1)
 	assertMachineCount(c, s.State, 2)
 
 	// Check unassigned placement on an unused machine
@@ -212,9 +212,9 @@
 	c.Assert(err, IsNil)
 	err = s.State.AssignUnit(unit2, state.AssignUnused)
 	c.Assert(err, IsNil)
-	mid, err = unit2.AssignedMachineId()
+	m, err = unit2.AssignedMachine()
 	c.Assert(err, IsNil)
-	c.Assert(mid, Equals, 2)
+	c.Assert(m.Id(), Equals, 2)
 	assertMachineCount(c, s.State, 3)
 
 	// Check cannot assign subordinates to machines

=== modified file 'state/service_test.go'
--- state/service_test.go	2012-08-21 15:39:59 +0000
+++ state/service_test.go	2012-09-04 06:33:21 +0000
@@ -105,9 +105,9 @@
 	c.Assert(principal, Equals, false)
 
 	// Check the subordinate unit has been assigned its principal's machine.
-	id, err := subZero.AssignedMachineId()
+	m, err = subZero.AssignedMachine()
 	c.Assert(err, IsNil)
-	c.Assert(id, Equals, m.Id())
+	c.Assert(m.Id(), Equals, m.Id())
 
 	// Check that subordinate units must be added to other units.
 	_, err = logging.AddUnit()

=== modified file 'state/unit.go'
--- state/unit.go	2012-08-21 15:39:59 +0000
+++ state/unit.go	2012-09-04 06:33:21 +0000
@@ -233,21 +233,21 @@
 	return u.principalKey == ""
 }
 
-// AssignedMachineId returns the id of the assigned machine.
-func (u *Unit) AssignedMachineId() (id int, err error) {
+// AssignedMachine returns the machine assigned to this unit.
+func (u *Unit) AssignedMachine() (machine *Machine, err error) {
 	defer trivial.ErrorContextf(&err, "cannot get machine id of unit %q", u)
 	topology, err := readTopology(u.st.zk)
 	if err != nil {
-		return 0, err
+		return nil, err
 	}
 	if !topology.HasUnit(u.key) {
-		return 0, stateChanged
+		return nil, stateChanged
 	}
 	machineKey, err := topology.UnitMachineKey(u.key)
 	if err != nil {
-		return 0, err
+		return nil, err
 	}
-	return keySeq(machineKey), nil
+	return u.st.Machine(keySeq(machineKey))
 }
 
 // AssignToMachine assigns this unit to a given machine.

=== modified file 'worker/firewaller/firewaller.go'
--- worker/firewaller/firewaller.go	2012-08-31 16:14:09 +0000
+++ worker/firewaller/firewaller.go	2012-09-04 06:33:21 +0000
@@ -106,14 +106,14 @@
 			for _, unit := range change.Added {
 				unitd := newUnitData(unit, fw)
 				fw.unitds[unit.Name()] = unitd
-				machineId, err := unit.AssignedMachineId()
+				machine, err := unit.AssignedMachine()
 				if err != nil {
 					fw.tomb.Kill(err)
 				}
-				if fw.machineds[machineId] == nil {
+				if fw.machineds[machine.Id()] == nil {
 					panic("machine of added unit is not watched")
 				}
-				unitd.machined = fw.machineds[machineId]
+				unitd.machined = fw.machineds[machine.Id()]
 				unitd.machined.unitds[unit.Name()] = unitd
 				if fw.serviceds[unit.ServiceName()] == nil {
 					service, err := fw.st.Service(unit.ServiceName())

=== modified file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	2012-08-31 05:41:43 +0000
+++ worker/firewaller/firewaller_test.go	2012-09-04 06:33:21 +0000
@@ -64,9 +64,7 @@
 	units, err := s.Conn.AddUnits(svc, 1)
 	c.Assert(err, IsNil)
 	u := units[0]
-	id, err := u.AssignedMachineId()
-	c.Assert(err, IsNil)
-	m, err := s.State.Machine(id)
+	m, err := u.AssignedMachine()
 	c.Assert(err, IsNil)
 	return u, m
 }