From afd33679f5eccc9e662587ca332c62ac439666f7 Mon Sep 17 00:00:00 2001 From: Wilken Rivera Date: Fri, 23 Oct 2020 13:17:05 -0400 Subject: [PATCH] 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= 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 | 2 - builder/azure/arm/builder_acc_test.go | 44 ++++++ builder/azure/arm/step_deploy_template.go | 160 +++++++++++----------- 3 files changed, 123 insertions(+), 83 deletions(-) diff --git a/builder/azure/arm/builder.go b/builder/azure/arm/builder.go index 3b01a5bfa..90bbcc413 100644 --- a/builder/azure/arm/builder.go +++ b/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) diff --git a/builder/azure/arm/builder_acc_test.go b/builder/azure/arm/builder_acc_test.go index 1e63f5ef9..1837bf5b0 100644 --- a/builder/azure/arm/builder_acc_test.go +++ b/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": { diff --git a/builder/azure/arm/step_deploy_template.go b/builder/azure/arm/step_deploy_template.go index 003514e70..04a5295f4 100644 --- a/builder/azure/arm/step_deploy_template.go +++ b/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 - //is marked as created + // Only clean up if this is an existing resource group that has been verified to exist. + // 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) - - // Get image disk details before deleting the image; otherwise we won't be able to - // delete the disk as the image request will return a 404 - 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)) + err := s.deleteDeploymentResources(context.TODO(), deploymentName, resourceGroupName) + if err != nil { + s.reportIfError(err, resourceGroupName) } - ui.Say(" -> Deployment Resources within: " + deploymentName) - 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)) - } - } + NewStepDeleteAdditionalDisks(s.client, ui).Run(context.TODO(), state) } 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) - var deploymentName = s.name + 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 { - err = f.WaitForCompletionRef(ctx, s.client.DeploymentsClient.Client) + if err != nil { + 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 { - imageType = "Microsoft.Compute/disks" - imageName = *vm.StorageProfile.OsDisk.ManagedDisk.ID + return imageType, imageName, nil } + 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) + } +}