Merge lp:~dave-cheney/juju-core/go-cmd-jujud-provisioner-unknown-machines into lp:~juju/juju-core/trunk

[Dave Cheney]

cmd/jujud: provisioner shuts down unknown instance

This proposal adds the facility for the PA to detect lost or
unknown instances running in the environment that are not
associated with a machine in the state and shut them down.

https://codereview.appspot.com/6315043/

[Gustavo Niemeyer (niemeyer)]

LGTM, with a couple of minors.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#newcode110
cmd/jujud/provisioning.go:110: p.tomb.Kill(err)
return?

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#newcode197
cmd/jujud/provisioning.go:197: machines, err := p.st.AllMachines()
These All* methods aren't a great API. We can do that for now, but we'll
need something better soon. Think about 100k machines running. This is
fetching 200k items from the DB regularly, without much benefit in most
cases.

Can you please add a note here and file a bug in Launchpad against
juju-core?

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#newcode206
cmd/jujud/provisioning.go:206: if id == "" {
Please add the same TODO we have elsewhere. InstanceId should really not
be returning an empty id.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning_test.go#newcode392
cmd/jujud/provisioning_test.go:392: // where the final machine has been
removed from the state which the PA was
"removed from the state which the PA was down"?

https://codereview.appspot.com/6315043/

[Gustavo Niemeyer (niemeyer)]

LGTM, with a couple of trivials.

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/6001/cmd/jujud/provisioning.go#newcode206
cmd/jujud/provisioning.go:206: if id == "" {
On 2012/06/19 05:19:17, dfc wrote:
> On 2012/06/19 01:53:28, niemeyer wrote:
> > Please add the same TODO we have elsewhere. InstanceId should really
not be
> > returning an empty id.

> Done.

Not done?

https://codereview.appspot.com/6315043/diff/10001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6315043/diff/10001/cmd/jujud/provisioning.go#newcode104
cmd/jujud/provisioning.go:104: p.tomb.Kill(err)
return

https://codereview.appspot.com/6315043/

[Gustavo Niemeyer (niemeyer)]

On 2012/06/20 12:25:53, dfc wrote:
> Please take a look.

The fix looks good, Dave, but this will need a different branch and a
different CL. This one was already submitted.

Feel free to propose+submit it directly with the specific fix.

https://codereview.appspot.com/6315043/

=== modified file 'cmd/jujud/provisioning.go'
--- cmd/jujud/provisioning.go	2012-06-18 22:55:56 +0000
+++ cmd/jujud/provisioning.go	2012-06-19 05:20:25 +0000
@@ -98,6 +98,12 @@
 				continue
 			}
 			log.Printf("provisioner loaded new environment configuration")
+
+			// call processMachines to stop any unknown instances before watching machines.
+			if err := p.processMachines(nil, nil); err != nil {
+				p.tomb.Kill(err)
+			}
+
 			p.innerLoop()
 		}
 	}
@@ -132,7 +138,9 @@
 				}
 				return
 			}
-			if err := p.processMachines(machines); err != nil {
+			// TODO(dfc) fire process machines periodically to shut down unknown
+			// instances.
+			if err := p.processMachines(machines.Added, machines.Deleted); err != nil {
 				p.tomb.Kill(err)
 			}
 		}
@@ -151,29 +159,62 @@
 	return p.tomb.Wait()
 }
 
