Merge pull request juju#6321 from perrito666/fix_1604965

Fix 1604965: machine stays in pending state even though node has been marked failed deployment in MAAS

Maas provider was missing an error state by the maas controller, it is now handled.
Additionally juju machine status is updated upon failure.

### QA Steps
* Deploy a maas controller
* tamper with the machines power profiles
* deploy a charm
* within moments you will see a failed deployment and the machine will be down.

Author: jujubot

apiserver/instancepoller/instancepoller.go

@@ -212,6 +212,12 @@ func (a *InstancePollerAPI) SetInstanceStatus(args params.SetStatus) (params.Err
 				Since:   &now,
 			}
 			err = machine.SetInstanceStatus(s)
+			if status.Status(arg.Status) == status.ProvisioningError {
+				s.Status = status.Error
+				if err == nil {
+					err = machine.SetStatus(s)
+				}
+			}
 		}
 		result.Results[i].Error = common.ServerError(err)
 	}

apiserver/instancepoller/state.go

@@ -20,6 +20,7 @@ type StateMachine interface {
 	SetProviderAddresses(...network.Address) error
 	InstanceStatus() (status.StatusInfo, error)
 	SetInstanceStatus(status.StatusInfo) error
+	SetStatus(status.StatusInfo) error
 	String() string
 	Refresh() error
 	Life() state.Life

apiserver/provisioner/provisioner.go

@@ -809,6 +809,12 @@ func (p *ProvisionerAPI) SetInstanceStatus(args params.SetStatus) (params.ErrorR
 				Since:   &now,
 			}
 			err = machine.SetInstanceStatus(s)
+			if status.Status(arg.Status) == status.ProvisioningError {
+				s.Status = status.Error
+				if err == nil {
+					err = machine.SetStatus(s)
+				}
+			}
 		}
 		result.Results[i].Error = common.ServerError(err)
 	}

provider/maas/environ.go

@@ -54,6 +54,8 @@ var shortAttempt = utils.AttemptStrategy{
 	Delay: 200 * time.Millisecond,
 }
 
+const statusPollInterval = 5 * time.Second
+
 var (
 	ReleaseNodes         = releaseNodes
 	DeploymentStatusCall = deploymentStatusCall
@@ -948,6 +950,7 @@ func (environ *maasEnviron) StartInstance(args environs.StartInstanceParams) (
 			return nil, errors.Annotatef(err, "cannot run instances")
 		}
 	}
