Merge pull request #2430 from damdo/fix-data-disk-ultrassd-w-caching-…

…option

fix: validation of caching options for Ultra disks as data disks

api/v1beta1/azuremachine_default.go

@@ -19,6 +19,7 @@ package v1beta1
 import (
 	"encoding/base64"
 
+	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute"
 	"golang.org/x/crypto/ssh"
 	"k8s.io/apimachinery/pkg/util/uuid"
 	utilSSH "sigs.k8s.io/cluster-api-provider-azure/util/ssh"
@@ -67,7 +68,12 @@ func (s *AzureMachineSpec) SetDataDisksDefaults() {
 			}
 		}
 		if disk.CachingType == "" {
-			s.DataDisks[i].CachingType = "ReadWrite"
+			if s.DataDisks[i].ManagedDisk != nil &&
+				s.DataDisks[i].ManagedDisk.StorageAccountType == string(compute.StorageAccountTypesUltraSSDLRS) {
+				s.DataDisks[i].CachingType = string(compute.CachingTypesNone)
+			} else {
+				s.DataDisks[i].CachingType = string(compute.CachingTypesReadWrite)
+			}
 		}
 	}
 }

api/v1beta1/azuremachine_default_test.go

@@ -215,6 +215,14 @@ func TestAzureMachineSpec_SetDataDisksDefaults(t *testing.T) {
 					DiskSizeGB: 30,
 					Lun:        to.Int32Ptr(2),
 				},
+				{
+					NameSuffix: "testdisk3",
+					DiskSizeGB: 30,
+					ManagedDisk: &ManagedDiskParameters{
+						StorageAccountType: "UltraSSD_LRS",
+					},
+					Lun: to.Int32Ptr(3),
+				},
 			},
 			output: []DataDisk{
 				{
@@ -229,6 +237,15 @@ func TestAzureMachineSpec_SetDataDisksDefaults(t *testing.T) {
 					Lun:         to.Int32Ptr(2),
 					CachingType: "ReadWrite",
 				},
+				{
+					NameSuffix: "testdisk3",
+					DiskSizeGB: 30,
+					Lun:        to.Int32Ptr(3),
+					ManagedDisk: &ManagedDiskParameters{
+						StorageAccountType: "UltraSSD_LRS",
+					},
+					CachingType: "None",
+				},
 			},
 		},
 	}

api/v1beta1/azuremachine_validation.go

@@ -143,7 +143,7 @@ func ValidateDataDisks(dataDisks []DataDisk, fieldPath *field.Path) field.ErrorL
 		}
 
 		// validate cachingType
-		allErrs = append(allErrs, validateCachingType(disk.CachingType, fieldPath)...)
+		allErrs = append(allErrs, validateCachingType(disk.CachingType, fieldPath, disk.ManagedDisk)...)
 	}
 	return allErrs
 }
@@ -162,7 +162,7 @@ func ValidateOSDisk(osDisk OSDisk, fieldPath *field.Path) field.ErrorList {
 		allErrs = append(allErrs, field.Required(fieldPath.Child("OSType"), "the OS type cannot be empty"))
 	}
 
-	allErrs = append(allErrs, validateCachingType(osDisk.CachingType, fieldPath)...)
+	allErrs = append(allErrs, validateCachingType(osDisk.CachingType, fieldPath, osDisk.ManagedDisk)...)
 
 	if osDisk.ManagedDisk != nil {
 		if errs := validateManagedDisk(osDisk.ManagedDisk, fieldPath.Child("managedDisk"), true); len(errs) > 0 {
@@ -278,10 +278,16 @@ func validateStorageAccountType(storageAccountType string, fieldPath *field.Path
 	return allErrs
 }
 
-func validateCachingType(cachingType string, fieldPath *field.Path) field.ErrorList {
+func validateCachingType(cachingType string, fieldPath *field.Path, managedDisk *ManagedDiskParameters) field.ErrorList {
 	allErrs := field.ErrorList{}
 	cachingTypeChildPath := fieldPath.Child("CachingType")
 
+	if managedDisk != nil && managedDisk.StorageAccountType == string(compute.StorageAccountTypesUltraSSDLRS) {
+		if cachingType != string(compute.CachingTypesNone) {
+			allErrs = append(allErrs, field.Invalid(cachingTypeChildPath, cachingType, fmt.Sprintf("cachingType '%s' is not supported when storageAccountType is '%s'. Allowed values are: '%s'", cachingType, compute.StorageAccountTypesUltraSSDLRS, compute.CachingTypesNone)))
+		}
+	}
+
 	for _, possibleCachingType := range compute.PossibleCachingTypesValues() {
 		if string(possibleCachingType) == cachingType {
 			return allErrs

api/v1beta1/azuremachine_validation_test.go

@@ -385,6 +385,51 @@ func TestAzureMachine_ValidateDataDisks(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "valid combination of managed disk storage account type UltraSSD_LRS and cachingType None",
+			disks: []DataDisk{
+				{
+					NameSuffix: "my_disk_1",
+					DiskSizeGB: 64,
+					ManagedDisk: &ManagedDiskParameters{
+						StorageAccountType: string(compute.StorageAccountTypesUltraSSDLRS),
+					},
+					Lun:         to.Int32Ptr(0),
+					CachingType: string(compute.CachingTypesNone),
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid combination of managed disk storage account type UltraSSD_LRS and cachingType ReadWrite",
+			disks: []DataDisk{
+				{
+					NameSuffix: "my_disk_1",
+					DiskSizeGB: 64,
+					ManagedDisk: &ManagedDiskParameters{
+						StorageAccountType: string(compute.StorageAccountTypesUltraSSDLRS),
+					},
+					Lun:         to.Int32Ptr(0),
+					CachingType: string(compute.CachingTypesReadWrite),
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid combination of managed disk storage account type UltraSSD_LRS and cachingType ReadOnly",
+			disks: []DataDisk{
+				{
+					NameSuffix: "my_disk_1",
+					DiskSizeGB: 64,
+					ManagedDisk: &ManagedDiskParameters{
+						StorageAccountType: string(compute.StorageAccountTypesUltraSSDLRS),
+					},
+					Lun:         to.Int32Ptr(0),
+					CachingType: string(compute.CachingTypesReadOnly),
+				},
+			},
+			wantErr: true,
+		},
 	}
 
 	for _, test := range testcases {

docs/book/src/topics/data-disks.md

@@ -34,6 +34,8 @@ az vm list-skus -l <location> -z -s <VM-size>
 
 Provided that the chosen region and zone support Ultra disks, Azure Machine objects having Ultra disks specified as Data disks will have their virtual machines created with the `AdditionalCapabilities.UltraSSDEnabled` additional capability set to `true`. This capability can also be manually set on the Azure Machine spec and will override the automatically chosen value (if any).
 
+When the chosen StorageAccountType is `UltraSSD_LRS`, caching is not supported for the disk and the corresponding `cachingType` field must be set to `None`. In this configuration, if no value is set, `cachingType` will be defaulted to `None`.
+
 See [Ultra disk](https://docs.microsoft.com/en-us/azure/virtual-machines/disks-types#ultra-disk) for ultra disk performance and GA scope.
 
 ### Ultra disk support for Persistent Volumes