provider/azure: address review comments

- drop "controller" model name special case
- add test for retrying until common deployment
  is complete

provider/azure/environ.go

@@ -21,7 +21,7 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/juju/errors"
 	"github.com/juju/loggo"
-	"github.com/juju/utils"
+	"github.com/juju/retry"
 	"github.com/juju/utils/arch"
 	"github.com/juju/utils/os"
 	jujuseries "github.com/juju/utils/series"
@@ -783,6 +783,10 @@ func (env *azureEnviron) waitCommonResourcesCreated() error {
 	return nil
 }
 
+type deploymentIncompleteError struct {
+	error
+}
+
 func (env *azureEnviron) waitCommonResourcesCreatedLocked() error {
 	deploymentsClient := resources.DeploymentsClient{env.resources}
 
@@ -794,11 +798,7 @@ func (env *azureEnviron) waitCommonResourcesCreatedLocked() error {
 	// for the "common" deployment to be in one of the terminal
 	// states. The deployment typically takes only around 30 seconds,
 	// but we allow for a longer duration to be defensive.
-	attempt := utils.AttemptStrategy{
-		Total: 5 * time.Minute,
-		Delay: 5 * time.Second,
-	}
-	for a := attempt.Start(); a.Next(); {
+	waitDeployment := func() error {
 		var result resources.DeploymentExtended
 		if err := env.callAPI(func() (autorest.Response, error) {
 			var err error
@@ -816,19 +816,34 @@ func (env *azureEnviron) waitCommonResourcesCreatedLocked() error {
 			return errors.Annotate(err, "querying common deployment")
 		}
 		if result.Properties == nil {
-			continue
+			return deploymentIncompleteError{errors.New("deployment incomplete")}
 		}
-		switch state := to.String(result.Properties.ProvisioningState); state {
-		case "Succeeded":
+
+		state := to.String(result.Properties.ProvisioningState)
+		if state == "Succeeded" {
 			// The deployment has succeeded, so the resources are
 			// ready for use.
 			return nil
+		}
+		err := errors.Errorf("common resource deployment status is %q", state)
+		switch state {
 		case "Canceled", "Failed", "Deleted":
-			return errors.Errorf(
-				"common resource deployment status is %q", state,
-			)
+		default:
+			err = deploymentIncompleteError{err}
 		}
+		return err
 	}
+	return retry.Call(retry.CallArgs{
+		Func: waitDeployment,
+		IsFatalError: func(err error) bool {
+			_, ok := err.(deploymentIncompleteError)
+			return !ok
+		},
+		Attempts:    -1,
+		Delay:       5 * time.Second,
+		MaxDuration: 5 * time.Minute,
+		Clock:       env.provider.config.RetryClock,
+	})
 	return errors.Errorf("timed out waiting for common resources to be created")
 }
 

provider/azure/environ_test.go

@@ -117,6 +117,7 @@ type environSuite struct {
 	storageAccount     *storage.Account
 	storageAccountKeys *storage.AccountListKeysResult
 	ubuntuServerSKUs   []compute.VirtualMachineImageResource
+	commonDeployment   *resources.DeploymentExtended
 	deployment         *resources.Deployment
 }
 
@@ -213,6 +214,12 @@ func (s *environSuite) SetUpTest(c *gc.C) {
 		{Name: to.StringPtr("16.04-LTS")},
 	}
 
+	s.commonDeployment = &resources.DeploymentExtended{
+		Properties: &resources.DeploymentPropertiesExtended{
+			ProvisioningState: to.StringPtr("Succeeded"),
+		},
+	}
+
 	s.deployment = nil
 }
 
@@ -329,12 +336,7 @@ func (s *environSuite) startInstanceSenders(bootstrap bool) azuretesting.Senders
 	if !bootstrap {
 		// When starting an instance, we must wait for the common
 		// deployment to complete.
-		commonDeployment := &resources.DeploymentExtended{
-			Properties: &resources.DeploymentPropertiesExtended{
-				ProvisioningState: to.StringPtr("Succeeded"),
-			},
-		}
-		senders = append(senders, s.makeSender("/deployments/common", commonDeployment))
+		senders = append(senders, s.makeSender("/deployments/common", s.commonDeployment))
 	}
 	senders = append(senders, s.makeSender("/deployments/machine-0", s.deployment))
 	return senders
@@ -662,6 +664,57 @@ func (s *environSuite) TestStartInstanceTooManyRequestsTimeout(c *gc.C) {
 	})
 }
 
+func (s *environSuite) TestStartInstanceCommonDeployment(c *gc.C) {
+	// StartInstance waits for the "common" deployment to complete
+	// successfully before creating the VM deployment. If the deployment
+	// is seen to be in a terminal state, the process will stop
+	// immediately.
+	s.commonDeployment.Properties.ProvisioningState = to.StringPtr("Failed")
+
+	env := s.openEnviron(c)
+	senders := s.startInstanceSenders(false)
+	s.sender = senders
+	s.requests = nil
+
+	_, err := env.StartInstance(makeStartInstanceParams(c, s.controllerUUID, "quantal"))
+	c.Assert(err, gc.ErrorMatches,
+		`creating virtual machine "machine-0": `+
+			`waiting for common resources to be created: `+
+			`common resource deployment status is "Failed"`)
+}
+
+func (s *environSuite) TestStartInstanceCommonDeploymentRetryTimeout(c *gc.C) {
+	// StartInstance waits for the "common" deployment to complete
+	// successfully before creating the VM deployment.
+	s.commonDeployment.Properties.ProvisioningState = to.StringPtr("Running")
+
+	env := s.openEnviron(c)
+	senders := s.startInstanceSenders(false)
+
+	const failures = 60 // 5 minutes / 5 seconds
+	head, tail := senders[:2], senders[2:]
+	for i := 0; i < failures; i++ {
+		head = append(head, s.makeSender("/deployments/common", s.commonDeployment))
+	}
+	senders = append(head, tail...)
+	s.sender = senders
+	s.requests = nil
+
+	_, err := env.StartInstance(makeStartInstanceParams(c, s.controllerUUID, "quantal"))
+	c.Assert(err, gc.ErrorMatches,
+		`creating virtual machine "machine-0": `+
+			`waiting for common resources to be created: `+
+			`max duration exceeded: deployment incomplete`)
+
+	var expectedCalls []gitjujutesting.StubCall
+	for i := 0; i < failures; i++ {
+		expectedCalls = append(expectedCalls, gitjujutesting.StubCall{
+			"After", []interface{}{5 * time.Second},
+		})
+	}
+	s.retryClock.CheckCalls(c, expectedCalls)
+}
+
 func (s *environSuite) TestStartInstanceDistributionGroup(c *gc.C) {
 	c.Skip("TODO: test StartInstance's DistributionGroup behaviour")
 }

provider/azure/upgrades.go

@@ -81,13 +81,6 @@ func (step commonDeploymentUpgradeStep) Run() error {
 }
 
 func isControllerEnviron(env *azureEnviron) (bool, error) {
-	if env.envName != "controller" {
-		// At the time of writing, the controller model name is
-		// hard-coded to "controller". If/when this changes, we
-		// must drop this check.
-		return false, nil
-	}
-
 	// Look for a machine with the "juju-is-controller" tag set to "true".
 	client := compute.VirtualMachinesClient{env.compute}
 	var result compute.VirtualMachineListResult

provider/azure/upgrades_test.go

@@ -101,13 +101,15 @@ func (s *environUpgradeSuite) TestEnvironUpgradeOperationCreateCommonDeployment(
 		},
 	}
 
+	vmListSender := azuretesting.NewSenderWithValue(&compute.VirtualMachineListResult{})
+	vmListSender.PathPattern = ".*/virtualMachines"
 	nsgSender := azuretesting.NewSenderWithValue(&nsg)
 	nsgSender.PathPattern = ".*/networkSecurityGroups/juju-internal-nsg"
 	deploymentSender := azuretesting.NewSenderWithValue(&resources.Deployment{})
 	deploymentSender.PathPattern = ".*/deployments/common"
-	s.sender = append(s.sender, nsgSender, deploymentSender)
+	s.sender = append(s.sender, vmListSender, nsgSender, deploymentSender)
 	c.Assert(op0.Steps[0].Run(), jc.ErrorIsNil)
-	c.Assert(s.requests, gc.HasLen, 2)
+	c.Assert(s.requests, gc.HasLen, 3)
 
 	expectedSecurityRules := []network.SecurityRule{{
 		Name: to.StringPtr("SSHInbound"),
@@ -171,7 +173,7 @@ func (s *environUpgradeSuite) TestEnvironUpgradeOperationCreateCommonDeployment(
 	}}
 
 	var actual resources.Deployment
-	unmarshalRequestBody(c, s.requests[1], &actual)
+	unmarshalRequestBody(c, s.requests[2], &actual)
 	c.Assert(actual.Properties, gc.NotNil)
 	c.Assert(actual.Properties.Template, gc.NotNil)
 	resources := (*actual.Properties.Template)["resources"].([]interface{})