Skip to content

VHD Migrations Fixes - VHD Acceptance Tests, and Disk Revoke Access on Failures #534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tanmay-hc merged 1 commit into from
Aug 29, 2025
Merged

VHD Migrations Fixes - VHD Acceptance Tests, and Disk Revoke Access on Failures #534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .web-docs/components/builder/arm/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ Providing `temp_resource_group_name` or `location` in combination with

- `capture_name_prefix` (string) - VHD prefix.

- `capture_container_name` (string) - Destination container name.
- `capture_container_name` (string) - Destination container name. This must be created before the build in the storage account

- `shared_image_gallery` (SharedImageGallery) - Use a [Shared Gallery
image](https://azure.microsoft.com/en-us/blog/announcing-the-public-preview-of-shared-image-gallery/)
Expand Down
6 changes: 3 additions & 3 deletions builder/azure/arm/builder_acc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ func testBuilderUserDataLinux(userdata string) string {

"storage_account": "{{user `+"`storage_account`"+`}}",
"resource_group_name": "{{user `+"`resource_group_name`"+`}}",
"capture_container_name": "test",
"capture_container_name": "packeracc",
"capture_name_prefix": "testBuilderUserDataLinux",

"os_type": "Linux",
Expand Down Expand Up @@ -528,7 +528,7 @@ const testBuilderAccBlobWindows = `

"storage_account": "{{user ` + "`storage_account`" + `}}",
"resource_group_name": "{{user ` + "`resource_group_name`" + `}}",
"capture_container_name": "azure-arm",
"capture_container_name": "packeracc",
"capture_name_prefix": "testBuilderAccBlobWin",

"os_type": "Windows",
Expand Down Expand Up @@ -566,7 +566,7 @@ const testBuilderAccBlobLinux = `

"storage_account": "{{user ` + "`storage_account`" + `}}",
"resource_group_name": "{{user ` + "`resource_group_name`" + `}}",
"capture_container_name": "test",
"capture_container_name": "packeracc",
"capture_name_prefix": "testBuilderAccBlobLinux",

"os_type": "Linux",
Expand Down
2 changes: 1 addition & 1 deletion builder/azure/arm/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ type Config struct {

// VHD prefix.
CaptureNamePrefix string `mapstructure:"capture_name_prefix"`
// Destination container name.
// Destination container name. This must be created before the build in the storage account
CaptureContainerName string `mapstructure:"capture_container_name"`
// Use a [Shared Gallery
// image](https://azure.microsoft.com/en-us/blog/announcing-the-public-preview-of-shared-image-gallery/)
Expand Down
40 changes: 22 additions & 18 deletions builder/azure/arm/step_capture_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,17 @@ import (
)

type StepCaptureImage struct {
client *AzureClient
config *Config
generalizeVM func(ctx context.Context, vmId virtualmachines.VirtualMachineId) error
getVMInternalID func(ctx context.Context, vmId virtualmachines.VirtualMachineId) (string, error)
captureManagedImage func(ctx context.Context, subscriptionId string, resourceGroupName string, imageName string, parameters *images.Image) error
grantAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) (string, error)
revokeAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) error
copyToStorage func(ctx context.Context, storageContainerName string, captureNamePrefix string, osDiskName string, accessUri string) error
say func(message string)
error func(e error)
client *AzureClient
config *Config
generalizeVM func(ctx context.Context, vmId virtualmachines.VirtualMachineId) error
getVMInternalID func(ctx context.Context, vmId virtualmachines.VirtualMachineId) (string, error)
captureManagedImage func(ctx context.Context, subscriptionId string, resourceGroupName string, imageName string, parameters *images.Image) error
grantAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) (string, error)
revokeAccess func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) error
copyToStorage func(ctx context.Context, storageContainerName string, captureNamePrefix string, osDiskName string, accessUri string) error
say func(message string)
error func(e error)
diskNameToRevokeAccessTo string
}

type GrantAccessOperationResponse struct {
Expand Down Expand Up @@ -212,7 +213,15 @@ func (s *StepCaptureImage) Run(ctx context.Context, state multistep.StateBag) mu
return multistep.ActionContinue
}

