builder/azure_arm: Fix build failures due to the deletion of attached managed disks

Previously (prior to v1.6.2) the Azure ARM builder had two delete functions
one that would run before any of the StepDelete* types, and one on deployment template
cleanup. The refactored coded re-introduces the logic from the previously removed
step in v1.6.1 as the main delete logic for the whole deployment
template. Ensuring that all deployed items are deleted before trying to
remove any managed disks.

This change moves all the deletion logic into the
step_deployment_template#Cleanup function to ensure that dependent steps
are only called once the created deployment items (i.e
StepDelateAdditionalDisks) have been deleted.

Test results before change
```

    compute.DisksClient#Delete: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="OperationNotAllowed" Message="Disk pkrdd02e9rzzu5k-1 is attached to VM /subscriptions/1f90521a-24f6-4758-ac3d-88d869fb0bf5/resourceGroups/packer-acceptance-test/providers/Microsoft.Compute/virtualMachines/pkrvm02e9rzzu5k."
--- FAIL: TestBuilderAcc_ManagedDisk_Windows_Build_Resource_Group_Additional_Disk (454.00s)
FAIL
FAIL    github.com/hashicorp/packer/builder/azure/arm   454.008s

```

Test results after change
```
--- PASS: TestBuilderAcc_ManagedDisk_Windows_Build_Resource_Group_Additional_Disk (563.56s)

```

Closes #10070

builder/azure/arm/builder.go

@@ -222,7 +222,6 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack
 			NewStepSnapshotDataDisks(azureClient, ui, &b.config),
 			NewStepCaptureImage(azureClient, ui),
 			NewStepPublishToSharedImageGallery(azureClient, ui, &b.config),
-			NewStepDeleteAdditionalDisks(azureClient, ui),
 		}
 	} else if b.config.OSType == constants.Target_Windows {
 		steps = []multistep.Step{
@@ -263,7 +262,6 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack
 			NewStepSnapshotDataDisks(azureClient, ui, &b.config),
 			NewStepCaptureImage(azureClient, ui),
 			NewStepPublishToSharedImageGallery(azureClient, ui, &b.config),
-			NewStepDeleteAdditionalDisks(azureClient, ui),
 		)
 	} else {
 		return nil, fmt.Errorf("Builder does not support the os_type '%s'", b.config.OSType)

builder/azure/arm/builder_acc_test.go

@@ -46,6 +46,14 @@ func TestBuilderAcc_ManagedDisk_Windows_Build_Resource_Group(t *testing.T) {
 	})
 }
 
