From c7d8f4e1505df459f0119e1eab2edc1c9b272765 Mon Sep 17 00:00:00 2001 From: Christopher Boumenot Date: Fri, 30 Nov 2018 12:32:41 -0800 Subject: [PATCH] azure: configuration for disk caching Export a configuration knob to change the disk caching setting. The default value remains ReadWrite. This seems the most appropriate value given Packer. Certain disk sizes require that disk caching be disable, and this knob allows the user to do just that. --- builder/azure/arm/config.go | 19 ++++++- builder/azure/arm/config_test.go | 55 +++++++++++++++++++ builder/azure/arm/template_factory.go | 13 +++-- .../azure/common/template/template_builder.go | 26 +++++---- ...est.TestSharedImageGallery00.approved.json | 2 +- .../common/template/template_builder_test.go | 16 +++--- website/source/docs/builders/azure.html.md | 3 + 7 files changed, 107 insertions(+), 27 deletions(-) diff --git a/builder/azure/arm/config.go b/builder/azure/arm/config.go index d3b7aaa95..70246b319 100644 --- a/builder/azure/arm/config.go +++ b/builder/azure/arm/config.go @@ -109,9 +109,9 @@ type Config struct { ManagedImageResourceGroupName string `mapstructure:"managed_image_resource_group_name"` ManagedImageName string `mapstructure:"managed_image_name"` ManagedImageStorageAccountType string `mapstructure:"managed_image_storage_account_type"` + managedImageStorageAccountType compute.StorageAccountTypes ManagedImageOSDiskSnapshotName string `mapstructure:"managed_image_os_disk_snapshot_name"` ManagedImageDataDiskSnapshotPrefix string `mapstructure:"managed_image_data_disk_snapshot_prefix"` - managedImageStorageAccountType compute.StorageAccountTypes manageImageLocation string // Deployment @@ -138,6 +138,8 @@ type Config struct { // Additional Disks AdditionalDiskSize []int32 `mapstructure:"disk_additional_size"` + DiskCachingType string `mapstructure:"disk_caching_type"` + diskCachingType compute.CachingTypes // Runtime Values UserName string @@ -471,6 +473,10 @@ func provideDefaultValues(c *Config) { c.managedImageStorageAccountType = compute.StorageAccountTypesStandardLRS } + if c.DiskCachingType == "" { + c.diskCachingType = compute.CachingTypesReadWrite + } + if c.ImagePublisher != "" && c.ImageVersion == "" { c.ImageVersion = DefaultImageVersion } @@ -745,6 +751,17 @@ func assertRequiredParametersSet(c *Config, errs *packer.MultiError) { default: errs = packer.MultiErrorAppend(errs, fmt.Errorf("The managed_image_storage_account_type %q is invalid", c.ManagedImageStorageAccountType)) } + + switch c.DiskCachingType { + case string(compute.CachingTypesNone): + c.diskCachingType = compute.CachingTypesNone + case string(compute.CachingTypesReadOnly): + c.diskCachingType = compute.CachingTypesReadOnly + case "", string(compute.CachingTypesReadWrite): + c.diskCachingType = compute.CachingTypesReadWrite + default: + errs = packer.MultiErrorAppend(errs, fmt.Errorf("The disk_caching_type %q is invalid", c.DiskCachingType)) + } } func assertManagedImageName(name, setting string) (bool, error) { diff --git a/builder/azure/arm/config_test.go b/builder/azure/arm/config_test.go index d2acdd20b..4b3a7796c 100644 --- a/builder/azure/arm/config_test.go +++ b/builder/azure/arm/config_test.go @@ -48,6 +48,10 @@ func TestConfigShouldProvideReasonableDefaultValues(t *testing.T) { if c.managedImageStorageAccountType == "" { t.Errorf("Expected 'managedImageStorageAccountType' to be populated, but it was empty!") } + + if c.diskCachingType == "" { + t.Errorf("Expected 'diskCachingType' to be populated, but it was empty!") + } } func TestConfigShouldBeAbleToOverrideDefaultedValues(t *testing.T) { @@ -57,6 +61,7 @@ func TestConfigShouldBeAbleToOverrideDefaultedValues(t *testing.T) { builderValues["vm_size"] = "override_vm_size" builderValues["communicator"] = "ssh" builderValues["managed_image_storage_account_type"] = "Premium_LRS" + builderValues["disk_caching_type"] = "None" c, _, err := newConfig(builderValues, getPackerConfiguration()) @@ -87,6 +92,10 @@ func TestConfigShouldBeAbleToOverrideDefaultedValues(t *testing.T) { if c.managedImageStorageAccountType != compute.StorageAccountTypesPremiumLRS { t.Errorf("Expected 'managed_image_storage_account_type' to be set to 'Premium_LRS', but found %q!", c.managedImageStorageAccountType) } + + if c.diskCachingType != compute.CachingTypesNone { + t.Errorf("Expected 'disk_caching_type' to be set to 'None', but found %q!", c.diskCachingType) + } } func TestConfigShouldDefaultVMSizeToStandardA1(t *testing.T) { @@ -1165,6 +1174,27 @@ func TestConfigShouldRejectMalformedManageImageStorageAccountTypes(t *testing.T) } } +func TestConfigShouldRejectMalformedDiskCachingType(t *testing.T) { + config := map[string]interface{}{ + "custom_managed_image_resource_group_name": "ignore", + "custom_managed_image_name": "ignore", + "location": "ignore", + "subscription_id": "ignore", + "communicator": "none", + "managed_image_resource_group_name": "ignore", + "managed_image_name": "ignore", + "disk_caching_type": "--invalid--", + + // Does not matter for this test case, just pick one. + "os_type": constants.Target_Linux, + } + + _, _, err := newConfig(config, getPackerConfiguration()) + if err == nil { + t.Fatal("expected config to reject custom and platform input for a managed image build") + } +} + func TestConfigShouldAcceptManagedImageStorageAccountTypes(t *testing.T) { config := map[string]interface{}{ "custom_managed_image_resource_group_name": "ignore", @@ -1190,6 +1220,31 @@ func TestConfigShouldAcceptManagedImageStorageAccountTypes(t *testing.T) { } } +func TestConfigShouldAcceptDiskCachingTypes(t *testing.T) { + config := map[string]interface{}{ + "custom_managed_image_resource_group_name": "ignore", + "custom_managed_image_name": "ignore", + "location": "ignore", + "subscription_id": "ignore", + "communicator": "none", + "managed_image_resource_group_name": "ignore", + "managed_image_name": "ignore", + + // Does not matter for this test case, just pick one. + "os_type": constants.Target_Linux, + } + + storage_account_types := []string{"None", "ReadOnly", "ReadWrite"} + + for _, x := range storage_account_types { + config["disk_caching_type"] = x + _, _, err := newConfig(config, getPackerConfiguration()) + if err != nil { + t.Fatalf("expected config to accept a disk_caching_type of %q", x) + } + } +} + func TestConfigShouldRejectTempAndBuildResourceGroupName(t *testing.T) { config := map[string]interface{}{ "capture_name_prefix": "ignore", diff --git a/builder/azure/arm/template_factory.go b/builder/azure/arm/template_factory.go index 00873e457..6a2bf3d2a 100644 --- a/builder/azure/arm/template_factory.go +++ b/builder/azure/arm/template_factory.go @@ -59,9 +59,9 @@ func GetVirtualMachineDeployment(config *Config) (*resources.Deployment, error) } if config.ImageUrl != "" { - builder.SetImageUrl(config.ImageUrl, osType) + builder.SetImageUrl(config.ImageUrl, osType, config.diskCachingType) } else if config.CustomManagedImageName != "" { - builder.SetManagedDiskUrl(config.customManagedImageID, config.managedImageStorageAccountType) + builder.SetManagedDiskUrl(config.customManagedImageID, config.managedImageStorageAccountType, config.diskCachingType) } else if config.ManagedImageName != "" && config.ImagePublisher != "" { imageID := fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Compute/locations/%s/publishers/%s/ArtifactTypes/vmimage/offers/%s/skus/%s/versions/%s", config.SubscriptionID, @@ -71,7 +71,7 @@ func GetVirtualMachineDeployment(config *Config) (*resources.Deployment, error) config.ImageSku, config.ImageVersion) - builder.SetManagedMarketplaceImage(config.Location, config.ImagePublisher, config.ImageOffer, config.ImageSku, config.ImageVersion, imageID, config.managedImageStorageAccountType) + builder.SetManagedMarketplaceImage(config.Location, config.ImagePublisher, config.ImageOffer, config.ImageSku, config.ImageVersion, imageID, config.managedImageStorageAccountType, config.diskCachingType) } else if config.SharedGallery.Subscription != "" { imageID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/galleries/%s/images/%s", config.SharedGallery.Subscription, @@ -83,9 +83,9 @@ func GetVirtualMachineDeployment(config *Config) (*resources.Deployment, error) config.SharedGallery.ImageVersion) } - builder.SetSharedGalleryImage(config.Location, imageID) + builder.SetSharedGalleryImage(config.Location, imageID, config.diskCachingType) } else { - builder.SetMarketPlaceImage(config.ImagePublisher, config.ImageOffer, config.ImageSku, config.ImageVersion) + builder.SetMarketPlaceImage(config.ImagePublisher, config.ImageOffer, config.ImageSku, config.ImageVersion, config.diskCachingType) } if config.OSDiskSizeGB > 0 { @@ -93,7 +93,8 @@ func GetVirtualMachineDeployment(config *Config) (*resources.Deployment, error) } if len(config.AdditionalDiskSize) > 0 { - builder.SetAdditionalDisks(config.AdditionalDiskSize, config.CustomManagedImageName != "" || (config.ManagedImageName != "" && config.ImagePublisher != "")) + isManaged := config.CustomManagedImageName != "" || (config.ManagedImageName != "" && config.ImagePublisher != "") + builder.SetAdditionalDisks(config.AdditionalDiskSize, isManaged, config.diskCachingType) } if config.customData != "" { diff --git a/builder/azure/common/template/template_builder.go b/builder/azure/common/template/template_builder.go index a9455b81e..188fdaf7b 100644 --- a/builder/azure/common/template/template_builder.go +++ b/builder/azure/common/template/template_builder.go @@ -14,7 +14,6 @@ const ( jsonIndent = " " resourceKeyVaults = "Microsoft.KeyVault/vaults" - resourceManagedDisk = "Microsoft.Compute/images" resourceNetworkInterfaces = "Microsoft.Network/networkInterfaces" resourcePublicIPAddresses = "Microsoft.Network/publicIPAddresses" resourceVirtualMachine = "Microsoft.Compute/virtualMachines" @@ -101,7 +100,7 @@ func (s *TemplateBuilder) BuildWindows(keyVaultName, winRMCertificateUrl string) return nil } -func (s *TemplateBuilder) SetManagedDiskUrl(managedImageId string, storageAccountType compute.StorageAccountTypes) error { +func (s *TemplateBuilder) SetManagedDiskUrl(managedImageId string, storageAccountType compute.StorageAccountTypes, cachingType compute.CachingTypes) error { resource, err := s.getResourceByType(resourceVirtualMachine) if err != nil { return err @@ -114,6 +113,7 @@ func (s *TemplateBuilder) SetManagedDiskUrl(managedImageId string, storageAccoun profile.OsDisk.OsType = s.osType profile.OsDisk.CreateOption = compute.DiskCreateOptionTypesFromImage profile.OsDisk.Vhd = nil + profile.OsDisk.Caching = cachingType profile.OsDisk.ManagedDisk = &compute.ManagedDiskParameters{ StorageAccountType: storageAccountType, } @@ -121,7 +121,7 @@ func (s *TemplateBuilder) SetManagedDiskUrl(managedImageId string, storageAccoun return nil } -func (s *TemplateBuilder) SetManagedMarketplaceImage(location, publisher, offer, sku, version, imageID string, storageAccountType compute.StorageAccountTypes) error { +func (s *TemplateBuilder) SetManagedMarketplaceImage(location, publisher, offer, sku, version, imageID string, storageAccountType compute.StorageAccountTypes, cachingType compute.CachingTypes) error { resource, err := s.getResourceByType(resourceVirtualMachine) if err != nil { return err @@ -133,11 +133,11 @@ func (s *TemplateBuilder) SetManagedMarketplaceImage(location, publisher, offer, Offer: &offer, Sku: &sku, Version: &version, - //ID: &imageID, } profile.OsDisk.OsType = s.osType profile.OsDisk.CreateOption = compute.DiskCreateOptionTypesFromImage profile.OsDisk.Vhd = nil + profile.OsDisk.Caching = cachingType profile.OsDisk.ManagedDisk = &compute.ManagedDiskParameters{ StorageAccountType: storageAccountType, } @@ -145,7 +145,7 @@ func (s *TemplateBuilder) SetManagedMarketplaceImage(location, publisher, offer, return nil } -func (s *TemplateBuilder) SetSharedGalleryImage(location, imageID string) error { +func (s *TemplateBuilder) SetSharedGalleryImage(location, imageID string, cachingType compute.CachingTypes) error { resource, err := s.getResourceByType(resourceVirtualMachine) if err != nil { return err @@ -156,17 +156,19 @@ func (s *TemplateBuilder) SetSharedGalleryImage(location, imageID string) error profile.ImageReference = &compute.ImageReference{ID: &imageID} profile.OsDisk.OsType = s.osType profile.OsDisk.Vhd = nil + profile.OsDisk.Caching = cachingType return nil } -func (s *TemplateBuilder) SetMarketPlaceImage(publisher, offer, sku, version string) error { +func (s *TemplateBuilder) SetMarketPlaceImage(publisher, offer, sku, version string, cachingType compute.CachingTypes) error { resource, err := s.getResourceByType(resourceVirtualMachine) if err != nil { return err } profile := resource.Properties.StorageProfile + profile.OsDisk.Caching = cachingType profile.ImageReference = &compute.ImageReference{ Publisher: to.StringPtr(publisher), Offer: to.StringPtr(offer), @@ -177,7 +179,7 @@ func (s *TemplateBuilder) SetMarketPlaceImage(publisher, offer, sku, version str return nil } -func (s *TemplateBuilder) SetImageUrl(imageUrl string, osType compute.OperatingSystemTypes) error { +func (s *TemplateBuilder) SetImageUrl(imageUrl string, osType compute.OperatingSystemTypes, cachingType compute.CachingTypes) error { resource, err := s.getResourceByType(resourceVirtualMachine) if err != nil { return err @@ -185,6 +187,8 @@ func (s *TemplateBuilder) SetImageUrl(imageUrl string, osType compute.OperatingS profile := resource.Properties.StorageProfile profile.OsDisk.OsType = osType + profile.OsDisk.Caching = cachingType + profile.OsDisk.Image = &compute.VirtualHardDisk{ URI: to.StringPtr(imageUrl), } @@ -224,7 +228,7 @@ func (s *TemplateBuilder) SetOSDiskSizeGB(diskSizeGB int32) error { return nil } -func (s *TemplateBuilder) SetAdditionalDisks(diskSizeGB []int32, isManaged bool) error { +func (s *TemplateBuilder) SetAdditionalDisks(diskSizeGB []int32, isManaged bool, cachingType compute.CachingTypes) error { resource, err := s.getResourceByType(resourceVirtualMachine) if err != nil { return err @@ -233,12 +237,12 @@ func (s *TemplateBuilder) SetAdditionalDisks(diskSizeGB []int32, isManaged bool) profile := resource.Properties.StorageProfile dataDisks := make([]DataDiskUnion, len(diskSizeGB)) - for i, additionalsize := range diskSizeGB { - dataDisks[i].DiskSizeGB = to.Int32Ptr(additionalsize) + for i, additionalSize := range diskSizeGB { + dataDisks[i].DiskSizeGB = to.Int32Ptr(additionalSize) dataDisks[i].Lun = to.IntPtr(i) dataDisks[i].Name = to.StringPtr(fmt.Sprintf("datadisk-%d", i+1)) dataDisks[i].CreateOption = "Empty" - dataDisks[i].Caching = "ReadWrite" + dataDisks[i].Caching = cachingType if isManaged { dataDisks[i].Vhd = nil dataDisks[i].ManagedDisk = profile.OsDisk.ManagedDisk diff --git a/builder/azure/common/template/template_builder_test.TestSharedImageGallery00.approved.json b/builder/azure/common/template/template_builder_test.TestSharedImageGallery00.approved.json index 3d57c2b1f..c180e1e16 100644 --- a/builder/azure/common/template/template_builder_test.TestSharedImageGallery00.approved.json +++ b/builder/azure/common/template/template_builder_test.TestSharedImageGallery00.approved.json @@ -139,7 +139,7 @@ "id": "/subscriptions/ignore/resourceGroups/ignore/providers/Microsoft.Compute/galleries/ignore/images/ignore" }, "osDisk": { - "caching": "ReadWrite", + "caching": "ReadOnly", "createOption": "FromImage", "name": "[parameters('osDiskName')]", "osType": "Linux" diff --git a/builder/azure/common/template/template_builder_test.go b/builder/azure/common/template/template_builder_test.go index cbe998c03..5de8ce4ae 100644 --- a/builder/azure/common/template/template_builder_test.go +++ b/builder/azure/common/template/template_builder_test.go @@ -20,7 +20,7 @@ func TestBuildLinux00(t *testing.T) { t.Fatal(err) } - err = testSubject.SetMarketPlaceImage("Canonical", "UbuntuServer", "16.04", "latest") + err = testSubject.SetMarketPlaceImage("Canonical", "UbuntuServer", "16.04", "latest", compute.CachingTypesReadWrite) if err != nil { t.Fatal(err) } @@ -48,7 +48,7 @@ func TestBuildLinux01(t *testing.T) { t.Fatal(err) } - err = testSubject.SetImageUrl("http://azure/custom.vhd", compute.Linux) + err = testSubject.SetImageUrl("http://azure/custom.vhd", compute.Linux, compute.CachingTypesReadWrite) if err != nil { t.Fatal(err) } @@ -72,7 +72,7 @@ func TestBuildLinux02(t *testing.T) { } testSubject.BuildLinux("--test-ssh-authorized-key--") - testSubject.SetImageUrl("http://azure/custom.vhd", compute.Linux) + testSubject.SetImageUrl("http://azure/custom.vhd", compute.Linux, compute.CachingTypesReadWrite) testSubject.SetOSDiskSizeGB(100) err = testSubject.SetVirtualNetwork("--virtual-network-resource-group--", "--virtual-network--", "--subnet-name--") @@ -105,7 +105,7 @@ func TestBuildWindows00(t *testing.T) { t.Fatal(err) } - err = testSubject.SetMarketPlaceImage("MicrosoftWindowsServer", "WindowsServer", "2012-R2-Datacenter", "latest") + err = testSubject.SetMarketPlaceImage("MicrosoftWindowsServer", "WindowsServer", "2012-R2-Datacenter", "latest", compute.CachingTypesReadWrite) if err != nil { t.Fatal(err) } @@ -133,12 +133,12 @@ func TestBuildWindows01(t *testing.T) { t.Fatal(err) } - err = testSubject.SetManagedMarketplaceImage("MicrosoftWindowsServer", "WindowsServer", "2012-R2-Datacenter", "latest", "2015-1", "1", "Premium_LRS") + err = testSubject.SetManagedMarketplaceImage("MicrosoftWindowsServer", "WindowsServer", "2012-R2-Datacenter", "latest", "2015-1", "1", "Premium_LRS", compute.CachingTypesReadWrite) if err != nil { t.Fatal(err) } - err = testSubject.SetAdditionalDisks([]int32{32, 64}, true) + err = testSubject.SetAdditionalDisks([]int32{32, 64}, true, compute.CachingTypesReadWrite) if err != nil { t.Fatal(err) } @@ -166,7 +166,7 @@ func TestBuildWindows02(t *testing.T) { t.Fatal(err) } - err = testSubject.SetAdditionalDisks([]int32{32, 64}, false) + err = testSubject.SetAdditionalDisks([]int32{32, 64}, false, compute.CachingTypesReadWrite) if err != nil { t.Fatal(err) } @@ -195,7 +195,7 @@ func TestSharedImageGallery00(t *testing.T) { } imageID := "/subscriptions/ignore/resourceGroups/ignore/providers/Microsoft.Compute/galleries/ignore/images/ignore" - err = testSubject.SetSharedGalleryImage("westcentralus", imageID) + err = testSubject.SetSharedGalleryImage("westcentralus", imageID, compute.CachingTypesReadOnly) if err != nil { t.Fatal(err) } diff --git a/website/source/docs/builders/azure.html.md b/website/source/docs/builders/azure.html.md index 8ee429c86..556d0b968 100644 --- a/website/source/docs/builders/azure.html.md +++ b/website/source/docs/builders/azure.html.md @@ -190,6 +190,9 @@ Providing `temp_resource_group_name` or `location` in combination with - `os_disk_size_gb` (number) Specify the size of the OS disk in GB (gigabytes). Values of zero or less than zero are ignored. +- `disk_caching_type` (string) Specify the disk caching type. Valid values are None, ReadOnly, + and ReadWrite. The default value is ReadWrite. + - `disk_additional_size` (array of integers) - The size(s) of any additional hard disks for the VM in gigabytes. If this is not specified then the VM will only contain an OS disk. The number of additional disks and maximum