func (*StepCaptureImage) Cleanup(multistep.StateBag) {
func (s *StepCaptureImage) Cleanup(state multistep.StateBag) {
Copy link
Contributor Author

@JenGoldstrich JenGoldstrich Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change, when control + cing mid copy (a common thing users may do as the copy action takes a long time, and is prone to fail from time to time)

Cancelling build after receiving interrupt
==> azure-arm: failed to capture OS Disk with err : failed to copy to storage with err : error copying: waiting for file to copy: context canceled
==> azure-arm: Provisioning step had errors: Running the cleanup provisioner, if present...
==> azure-arm:
==> azure-arm: Deleting Virtual Machine deployment and its attached resources...
==> azure-arm: Deleted -> Microsoft.Compute/virtualMachines : 'pkrvmblsda6b8r9'
==> azure-arm: Deleted -> Microsoft.Network/networkInterfaces : 'pkrniblsda6b8r9'
==> azure-arm: Deleted -> Microsoft.Network/virtualNetworks : 'pkrvnblsda6b8r9'
==> azure-arm: Deleted -> Microsoft.Network/publicIPAddresses : 'pkripblsda6b8r9'
==> azure-arm: Deleted -> Microsoft.Network/networkSecurityGroups : 'pkrsgblsda6b8r9'
==> azure-arm: Error deleting resource. Please delete manually.
==> azure-arm:
==> azure-arm: Name: /subscriptions/4712c895-8c02-49ab-b48c-9a3396eb6a78/resourceGroups/pkr-Resource-Group-blsda6b8r9/providers/Microsoft.Compute/disks/pkrosblsda6b8r9
==> azure-arm: Error: performing Delete: unexpected status 409 (409 Conflict) with error: OperationNotAllowed: There is an active shared access signature outstanding for disk pkrosblsda6b8r9. Call EndGetAccess before attaching or deleting the disk. Learn more here: aka.ms/revokeaccessapi.
==> azure-arm: performing Delete: unexpected status 409 (409 Conflict) with error: OperationNotAllowed: There is an active shared access signature outstanding for disk pkrosblsda6b8r9. Call EndGetAccess before attaching or deleting the disk. Learn more here: aka.ms/revokeaccessapi.
==> azure-arm: Removing the created Deployment object: 'pkrdpblsda6b8r9'
==> azure-arm:
==> azure-arm: Deleting KeyVault created during build
==> azure-arm: Deleted -> Microsoft.KeyVault/vaults : 'pkrkvblsda6b8r9'
==> azure-arm: Removing the created Deployment object: 'kvpkrdpblsda6b8r9'
==> azure-arm:
==> azure-arm: Cleanup requested, deleting resource group ...

And then the resource group does not delete.

vs after:

Cancelling build after receiving interrupt
==> azure-arm: failed to capture OS Disk with err : failed to copy to storage with err : error copying: waiting for file to copy: context canceled
==> azure-arm: Revoking access ...
==> azure-arm: Provisioning step had errors: Running the cleanup provisioner, if present...
==> azure-arm:
==> azure-arm: Deleting Virtual Machine deployment and its attached resources...
==> azure-arm: Deleted -> Microsoft.Compute/virtualMachines : 'pkrvmuyw5ff2drx'
==> azure-arm: Deleted -> Microsoft.Network/networkInterfaces : 'pkrniuyw5ff2drx'
==> azure-arm: Deleted -> Microsoft.Network/virtualNetworks : 'pkrvnuyw5ff2drx'
==> azure-arm: Deleted -> Microsoft.Network/publicIPAddresses : 'pkripuyw5ff2drx'
==> azure-arm: Deleted -> Microsoft.Network/networkSecurityGroups : 'pkrsguyw5ff2drx'
==> azure-arm: Deleted -> Microsoft.Compute/disks : '/subscriptions/4712c895-8c02-49ab-b48c-9a3396eb6a78/resourceGroups/pkr-Resource-Group-uyw5ff2drx/providers/Microsoft.Compute/disks/pkrosuyw5ff2drx'
==> azure-arm: Removing the created Deployment object: 'pkrdpuyw5ff2drx'
==> azure-arm:
==> azure-arm: Deleting KeyVault created during build
==> azure-arm: Deleted -> Microsoft.KeyVault/vaults : 'pkrkvuyw5ff2drx'
==> azure-arm: Removing the created Deployment object: 'kvpkrdpuyw5ff2drx'
==> azure-arm:
==> azure-arm: Cleanup requested, deleting resource group ...
==> azure-arm: Resource group has been deleted.
Build 'azure-arm' errored after 3 minutes 55 seconds: failed to capture OS Disk with err : failed to copy to storage with err : error copying: waiting for file to copy: context canceled

if s.diskNameToRevokeAccessTo != "" {
var subscriptionId = state.Get(constants.ArmSubscription).(string)
var resourceGroupName = state.Get(constants.ArmResourceGroupName).(string)
err := s.revokeAccess(context.Background(), subscriptionId, resourceGroupName, s.diskNameToRevokeAccessTo)
if err != nil {
state.Get("ui").(packersdk.Ui).Errorf("Failed to revoke access to disk, this will lead to failures cleaning up resources. Err: %s", err.Error())
}
}
}

func (s *StepCaptureImage) captureVHD(ctx context.Context, subscriptionId string, resourceGroupName string, diskName string) error {
Expand All @@ -221,7 +230,8 @@ func (s *StepCaptureImage) captureVHD(ctx context.Context, subscriptionId string
err = fmt.Errorf("failed to grant access with err : %s", err)
return err
}

// If the code is canceled or fails after this point, we must call to cleanup this granted access, otherwise delete operations will fail
s.diskNameToRevokeAccessTo = s.config.tmpOSDiskName
s.say(fmt.Sprintf(" -> accessUri : '%s'", accessUri))

var storageContainerName = s.config.CaptureContainerName
Expand All @@ -233,12 +243,6 @@ func (s *StepCaptureImage) captureVHD(ctx context.Context, subscriptionId string
return err
}

err = s.revokeAccess(ctx, subscriptionId, resourceGroupName, diskName)
if err != nil {
err = fmt.Errorf("failed to revoke access with err : %s", err)
return err
}

return nil
}

Expand Down
32 changes: 12 additions & 20 deletions builder/azure/arm/step_capture_image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/hashicorp/go-azure-sdk/resource-manager/compute/2022-03-01/images"
"github.com/hashicorp/go-azure-sdk/resource-manager/compute/2022-03-01/virtualmachines"
"github.com/hashicorp/packer-plugin-azure/builder/azure/common"
"github.com/hashicorp/packer-plugin-azure/builder/azure/common/constants"
"github.com/hashicorp/packer-plugin-sdk/multistep"
)
Expand Down Expand Up @@ -110,26 +111,16 @@ func TestStepCaptureImageShouldFailIfCopyToStorageFails(t *testing.T) {
}
}