+func TestBuilderAcc_ManagedDisk_Windows_Build_Resource_Group_Additional_Disk(t *testing.T) {
+	builderT.Test(t, builderT.TestCase{
+		PreCheck: func() { testAccPreCheck(t) },
+		Builder:  &Builder{},
+		Template: testBuilderAccManagedDiskWindowsBuildResourceGroupAdditionalDisk,
+	})
+}
+
 func TestBuilderAcc_ManagedDisk_Windows_DeviceLogin(t *testing.T) {
 	if os.Getenv(DeviceLoginAcceptanceTest) == "" {
 		t.Skip(fmt.Sprintf(
@@ -169,6 +177,42 @@ const testBuilderAccManagedDiskWindowsBuildResourceGroup = `
 }
 `
 
+const testBuilderAccManagedDiskWindowsBuildResourceGroupAdditionalDisk = `
+{
+	"variables": {
+	  "client_id": "{{env ` + "`ARM_CLIENT_ID`" + `}}",
+	  "client_secret": "{{env ` + "`ARM_CLIENT_SECRET`" + `}}",
+	  "subscription_id": "{{env ` + "`ARM_SUBSCRIPTION_ID`" + `}}"
+	},
+	"builders": [{
+	  "type": "test",
+
+	  "client_id": "{{user ` + "`client_id`" + `}}",
+	  "client_secret": "{{user ` + "`client_secret`" + `}}",
+	  "subscription_id": "{{user ` + "`subscription_id`" + `}}",
+
+	  "build_resource_group_name" : "packer-acceptance-test",
+	  "managed_image_resource_group_name": "packer-acceptance-test",
+	  "managed_image_name": "testBuilderAccManagedDiskWindowsBuildResourceGroupAdditionDisk-{{timestamp}}",
+
+	  "os_type": "Windows",
+	  "image_publisher": "MicrosoftWindowsServer",
+	  "image_offer": "WindowsServer",
+	  "image_sku": "2012-R2-Datacenter",
+
+	  "communicator": "winrm",
+	  "winrm_use_ssl": "true",
+	  "winrm_insecure": "true",
+	  "winrm_timeout": "3m",
+	  "winrm_username": "packer",
+	  "async_resourcegroup_delete": "true",
+
+	  "vm_size": "Standard_DS2_v2",
+	  "disk_additional_size": [10,15]
+	}]
+}
+`
+
 const testBuilderAccManagedDiskWindowsDeviceLogin = `
 {
 	"variables": {

builder/azure/arm/step_deploy_template.go

@@ -6,8 +6,10 @@ import (
 	"fmt"
 	"net/url"
 	"strings"
+	"time"
 
 	"github.com/hashicorp/packer/builder/azure/common/constants"
+	"github.com/hashicorp/packer/common/retry"
 	"github.com/hashicorp/packer/helper/multistep"
 	"github.com/hashicorp/packer/packer"
 )
@@ -15,7 +17,7 @@ import (
 type StepDeployTemplate struct {
 	client     *AzureClient
 	deploy     func(ctx context.Context, resourceGroupName string, deploymentName string) error
-	delete     func(ctx context.Context, client *AzureClient, resourceType string, resourceName string, resourceGroupName string) error
+	delete     func(ctx context.Context, deploymentName, resourceGroupName string) error
 	disk       func(ctx context.Context, resourceGroupName string, computeName string) (string, string, error)
 	deleteDisk func(ctx context.Context, imageType string, imageName string, resourceGroupName string) error
 	say        func(message string)
@@ -36,7 +38,7 @@ func NewStepDeployTemplate(client *AzureClient, ui packer.Ui, config *Config, de
 	}
 
 	step.deploy = step.deployTemplate
-	step.delete = deleteResource
+	step.delete = step.deleteDeploymentResources
 	step.disk = step.getImageDetails
 	step.deleteDisk = step.deleteImage
 	return step
@@ -62,8 +64,9 @@ func (s *StepDeployTemplate) Cleanup(state multistep.StateBag) {
 		}
 	}()
 
-	//Only clean up if this was an existing resource group and the resource group
+	// Only clean up if this is an existing resource group that has been verified to exist.
-	//is marked as created
+	// ArmIsResourceGroupCreated is set in step_create_resource_group to true, when Packer has verified that the resource group exists.
+	// ArmIsExistingResourceGroup is set to true when build_resource_group is set in the Packer configuration.
 	existingResourceGroup := state.Get(constants.ArmIsExistingResourceGroup).(bool)
 	resourceGroupCreated := state.Get(constants.ArmIsResourceGroupCreated).(bool)
 	if !existingResourceGroup || !resourceGroupCreated {
@@ -75,74 +78,12 @@ func (s *StepDeployTemplate) Cleanup(state multistep.StateBag) {
 
 	deploymentName := s.name
 	resourceGroupName := state.Get(constants.ArmResourceGroupName).(string)
-
+	err := s.deleteDeploymentResources(context.TODO(), deploymentName, resourceGroupName)
-	// Get image disk details before deleting the image; otherwise we won't be able to
+	if err != nil {
-	// delete the disk as the image request will return a 404
+		s.reportIfError(err, resourceGroupName)
-	computeName := state.Get(constants.ArmComputeName).(string)
-	imageType, imageName, err := s.disk(context.TODO(), resourceGroupName, computeName)
-
-	if err != nil && !strings.Contains(err.Error(), "ResourceNotFound") {
-		ui.Error(fmt.Sprintf("Could not retrieve OS Image details: %s", err))
 	}
 
-	ui.Say(" -> Deployment Resources within: " + deploymentName)
+	NewStepDeleteAdditionalDisks(s.client, ui).Run(context.TODO(), state)
-	if deploymentName != "" {
-		maxResources := int32(50)
-		deploymentOperations, err := s.client.DeploymentOperationsClient.ListComplete(context.TODO(), resourceGroupName, deploymentName, &maxResources)
-		if err != nil {
-			ui.Error(fmt.Sprintf("Error deleting resources.  Please delete them manually.\n\n"+
-				"Name: %s\n"+
-				"Error: %s", resourceGroupName, err))
-		}
-
-		for deploymentOperations.NotDone() {
-			deploymentOperation := deploymentOperations.Value()
-			// Sometimes an empty operation is added to the list by Azure
-			if deploymentOperation.Properties.TargetResource == nil {
-				if err := deploymentOperations.Next(); err != nil {
-					ui.Error(fmt.Sprintf("Error moving to to next deployment operation ...\n\n"+
-						"Name: %s\n"+
-						"Error: %s", resourceGroupName, err))
-					break
-				}
-				continue
-			}
-
-			ui.Say(fmt.Sprintf(" -> %s : '%s'",
-				*deploymentOperation.Properties.TargetResource.ResourceType,
-				*deploymentOperation.Properties.TargetResource.ResourceName))
-
-			err = s.delete(context.TODO(), s.client,
-				*deploymentOperation.Properties.TargetResource.ResourceType,
-				*deploymentOperation.Properties.TargetResource.ResourceName,
-				resourceGroupName)
-			if err != nil {
-				ui.Error(fmt.Sprintf("Error deleting resource.  Please delete manually.\n\n"+
-					"Name: %s\n"+
-					"Error: %s", *deploymentOperation.Properties.TargetResource.ResourceName, err))
-			}
-
-			if err = deploymentOperations.Next(); err != nil {
-				ui.Error(fmt.Sprintf("Error deleting resources.  Please delete them manually.\n\n"+
-					"Name: %s\n"+
-					"Error: %s", resourceGroupName, err))
-				break
-			}
-		}
-
-		// The disk is not defined as an operation in the template so it has to be deleted separately
-		if imageType == "" && imageName == "" {
-			return
-		}
-
-		ui.Say(fmt.Sprintf(" -> %s : '%s'", imageType, imageName))
-		err = s.deleteDisk(context.TODO(), imageType, imageName, resourceGroupName)
-		if err != nil {
-			ui.Error(fmt.Sprintf("Error deleting resource.  Please delete manually.\n\n"+
-				"Name: %s\n"+
-				"Error: %s", imageName, err))
-		}
-	}
 }
 
 func (s *StepDeployTemplate) deployTemplate(ctx context.Context, resourceGroupName string, deploymentName string) error {
@@ -152,27 +93,31 @@ func (s *StepDeployTemplate) deployTemplate(ctx context.Context, resourceGroupNa
 	}
 
 	f, err := s.client.DeploymentsClient.CreateOrUpdate(ctx, resourceGroupName, deploymentName, *deployment)
-	if err == nil {
-		err = f.WaitForCompletionRef(ctx, s.client.DeploymentsClient.Client)
-	}
 	if err != nil {
 		s.say(s.client.LastError.Error())
+		return err
 	}
+
+	err = f.WaitForCompletionRef(ctx, s.client.DeploymentsClient.Client)
+	if err == nil {
+		s.say(s.client.LastError.Error())
+	}
+
 	return err
 }
 
 func (s *StepDeployTemplate) deleteTemplate(ctx context.Context, state multistep.StateBag) error {
-	var resourceGroupName = state.Get(constants.ArmResourceGroupName).(string)
+	deploymentName := s.name
-	var deploymentName = s.name
+	resourceGroupName := state.Get(constants.ArmResourceGroupName).(string)
 	ui := state.Get("ui").(packer.Ui)
 
 	ui.Say(fmt.Sprintf("Removing the created Deployment object: '%s'", deploymentName))
 	f, err := s.client.DeploymentsClient.Delete(ctx, resourceGroupName, deploymentName)
-	if err == nil {
+	if err != nil {
-		err = f.WaitForCompletionRef(ctx, s.client.DeploymentsClient.Client)
+		return err
 	}
 
-	return err
+	return f.WaitForCompletionRef(ctx, s.client.DeploymentsClient.Client)
 }
 
 func (s *StepDeployTemplate) getImageDetails(ctx context.Context, resourceGroupName string, computeName string) (string, string, error) {
@@ -186,11 +131,12 @@ func (s *StepDeployTemplate) getImageDetails(ctx context.Context, resourceGroupN
 	if vm.StorageProfile.OsDisk.Vhd != nil {
 		imageType = "image"
 		imageName = *vm.StorageProfile.OsDisk.Vhd.URI
-	} else {
+		return imageType, imageName, nil
-		imageType = "Microsoft.Compute/disks"
-		imageName = *vm.StorageProfile.OsDisk.ManagedDisk.ID
 	}
 
+	imageType = "Microsoft.Compute/disks"
+	imageName = *vm.StorageProfile.OsDisk.ManagedDisk.ID
+
 	return imageType, imageName, nil
 }
 
@@ -267,3 +213,55 @@ func (s *StepDeployTemplate) deleteImage(ctx context.Context, imageType string,
 
 	return blob.Delete(nil)
 }
+
+func (s *StepDeployTemplate) deleteDeploymentResources(ctx context.Context, deploymentName, resourceGroupName string) error {
+	var maxResources int32 = 50
+	deploymentOperations, err := s.client.DeploymentOperationsClient.ListComplete(ctx, resourceGroupName, deploymentName, &maxResources)
+	if err != nil {
+		s.reportIfError(err, resourceGroupName)
+		return err
+	}
+
+	for deploymentOperations.NotDone() {
+		deploymentOperation := deploymentOperations.Value()
+		// Sometimes an empty operation is added to the list by Azure
+		if deploymentOperation.Properties.TargetResource == nil {
+			deploymentOperations.Next()
+			continue
+		}
+
+		resourceName := *deploymentOperation.Properties.TargetResource.ResourceName
+		resourceType := *deploymentOperation.Properties.TargetResource.ResourceType
+
+		s.say(fmt.Sprintf(" -> %s : '%s'", resourceType, resourceName))
+
+		retry.Config{
+			Tries:      10,
+			RetryDelay: (&retry.Backoff{InitialBackoff: 10 * time.Second, MaxBackoff: 600 * time.Second, Multiplier: 2}).Linear,
+		}.Run(ctx, func(ctx context.Context) error {
+			err := deleteResource(ctx, s.client,
+				resourceType,
+				resourceName,
+				resourceGroupName)
+			if err != nil {
+				s.reportIfError(err, resourceName)
+			}
+			return nil
+		})
+
+		if err = deploymentOperations.Next(); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func (s *StepDeployTemplate) reportIfError(err error, resourceName string) {
+	if err != nil {
+		s.say(fmt.Sprintf("Error deleting resource. Please delete manually.\n\n"+
+			"Name: %s\n"+
+			"Error: %s", resourceName, err.Error()))
+		s.error(err)
+	}
+}