+
 	defer func() {
 		if err != nil {
 			if err := environ.StopInstances(inst.Id()); err != nil {

provider/maas/instance.go

@@ -51,20 +51,24 @@ func maasObjectId(maasObject *gomaasapi.MAASObject) instance.Id {
 	return instance.Id(maasObject.URI().String())
 }
 
+func normalizeStatus(statusMsg string) string {
+	return strings.ToLower(strings.TrimSpace(statusMsg))
+}
+
 func convertInstanceStatus(statusMsg, substatus string, id instance.Id) instance.InstanceStatus {
 	maasInstanceStatus := status.Empty
-	switch statusMsg {
+	switch normalizeStatus(statusMsg) {
 	case "":
 		logger.Debugf("unable to obtain status of instance %s", id)
 		statusMsg = "error in getting status"
-	case "Deployed":
+	case "deployed":
 		maasInstanceStatus = status.Running
-	case "Deploying":
+	case "deploying":
 		maasInstanceStatus = status.Allocating
 		if substatus != "" {
 			statusMsg = fmt.Sprintf("%s: %s", statusMsg, substatus)
 		}
-	case "Failed Deployment":
+	case "failed deployment":
 		maasInstanceStatus = status.ProvisioningError
 		if substatus != "" {
 			statusMsg = fmt.Sprintf("%s: %s", statusMsg, substatus)

provider/maas/interfaces_test.go

@@ -923,8 +923,8 @@ func (s *interfacesSuite) TestMAAS2NetworkInterfaces(c *gc.C) {
 		MTU:                 1500,
 		GatewayAddress:      newAddressOnSpaceWithId("storage", network.Id("3"), "10.250.19.2"),
 	}}
-
-	instance := &maas2Instance{machine: &fakeMachine{interfaceSet: exampleInterfaces}}
+	machine := &fakeMachine{interfaceSet: exampleInterfaces}
+	instance := &maas2Instance{machine: machine}
 
 	infos, err := maas2NetworkInterfaces(instance, subnetsMap)
 	c.Assert(err, jc.ErrorIsNil)
@@ -965,8 +965,8 @@ func (s *interfacesSuite) TestMAAS2InterfacesNilVLAN(c *gc.C) {
 			children: []string{"eth0.100", "eth0.250", "eth0.50"},
 		},
 	}
-
-	instance := &maas2Instance{machine: &fakeMachine{interfaceSet: exampleInterfaces}}
+	machine := &fakeMachine{interfaceSet: exampleInterfaces}
+	instance := &maas2Instance{machine: machine}
 
 	expected := []network.InterfaceInfo{{
 		DeviceIndex:       0,

provider/maas/maas2_test.go

@@ -184,6 +184,14 @@ func (c *fakeController) ReleaseMachines(args gomaasapi.ReleaseMachinesArgs) err
 	return c.NextErr()
 }
 
+type fakeMachineOnlyController struct {
+	machines []gomaasapi.Machine
+}
+
+func (f *fakeMachineOnlyController) Machines(gomaasapi.MachinesArgs) ([]gomaasapi.Machine, error) {
+	return f.machines, nil
+}
+
 type fakeBootResource struct {
 	gomaasapi.BootResource
 	name         string

provider/maas/maas2instance.go

@@ -65,7 +65,10 @@ func (mi *maas2Instance) Addresses() ([]network.Address, error) {
 // Status returns a juju status based on the maas instance returned
 // status message.
 func (mi *maas2Instance) Status() instance.InstanceStatus {
-	// TODO (babbageclunk): this should rerequest to get live status.
+	// A fresh status is not obtained here because the interface it is intended
+	// to satisfy gets a new maas2Instance before each call, using a fresh status
+	// would cause us to mask errors since this interface does not contemplate
+	// returing them.
 	statusName := mi.machine.StatusName()
 	statusMsg := mi.machine.StatusMessage()
 	return convertInstanceStatus(statusName, statusMsg, mi.Id())

provider/maas/maas2instance_test.go

@@ -19,21 +19,24 @@ type maas2InstanceSuite struct {
 var _ = gc.Suite(&maas2InstanceSuite{})
 
 func (s *maas2InstanceSuite) TestString(c *gc.C) {
-	instance := &maas2Instance{machine: &fakeMachine{hostname: "peewee", systemID: "herman"}}
+	machine := &fakeMachine{hostname: "peewee", systemID: "herman"}
+	instance := &maas2Instance{machine: machine}
 	c.Assert(instance.String(), gc.Equals, "peewee:herman")
 }
 
 func (s *maas2InstanceSuite) TestID(c *gc.C) {
-	thing := &maas2Instance{machine: &fakeMachine{systemID: "herman"}}
+	machine := &fakeMachine{systemID: "herman"}
+	thing := &maas2Instance{machine: machine}
 	c.Assert(thing.Id(), gc.Equals, instance.Id("herman"))
 }
 
 func (s *maas2InstanceSuite) TestAddresses(c *gc.C) {
-	instance := &maas2Instance{machine: &fakeMachine{ipAddresses: []string{
+	machine := &fakeMachine{ipAddresses: []string{
 		"0.0.0.0",
 		"1.2.3.4",
 		"127.0.0.1",
-	}}}
+	}}
+	instance := &maas2Instance{machine: machine}
 	expectedAddresses := []network.Address{
 		network.NewAddress("0.0.0.0"),
 		network.NewAddress("1.2.3.4"),
@@ -45,26 +48,30 @@ func (s *maas2InstanceSuite) TestAddresses(c *gc.C) {
 }
 
 func (s *maas2InstanceSuite) TestZone(c *gc.C) {
-	instance := &maas2Instance{machine: &fakeMachine{zoneName: "inflatable"}}
+	machine := &fakeMachine{zoneName: "inflatable"}
+	instance := &maas2Instance{machine: machine}
 	zone, err := instance.zone()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(zone, gc.Equals, "inflatable")
 }
 
 func (s *maas2InstanceSuite) TestStatusSuccess(c *gc.C) {
-	thing := &maas2Instance{machine: &fakeMachine{statusMessage: "Wexler", statusName: "Deploying"}}
+	machine := &fakeMachine{statusMessage: "Wexler", statusName: "Deploying"}
+	thing := &maas2Instance{machine: machine}
 	result := thing.Status()
 	c.Assert(result, jc.DeepEquals, instance.InstanceStatus{status.Allocating, "Deploying: Wexler"})
 }
 
 func (s *maas2InstanceSuite) TestStatusError(c *gc.C) {
-	thing := &maas2Instance{machine: &fakeMachine{statusMessage: "", statusName: ""}}
+	machine := &fakeMachine{statusMessage: "", statusName: ""}
+	thing := &maas2Instance{machine: machine}
 	result := thing.Status()
 	c.Assert(result, jc.DeepEquals, instance.InstanceStatus{"", "error in getting status"})
 }
 
 func (s *maas2InstanceSuite) TestHostname(c *gc.C) {
-	thing := &maas2Instance{machine: &fakeMachine{hostname: "saul-goodman"}}
+	machine := &fakeMachine{hostname: "saul-goodman"}
+	thing := &maas2Instance{machine: machine}
 	result, err := thing.hostname()
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(result, gc.Equals, "saul-goodman")

worker/instancepoller/updater.go

@@ -271,6 +271,7 @@ func pollInstanceInfo(context machineContext, m machine) (instInfo instanceInfo,
 				return instanceInfo{}, err
 			}
 		}
+
 	}
 	if m.Life() != params.Dead {
 		providerAddresses, err := m.ProviderAddresses()