Skip to content
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

Deploy template error handling cleanup #296

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Deploy template error handling cleanup #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7ea378
Seperate cleanup for Virtual Machines and KeyVaults to reduce noise d…
JenGoldstrich Apr 21, 2023
cab8d84
Add isVirtualMachineDeployment param to existing tests
JenGoldstrich Apr 21, 2023
996058f
implement Wilken's feedback
JenGoldstrich Apr 25, 2023
6803fea
Not sure why this change is generated by Mac
JenGoldstrich Apr 25, 2023
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
6 changes: 3 additions & 3 deletions builder/azure/arm/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
},
NewStepCreateResourceGroup(azureClient, ui),
NewStepValidateTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment, VirtualMachineTemplate),
NewStepGetIPAddress(azureClient, ui, endpointConnectType),
&communicator.StepConnectSSH{
Config: &b.config.Comm,
Expand Down Expand Up @@ -257,7 +257,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
keyVaultDeploymentName := b.stateBag.Get(constants.ArmKeyVaultDeploymentName).(string)
steps = append(steps,
NewStepValidateTemplate(azureClient, ui, &b.config, keyVaultDeploymentName, GetCommunicatorSpecificKeyVaultDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, keyVaultDeploymentName, GetCommunicatorSpecificKeyVaultDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, keyVaultDeploymentName, GetCommunicatorSpecificKeyVaultDeployment, KeyVaultTemplate),
)
} else if b.config.Comm.Type == "winrm" {
steps = append(steps, NewStepCertificateInKeyVault(&azureClient.VaultClient, ui, &b.config, b.config.winrmCertificate))
Expand All @@ -281,7 +281,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook)
NewStepGetCertificate(azureClient, ui),
NewStepSetCertificate(&b.config, ui),
NewStepValidateTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment),
NewStepDeployTemplate(azureClient, ui, &b.config, deploymentName, GetVirtualMachineDeployment, VirtualMachineTemplate),
NewStepGetIPAddress(azureClient, ui, endpointConnectType),
)

Expand Down
112 changes: 64 additions & 48 deletions builder/azure/arm/step_deploy_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ import (
"github.com/hashicorp/packer-plugin-sdk/retry"
)

type DeploymentTemplateType int

const (
VirtualMachineTemplate DeploymentTemplateType = iota
KeyVaultTemplate
)

