fix: [arm builder] storage blob VHD deletion, cleanup post build deletion #410

JenGoldstrich · 2024-04-17T21:37:12Z

This PR has three main purposes

Fix v2.1.0: Storage account deletion URI invalid, wrong parameters sent #409 by passing in storage account name to client as expected
Fix NIC not disassociated before deletion #273 by deleting the VM/Network Interface before deleting other networking resources, these errors were harmless and didn't actually lead to orphaned resources, but its noisy and unnecessary
Make it possible to unit test these new changes with substantial refactoring to step_deploy_template's CleanUp step, moving several of the resource deletions into their own functions.

Closes #409
Closes #273

Before this PR when building a VHD image with additional disks that should be deleted, you can see extra print statements at the bottom of 2, and my storage account as well as the failed additional disk deletion, and NIC repeated retry errors.

==> azure-arm:
==> azure-arm: Deleting Virtual Machine deployment and its attached resources...
==> azure-arm: Adding to deletion queue -> Microsoft.Compute/virtualMachines : 'replaced'
==> azure-arm: Adding to deletion queue -> Microsoft.Network/networkInterfaces : 'replaced'
==> azure-arm: Adding to deletion queue -> Microsoft.Network/virtualNetworks : 'replaced'
==> azure-arm: Adding to deletion queue -> Microsoft.Network/publicIPAddresses : 'replaced'
==> azure-arm: Waiting for deletion of all resources...
==> azure-arm: Attempting deletion -> Microsoft.Compute/virtualMachines : 'replaced'==> azure-arm: Attempting deletion -> Microsoft.Network/publicIPAddresses : 'replaced'
==> azure-arm: Attempting deletion -> Microsoft.Network/virtualNetworks : 'replaced'==> azure-arm: Attempting deletion -> Microsoft.Network/networkInterfaces : 'replaced'
==> azure-arm: Couldn't delete Microsoft.Network/publicIPAddresses resource. Will retry.
==> azure-arm: Name: replaced
==> azure-arm: Couldn't delete Microsoft.Network/virtualNetworks resource. Will retry.
==> azure-arm: Name: replaced
==> azure-arm: Attempting deletion -> Microsoft.Network/publicIPAddresses : 'replaced'
==> azure-arm: Attempting deletion -> Microsoft.Network/virtualNetworks : 'replaced'==> azure-arm: Skipping deletion -> image : 'https://removedstorageaccount.blob.core.windows.net/images/removed.vhd' since 'keep_os_disk' is set to true
==> azure-arm:  Deleting Additional Disk -> 1: 'https://removedstorageaccount.blob.core.windows.net/images/pkrdisk-1.vhd'
==> azure-arm: Failed to delete the managed Additional Disk!
==> azure-arm:  Deleting Additional Disk -> 2: 'https://removedstorageaccount.blob.core.windows.net/images/pkrdisk-2.vhd'
==> azure-arm: Failed to delete the managed Additional Disk!
==> azure-arm: Removing the created Deployment object: 'pkrdplafewlqjyv'
==> azure-arm:
==> azure-arm: The resource group was not created by Packer, not deleting ...
==> azure-arm: https://removedstorageaccount.blob.core.windows.net/
==> azure-arm: 2
Build 'azure-arm' finished after 3 minutes 6 seconds.

After

==> azure-arm: Deleting Virtual Machine deployment and its attached resources...
==> azure-arm: Deleted -> Microsoft.Compute/virtualMachines : 'pkrvmq58v6zr4b1'
==> azure-arm: Deleted -> Microsoft.Network/networkInterfaces : 'pkrniq58v6zr4b1'
==> azure-arm: Deleted -> Microsoft.Network/publicIPAddresses : 'pkripq58v6zr4b1'
==> azure-arm: Deleted -> Microsoft.Network/virtualNetworks : 'pkrvnq58v6zr4b1'
==> azure-arm: Skipping deletion -> image : 'https://removed.blob.core.windows.net/images/removed.vhd' since 'keep_os_disk' is set to true
==> azure-arm:  Deleted Additional Disk -> 1: 'https://removed.blob.core.windows.net/images/removed-1.vhd'
==> azure-arm:  Deleted Additional Disk -> 2: 'https://removed.blob.core.windows.net/images/removed-2.vhd'
==> azure-arm: Removing the created Deployment object: 'pkrdpq58v6zr4b1'
==> azure-arm:
==> azure-arm: The resource group was not created by Packer, not deleting ...
Build 'azure-arm' finished after 3 minutes 1 second.