-func (p *Provisioner) processMachines(changes *state.MachinesChange) error {
+func (p *Provisioner) processMachines(added, removed []*state.Machine) error {
 	// step 1. find which of the added machines have not
 	// yet been allocated a started instance.
-	notstarted, err := p.findNotStarted(changes.Added)
+	notstarted, err := p.findNotStarted(added)
 	if err != nil {
 		return err
 	}
 
 	// step 2. start an instance for any machines we found.
-	if _, err := p.startMachines(notstarted); err != nil {
+	if err := p.startMachines(notstarted); err != nil {
 		return err
 	}
 
 	// step 3. stop all machines that were removed from the state.
-	stopping, err := p.instancesForMachines(changes.Deleted)
+	stopping, err := p.instancesForMachines(removed)
 	if err != nil {
 		return err
 	}
 
-	// TODO(dfc) obtain a list of started instances from the Environ and compare that
-	// with the known instances stored in the machine.InstanceId() config.
-
-	return p.stopInstances(stopping)
+	// step 4. find instances which are running but have no machine 
+	// associated with them.
+	unknown, err := p.findUnknownInstances()
+
+	return p.stopInstances(append(stopping, unknown...))
+}
+
+// findUnknownInstances finds instances which are not associated with a machine.
+func (p *Provisioner) findUnknownInstances() ([]environs.Instance, error) {
+	all, err := p.environ.AllInstances()
+	if err != nil {
+		return nil, err
+	}
+	instances := make(map[string]environs.Instance)
+	for _, i := range all {
+		instances[i.Id()] = i
+	}
+	// TODO(dfc) this is very inefficient, p.machines cache may help.
+	machines, err := p.st.AllMachines()
+	if err != nil {
+		return nil, err
+	}
+	for _, m := range machines {
+		id, err := m.InstanceId()
+		if err != nil {
+			return nil, err
+		}
+		if id == "" {
+			continue
+		}
+		delete(instances, id)
+	}
+	var unknown []environs.Instance
+	for _, i := range instances {
+		unknown = append(unknown, i)
+	}
+	return unknown, nil
 }
 
 // findNotStarted finds machines without an InstanceId set, these are defined as not started.
@@ -193,15 +234,13 @@
 	return notstarted, nil
 }
 
-func (p *Provisioner) startMachines(machines []*state.Machine) ([]*state.Machine, error) {
-	var started []*state.Machine
+func (p *Provisioner) startMachines(machines []*state.Machine) error {
 	for _, m := range machines {
 		if err := p.startMachine(m); err != nil {
-			return nil, err
+			return err
 		}
-		started = append(started, m)
 	}
-	return started, nil
+	return nil
 }
 
 func (p *Provisioner) startMachine(m *state.Machine) error {
@@ -228,7 +267,7 @@
 }
 
 func (p *Provisioner) stopInstances(instances []environs.Instance) error {
-	// although calling StopInstance with an empty slice should produce no change in the 
+	// Although calling StopInstance with an empty slice should produce no change in the 
 	// provider, environs like dummy do not consider this a noop.
 	if len(instances) == 0 {
 		return nil
@@ -272,6 +311,8 @@
 	return inst, nil
 }
 
+// instancesForMachines returns a list of environs.Instance that represent the list of machines running
+// in the provider.
 func (p *Provisioner) instancesForMachines(machines []*state.Machine) ([]environs.Instance, error) {
 	var insts []environs.Instance
 	for _, m := range machines {

=== modified file 'cmd/jujud/provisioning_test.go'
--- cmd/jujud/provisioning_test.go	2012-06-19 01:31:04 +0000
+++ cmd/jujud/provisioning_test.go	2012-06-19 05:20:25 +0000
@@ -350,6 +350,82 @@
 	c.Assert(p.Stop(), IsNil)
 }
 
+func (s *ProvisioningSuite) TestProvisioningStopsUnknownInstances(c *C) {
+	p, err := NewProvisioner(s.zkInfo)
+	c.Check(err, IsNil)
+	// we are not using defer s.stopProvisioner(c, p) because we need to control when 
+	// the PA is restarted in this test. Methods like Fatalf and Assert should not be used.
+	op := make(chan dummy.Operation, 1)
+	dummy.Listen(op)
+
+	// create a machine
+	m, err := s.st.AddMachine()
+	c.Check(err, IsNil)
+
+	s.checkStartInstance(c, op)
+	s.checkMachineIdSet(c, m)
+
+	// create a second machine
+	m, err = s.st.AddMachine()
+	c.Check(err, IsNil)
+
+	s.checkStartInstance(c, op)
+	s.checkMachineIdSet(c, m)
+
+	// stop the PA
+	c.Check(p.Stop(), IsNil)
+
+	// remove the machine
+	err = s.st.RemoveMachine(m.Id())
+	c.Check(err, IsNil)
+
+	// start a new provisioner
+	p, err = NewProvisioner(s.zkInfo)
+	c.Check(err, IsNil)
+
+	s.checkStopInstance(c, op)
+
+	c.Assert(p.Stop(), IsNil)
+}
+
+// This check is different from the one above as it catches the edge case
+// where the final machine has been removed from the state while the PA was 
+// not running. 
+func (s *ProvisioningSuite) TestProvisioningStopsOnlyUnknownInstances(c *C) {
+	p, err := NewProvisioner(s.zkInfo)
+	c.Check(err, IsNil)
+	// we are not using defer s.stopProvisioner(c, p) because we need to control when 
+	// the PA is restarted in this test. Methods like Fatalf and Assert should not be used.
+	op := make(chan dummy.Operation, 1)
+	dummy.Listen(op)
+
+	// create a machine
+	m, err := s.st.AddMachine()
+	c.Check(err, IsNil)
+
+	s.checkStartInstance(c, op)
+	s.checkMachineIdSet(c, m)
+
+	// stop the PA
+	c.Check(p.Stop(), IsNil)
+
+	// remove the machine
+	err = s.st.RemoveMachine(m.Id())
+	c.Check(err, IsNil)
+
+	machines, err := s.st.AllMachines()
+	c.Check(err, IsNil)
+	c.Check(len(machines), Equals, 0) // it's really gone	
+
+	// start a new provisioner
+	p, err = NewProvisioner(s.zkInfo)
+	c.Check(err, IsNil)
+
+	s.checkStopInstance(c, op)
+
+	c.Assert(p.Stop(), IsNil)
+}
+
 func (s *ProvisioningSuite) TestProvisioningRecoversAfterInvalidEnvironmentPublished(c *C) {
 	p, err := NewProvisioner(s.zkInfo)
 	c.Assert(err, IsNil)