type StepDeployTemplate struct {
client *AzureClient
deploy func(ctx context.Context, resourceGroupName string, deploymentName string) error
Expand All @@ -30,16 +37,18 @@ type StepDeployTemplate struct {
config *Config
factory templateFactoryFunc
name string
templateType DeploymentTemplateType
}

func NewStepDeployTemplate(client *AzureClient, ui packersdk.Ui, config *Config, deploymentName string, factory templateFactoryFunc) *StepDeployTemplate {
func NewStepDeployTemplate(client *AzureClient, ui packersdk.Ui, config *Config, deploymentName string, factory templateFactoryFunc, templateType DeploymentTemplateType) *StepDeployTemplate {
var step = &StepDeployTemplate{
client: client,
say: func(message string) { ui.Say(message) },
error: func(e error) { ui.Error(e.Error()) },
config: config,
factory: factory,
name: deploymentName,
client: client,
say: func(message string) { ui.Say(message) },
error: func(e error) { ui.Error(e.Error()) },
config: config,
factory: factory,
name: deploymentName,
templateType: templateType,
}

step.deploy = step.deployTemplate
Expand Down Expand Up @@ -71,52 +80,59 @@ func (s *StepDeployTemplate) Cleanup(state multistep.StateBag) {
}()

ui := state.Get("ui").(packersdk.Ui)
ui.Say("\nDeleting individual resources ...")

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)
isManagedDisk := state.Get(constants.ArmIsManagedImage).(bool)
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.delete(context.TODO(), deploymentName, resourceGroupName)
if err != nil {
s.reportIfError(err, resourceGroupName)
}
if s.templateType == KeyVaultTemplate {
ui.Say("\nDeleting KeyVault created during build")
err := s.delete(context.TODO(), deploymentName, resourceGroupName)
if err != nil {
s.reportIfError(err, resourceGroupName)
}

// The disk was not found on the VM, this is an error.
if imageType == "" && imageName == "" {
ui.Error(fmt.Sprintf("Failed to find temporary OS disk on VM. Please delete manually.\n\n"+
"VM Name: %s\n"+
"Error: %s", computeName, err))
return
}
if !state.Get(constants.ArmKeepOSDisk).(bool) {
ui.Say(fmt.Sprintf(" Deleting -> %s : '%s'", imageType, imageName))
err = s.deleteDisk(context.TODO(), imageName, resourceGroupName, isManagedDisk)
} else {
ui.Say("\nDeleting Virtual Machine deployment and its attatched resources...")
// 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)
isManagedDisk := state.Get(constants.ArmIsManagedImage).(bool)
imageType, imageName, err := s.disk(context.TODO(), resourceGroupName, computeName)
if err != nil {
ui.Error(fmt.Sprintf("Error deleting resource. Please delete manually.\n\n"+
"Name: %s\n"+
"Error: %s", imageName, err))
ui.Error(fmt.Sprintf("Could not retrieve OS Image details: %s", err))
}
err = s.delete(context.TODO(), deploymentName, resourceGroupName)
if err != nil {
s.reportIfError(err, resourceGroupName)
}
// The disk was not found on the VM, this is an error.
if imageType == "" && imageName == "" {
ui.Error(fmt.Sprintf("Failed to find temporary OS disk on VM. Please delete manually.\n\n"+
"VM Name: %s\n"+
"Error: %s", computeName, err))
return
}
if !state.Get(constants.ArmKeepOSDisk).(bool) {
ui.Say(fmt.Sprintf(" Deleting -> %s : '%s'", imageType, imageName))
err = s.deleteDisk(context.TODO(), imageName, resourceGroupName, isManagedDisk)
if err != nil {
ui.Error(fmt.Sprintf("Error deleting resource. Please delete manually.\n\n"+
"Name: %s\n"+
"Error: %s", imageName, err))
}
}
}

var dataDisks []string
if disks := state.Get(constants.ArmAdditionalDiskVhds); disks != nil {
dataDisks = disks.([]string)
}
for i, additionaldisk := range dataDisks {
s.say(fmt.Sprintf(" Deleting Additional Disk -> %d: '%s'", i+1, additionaldisk))
var dataDisks []string
if disks := state.Get(constants.ArmAdditionalDiskVhds); disks != nil {
dataDisks = disks.([]string)
}
for i, additionaldisk := range dataDisks {
s.say(fmt.Sprintf(" Deleting Additional Disk -> %d: '%s'", i+1, additionaldisk))

err := s.deleteImage(context.TODO(), additionaldisk, resourceGroupName, isManagedDisk)
if err != nil {
s.say("Failed to delete the managed Additional Disk!")
err := s.deleteImage(context.TODO(), additionaldisk, resourceGroupName, isManagedDisk)
if err != nil {
s.say("Failed to delete the managed Additional Disk!")
}
}

}
}

Expand Down Expand Up @@ -299,9 +315,9 @@ func (s *StepDeployTemplate) deleteDeploymentResources(ctx context.Context, depl
resourceName,
resourceGroupName)
if err != nil {
s.say(fmt.Sprintf("Error deleting resource. Will retry.\n"+
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We regularly fail to delete attached network resources related to the resource being still attached, this leads to a lot of spam in the output, here I've changed this to only log the error when we fail for real, since almost every build I run fails this check at least once for at least two different resources.

"Name: %s\n"+
"Error: %s\n", resourceName, err.Error()))
s.say(fmt.Sprintf("Couldn't delete %s resource. Will retry.\n"+
"Name: %s",
resourceType, resourceName))
}
return err
})
Expand Down
50 changes: 36 additions & 14 deletions builder/azure/arm/step_deploy_template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,10 @@ func TestStepDeployTemplateShouldTakeStepArgumentsFromStateBag(t *testing.T) {

return nil
},
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
templateType: VirtualMachineTemplate,
}