JenGoldstrich · 2024-04-17T22:21:02Z

builder/azure/arm/azure_client.go

 		storageAccountAuthorizer, err := commonclient.BuildStorageAuthorizer(ctx, authOptions, *cloud)
 		if err != nil {
 			return nil, err
 		}
-
-		blobClient, err := giovanniBlobStorageSDK.NewWithBaseUri(cloud.Storage.Name())


This actually was just passing in "AzureStorage" as a string, however the client never invoked this so we never ran into the issue since Delete blobs endpoint previously accepted storage account, now we had to give it this URL.

See https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview#standard-endpoints for more info

builder/azure/arm/step_deploy_template.go

JenGoldstrich · 2024-04-17T23:33:52Z

builder/azure/arm/step_deploy_template.go

@@ -143,7 +143,7 @@ func (s *StepDeployTemplate) Cleanup(state multistep.StateBag) {

 			err := s.deleteImage(ctx, additionaldisk, resourceGroupName, (isManagedDisk || isSIGImage), subscriptionId, armStorageAccountName)
 			if err != nil {
-				s.say("Failed to delete the managed Additional Disk!")
+				s.say(fmt.Sprintf("Failed to delete the managed Additional Disk! %s", err))


We weren't actually logging the error here 🤦‍♀️

JenGoldstrich · 2024-04-17T23:34:33Z

go.mod

@@ -22,7 +22,7 @@ require (
 	github.com/hashicorp/go-azure-sdk/resource-manager v0.20240411.1145857
 	github.com/hashicorp/go-azure-sdk/sdk v0.20240411.1145857
 	github.com/mitchellh/go-homedir v1.1.0
-	github.com/tombuildsstuff/giovanni v0.25.3
+	github.com/tombuildsstuff/giovanni v0.26.1


This update isn't required, I was just seeing when the change was introduced and figured I'd upgrade while testing

builder/azure/arm/step_deploy_template.go

JenGoldstrich · 2024-04-20T01:00:55Z

builder/azure/arm/step_deploy_template.go

@@ -191,9 +274,6 @@ func (s *StepDeployTemplate) getImageDetails(ctx context.Context, subscriptionId
 	defer cancel()
 	vmID := virtualmachines.NewVirtualMachineID(subscriptionId, resourceGroupName, computeName)
 	vm, err := s.client.VirtualMachinesClient.Get(pollingContext, vmID, virtualmachines.DefaultGetOperationOptions())
-	if err != nil {


This was invalid code, the lower error was unreachable

builder/azure/arm/step_deploy_template.go

lbajolet-hashicorp · 2024-04-29T13:48:00Z

builder/azure/arm/step_deploy_template.go

+		s.reportResourceDeletionFailure(err, resourceName)
+	}
+	return err
+


Tiniest of nits: extra blank line here

lbajolet-hashicorp

Left a few last nits, but overall this LGTM!

builder/azure/arm/step_deploy_template.go

JenGoldstrich force-pushed the fix_storage_account_delete_and_tidy_deletion_process branch from 1134baf to 94d542c Compare April 17, 2024 21:38

JenGoldstrich changed the title ~~Fix incorrect arguments passed into storage account and delete VM and…~~ fix: storage account deletion wrong params, nic disaccoiation repeated errors every build Apr 17, 2024

JenGoldstrich commented Apr 17, 2024

View reviewed changes

builder/azure/arm/step_deploy_template.go Outdated Show resolved Hide resolved

JenGoldstrich marked this pull request as ready for review April 17, 2024 23:14

JenGoldstrich requested a review from a team as a code owner April 17, 2024 23:14

JenGoldstrich mentioned this pull request Apr 17, 2024

NIC not disassociated before deletion #273

Closed

JenGoldstrich commented Apr 17, 2024

View reviewed changes

lbajolet-hashicorp reviewed Apr 19, 2024

View reviewed changes

JenGoldstrich changed the title ~~fix: storage account deletion wrong params, nic disaccoiation repeated errors every build~~ fix: storage account deletion wrong params, cleanup post build deletion Apr 19, 2024

JenGoldstrich force-pushed the fix_storage_account_delete_and_tidy_deletion_process branch 4 times, most recently from b4959b7 to f555542 Compare April 20, 2024 00:59

JenGoldstrich commented Apr 20, 2024

View reviewed changes

builder/azure/arm/step_deploy_template.go Outdated Show resolved Hide resolved

JenGoldstrich requested a review from lbajolet-hashicorp April 20, 2024 01:38

JenGoldstrich changed the title ~~fix: storage account deletion wrong params, cleanup post build deletion~~ fix: [arm builder] storage account deletion wrong params, cleanup post build deletion Apr 20, 2024

JenGoldstrich changed the title ~~fix: [arm builder] storage account deletion wrong params, cleanup post build deletion~~ fix: [arm builder] storage blob deletion wrong params, cleanup post build deletion Apr 20, 2024

lbajolet-hashicorp reviewed Apr 23, 2024

View reviewed changes

lbajolet-hashicorp force-pushed the fix_storage_account_delete_and_tidy_deletion_process branch from 0d99043 to 5362dbe Compare April 24, 2024 17:48

lbajolet-hashicorp reviewed Apr 29, 2024

View reviewed changes

lbajolet-hashicorp approved these changes Apr 29, 2024

View reviewed changes

builder/azure/arm/step_deploy_template.go Outdated Show resolved Hide resolved

JenGoldstrich force-pushed the fix_storage_account_delete_and_tidy_deletion_process branch from 0eebcbe to a542433 Compare April 29, 2024 16:01

JenGoldstrich changed the title ~~fix: [arm builder] storage blob deletion wrong params, cleanup post build deletion~~ fix: [arm builder] storage blob VHD deletion, cleanup post build deletion Apr 29, 2024

Refactor Cleanup of deployment to reduce extra errors

8090c68

JenGoldstrich force-pushed the fix_storage_account_delete_and_tidy_deletion_process branch from 9ed6f31 to 8090c68 Compare April 29, 2024 20:44

JenGoldstrich merged commit d474ba5 into main Apr 29, 2024
12 checks passed

JenGoldstrich deleted the fix_storage_account_delete_and_tidy_deletion_process branch April 29, 2024 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [arm builder] storage blob VHD deletion, cleanup post build deletion #410

fix: [arm builder] storage blob VHD deletion, cleanup post build deletion #410

JenGoldstrich commented Apr 17, 2024 •

edited

Loading

JenGoldstrich Apr 17, 2024

JenGoldstrich Apr 17, 2024

JenGoldstrich Apr 17, 2024

JenGoldstrich Apr 20, 2024

lbajolet-hashicorp Apr 29, 2024

lbajolet-hashicorp left a comment

fix: [arm builder] storage blob VHD deletion, cleanup post build deletion #410

fix: [arm builder] storage blob VHD deletion, cleanup post build deletion #410

Conversation

JenGoldstrich commented Apr 17, 2024 • edited Loading

JenGoldstrich Apr 17, 2024

Choose a reason for hiding this comment

JenGoldstrich Apr 17, 2024

Choose a reason for hiding this comment

JenGoldstrich Apr 17, 2024

Choose a reason for hiding this comment

JenGoldstrich Apr 20, 2024

Choose a reason for hiding this comment

lbajolet-hashicorp Apr 29, 2024

Choose a reason for hiding this comment

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

JenGoldstrich commented Apr 17, 2024 •

edited

Loading