func TestStepCaptureImageShouldFailIfRevokeAccessFails(t *testing.T) {
func TestStepCaptureImageShouldRevokeAccessOnCleanup(t *testing.T) {
revokeAccessCalled := false
revokeAccessPtr := &revokeAccessCalled
var testSubject = &StepCaptureImage{
config: &Config{
tmpOSDiskName: "tmpOSDiskName",
CaptureContainerName: "CaptureContainerName",
CaptureNamePrefix: "CaptureNamePrefix",
},
grantAccess: func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) (string, error) {
return "accessuri", nil
},
copyToStorage: func(ctx context.Context, storageContainerName string, captureNamePrefix string, osDiskName string, accessUri string) error {
return nil
},
revokeAccess: func(ctx context.Context, subscriptionId string, resourceGroupName string, osDiskName string) error {
return fmt.Errorf("!! Unit Test FAIL !!")
},
getVMInternalID: func(context.Context, virtualmachines.VirtualMachineId) (string, error) {
return "id", nil
},
generalizeVM: func(context.Context, virtualmachines.VirtualMachineId) error {
revokeAccessPtr = common.BoolPtr(true)
return nil
},
say: func(message string) {},
Expand All @@ -138,13 +129,14 @@ func TestStepCaptureImageShouldFailIfRevokeAccessFails(t *testing.T) {

stateBag := createTestStateBagStepCaptureImage()

var result = testSubject.Run(context.Background(), stateBag)
if result != multistep.ActionHalt {
t.Fatalf("Expected the step to return 'ActionHalt', but got '%d'.", result)
testSubject.Cleanup(stateBag)
if *revokeAccessPtr {
t.Fatal("Revoke Access should not be called on TestCaptureImage.Cleanup when a tmpOSDiskName is set, but was")
}

if _, ok := stateBag.GetOk(constants.Error); ok == false {
t.Fatalf("Expected the step to set stateBag['%s'], but it was not.", constants.Error)
testSubject.diskNameToRevokeAccessTo = "tmpOSDiskName"
testSubject.Cleanup(stateBag)
if !*revokeAccessPtr {
t.Fatal("Revoke Access should be called on TestCaptureImage.Cleanup when a tmpOSDiskName is set, but wasn't")
}
}

Expand Down
2 changes: 1 addition & 1 deletion docs-partials/builder/azure/arm/Config-not-required.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

- `capture_name_prefix` (string) - VHD prefix.

- `capture_container_name` (string) - Destination container name.
- `capture_container_name` (string) - Destination container name. This must be created before the build in the storage account

- `shared_image_gallery` (SharedImageGallery) - Use a [Shared Gallery
image](https://azure.microsoft.com/en-us/blog/announcing-the-public-preview-of-shared-image-gallery/)
Expand Down
5 changes: 5 additions & 0 deletions terraform/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ resource "azurerm_storage_account" "storage-account" {
account_replication_type = "GRS"
}

resource "azurerm_storage_container" "example" {
name = "packeracc"
storage_account_id = azurerm_storage_account.storage-account.id
}

resource "azurerm_shared_image_gallery" "gallery" {
name = "${var.resource_prefix}_acctestgallery"
resource_group_name = azurerm_resource_group.rg.name
Expand Down
Loading