stateBag := createTestStateBagStepValidateTemplate()
Expand All @@ -89,9 +90,10 @@ func TestStepDeployTemplateShouldTakeStepArgumentsFromStateBag(t *testing.T) {

func TestStepDeployTemplateDeleteImageShouldFailWhenImageUrlCannotBeParsed(t *testing.T) {
var testSubject = &StepDeployTemplate{
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
templateType: VirtualMachineTemplate,
}
// Invalid URL per https://golang.org/src/net/url/url_test.go
err := testSubject.deleteImage(context.TODO(), "http://[fe80::1%en0]/", "Unit Test: ResourceGroupName", false)
Expand All @@ -102,9 +104,10 @@ func TestStepDeployTemplateDeleteImageShouldFailWhenImageUrlCannotBeParsed(t *te

func TestStepDeployTemplateDeleteImageShouldFailWithInvalidImage(t *testing.T) {
var testSubject = &StepDeployTemplate{
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
say: func(message string) {},
error: func(e error) {},
name: "--deployment-name--",
templateType: VirtualMachineTemplate,
}
err := testSubject.deleteImage(context.TODO(), "storage.blob.core.windows.net/abc", "Unit Test: ResourceGroupName", false)
if err == nil {
Expand All @@ -114,7 +117,7 @@ func TestStepDeployTemplateDeleteImageShouldFailWithInvalidImage(t *testing.T) {

func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInExistingResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, true)
Expand All @@ -132,7 +135,7 @@ func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInExistingResourceGr

func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInTemporaryResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, true)
Expand All @@ -150,7 +153,7 @@ func TestStepDeployTemplateCleanupShouldDeleteManagedOSImageInTemporaryResourceG

func TestStepDeployTemplateCleanupShouldDeleteVHDOSImageInExistingResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, false)
Expand All @@ -168,7 +171,7 @@ func TestStepDeployTemplateCleanupShouldDeleteVHDOSImageInExistingResourceGroup(

func TestStepDeployTemplateCleanupShouldVHDOSImageInTemporaryResourceGroup(t *testing.T) {
var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter)
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, VirtualMachineTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, false)
Expand All @@ -184,6 +187,24 @@ func TestStepDeployTemplateCleanupShouldVHDOSImageInTemporaryResourceGroup(t *te
}
}

func TestStepDeployTemplateCleanupShouldNotDeleteDiskForKeyVaultDeployments(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test

var deleteDiskCounter = 0
var testSubject = createTestStepDeployTemplateDeleteOSImage(&deleteDiskCounter, KeyVaultTemplate)

stateBag := createTestStateBagStepDeployTemplate()
stateBag.Put(constants.ArmIsManagedImage, false)
stateBag.Put(constants.ArmIsSIGImage, false)
stateBag.Put(constants.ArmIsExistingResourceGroup, true)
stateBag.Put(constants.ArmIsResourceGroupCreated, true)
stateBag.Put(constants.ArmKeepOSDisk, false)
stateBag.Put("ui", packersdk.TestUi(t))

testSubject.Cleanup(stateBag)
if deleteDiskCounter != 0 {
t.Fatalf("Expected DeployTemplate Cleanup to not invoke deleteDisk, but invoked %d times", deleteDiskCounter)
}
}

func createTestStateBagStepDeployTemplate() multistep.StateBag {
stateBag := new(multistep.BasicStateBag)

Expand All @@ -194,7 +215,7 @@ func createTestStateBagStepDeployTemplate() multistep.StateBag {
return stateBag
}

func createTestStepDeployTemplateDeleteOSImage(deleteDiskCounter *int) *StepDeployTemplate {
func createTestStepDeployTemplateDeleteOSImage(deleteDiskCounter *int, templateType DeploymentTemplateType) *StepDeployTemplate {
return &StepDeployTemplate{
deploy: func(context.Context, string, string) error { return nil },
say: func(message string) {},
Expand All @@ -212,5 +233,6 @@ func createTestStepDeployTemplateDeleteOSImage(deleteDiskCounter *int) *StepDepl
deleteDeployment: func(ctx context.Context, state multistep.StateBag) error {
return nil
},
templateType: templateType,
}
}
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 @@ -378,7 +378,7 @@
[Windows](https://learn.microsoft.com/en-us/azure/virtual-machines/windows/hybrid-use-benefit-licensing)
or
[Linux](https://learn.microsoft.com/en-us/azure/virtual-machines/linux/azure-hybrid-benefit-linux)

- `secure_boot_enabled` (bool) - Specifies if Secure Boot and Trusted Launch is enabled for the Virtual Machine.

- `vtpm_enabled` (bool) - Specifies if vTPM (virtual Trusted Platform Module) and Trusted Launch is enabled for the Virtual Machine.
Expand Down