From bdd9f0efe31244707ac85840e3d91825b79b9fa8 Mon Sep 17 00:00:00 2001 From: Alex Volchok Date: Wed, 27 Nov 2019 11:01:28 +0200 Subject: [PATCH 1/9] Adding multiple disk attachment support for alibaba cloud (cherry picked from commit 7c658e2911155d7acb49a6f54ade3190e4c76718) --- .../alicloud-machine-class.yaml | 13 +++++++++ pkg/apis/machine/v1alpha1/types.go | 10 +++++++ pkg/driver/driver_alicloud.go | 29 ++++++++++++++++++- 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/kubernetes/machine_classes/alicloud-machine-class.yaml b/kubernetes/machine_classes/alicloud-machine-class.yaml index 3293b4402..848e073c0 100644 --- a/kubernetes/machine_classes/alicloud-machine-class.yaml +++ b/kubernetes/machine_classes/alicloud-machine-class.yaml @@ -12,6 +12,19 @@ spec: zoneID: cn-hangzhou-e # Zone of the region securityGroupID: sg-1234567890 # ID of security group, it has to be within the same VPC of vSwitch vSwitchID: vsw-1234567890 # similar to AWS subnet ID + disks: # Array of disks to be associated with this instance + - name: disk1 + size: 50 + category: cloud_ssd + encrypted: true + description: 50 GB disk description + + - name: disk2 + size: 40 + category: cloud_ssd + encrypted: true + description: 40 GB disk description + systemDisk: category: cloud_efficiency # cloud, cloud_efficiency, cloud_ssd, ephemeral_ssd size: 30 # 20-500 diff --git a/pkg/apis/machine/v1alpha1/types.go b/pkg/apis/machine/v1alpha1/types.go index 2bc9691cc..6f85ac5ef 100644 --- a/pkg/apis/machine/v1alpha1/types.go +++ b/pkg/apis/machine/v1alpha1/types.go @@ -1236,6 +1236,7 @@ type AlicloudMachineClassSpec struct { VSwitchID string `json:"vSwitchID"` PrivateIPAddress string `json:"privateIPAddress,omitempty"` SystemDisk *AlicloudSystemDisk `json:"systemDisk,omitempty"` + DataDisks []*AlicloudDataDisk `json:"disks,omitempty"` InstanceChargeType string `json:"instanceChargeType,omitempty"` InternetChargeType string `json:"internetChargeType,omitempty"` InternetMaxBandwidthIn *int `json:"internetMaxBandwidthIn,omitempty"` @@ -1247,6 +1248,15 @@ type AlicloudMachineClassSpec struct { SecretRef *corev1.SecretReference `json:"secretRef,omitempty"` } +type AlicloudDataDisk struct { + Name string `json:"name"` + Category string `json:"category"` + // +optional + Description string `json:"description"` + Encrypted bool `json:"encrypted"` + Size int `json:"size"` +} + // AlicloudSystemDisk describes SystemDisk for Alicloud. type AlicloudSystemDisk struct { Category string `json:"category"` diff --git a/pkg/driver/driver_alicloud.go b/pkg/driver/driver_alicloud.go index 13cb7fcaa..065691913 100644 --- a/pkg/driver/driver_alicloud.go +++ b/pkg/driver/driver_alicloud.go @@ -21,12 +21,13 @@ import ( "encoding/base64" "errors" "fmt" + "strconv" "strings" "github.com/golang/glog" "github.com/prometheus/client_golang/prometheus" - v1alpha1 "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" "github.com/gardener/machine-controller-manager/pkg/metrics" corev1 "k8s.io/api/core/v1" @@ -75,6 +76,32 @@ func (c *AlicloudDriver) Create() (string, string, error) { request.InternetMaxBandwidthOut = requests.NewInteger(*c.AlicloudMachineClass.Spec.InternetMaxBandwidthOut) } + if ok := c.AlicloudMachineClass.Spec.DataDisks != nil; ok { + disks := c.AlicloudMachineClass.Spec.DataDisks + var dataDiskRequests []ecs.RunInstancesDataDisk + + for _, disk := range disks { + + dataDiskRequest := ecs.RunInstancesDataDisk{ + Category: disk.Category, + DeleteWithInstance: strconv.FormatBool(true), + Encrypted: strconv.FormatBool(disk.Encrypted), + DiskName: disk.Name, + Description: disk.Description, + } + + dataDiskRequest.Size = fmt.Sprintf("%d", disk.Size) + + if disk.Category == "DiskEphemeralSSD" { + dataDiskRequest.DeleteWithInstance = "" + } + + dataDiskRequests = append(dataDiskRequests, dataDiskRequest) + } + + request.DataDisk = &dataDiskRequests + } + if c.AlicloudMachineClass.Spec.SystemDisk != nil { request.SystemDiskCategory = c.AlicloudMachineClass.Spec.SystemDisk.Category request.SystemDiskSize = fmt.Sprintf("%d", c.AlicloudMachineClass.Spec.SystemDisk.Size) From 66bebe9d48f84e0b92d17ac226b19432d4f673b4 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Wed, 29 Jan 2020 17:36:33 +0200 Subject: [PATCH 2/9] Support Data Disks in AliCloud and Azure --- .../alicloud-machine-class.yaml | 26 +- .../machine_classes/aws-machine-class.yaml | 11 +- .../machine_classes/azure-machine-class.yaml | 15 + .../machine_classes/gcp-machine-class.yaml | 7 + pkg/apis/machine/types.go | 21 ++ pkg/apis/machine/v1alpha1/types.go | 23 +- .../v1alpha1/zz_generated.conversion.go | 132 +++++++- .../machine/v1alpha1/zz_generated.deepcopy.go | 43 +++ .../machine/validation/awsmachineclass.go | 8 + .../machine/validation/azuremachineclass.go | 31 ++ pkg/apis/machine/zz_generated.deepcopy.go | 43 +++ pkg/controller/drain_test.go | 8 +- pkg/driver/driver_alicloud.go | 45 +-- pkg/driver/driver_alicloud_test.go | 303 ++++++++++-------- pkg/driver/driver_aws_test.go | 46 ++- pkg/driver/driver_azure.go | 100 ++++-- pkg/driver/driver_azure_test.go | 90 ++++++ pkg/driver/driver_test.go | 10 + pkg/openapi/openapi_generated.go | 163 +++++++--- 19 files changed, 878 insertions(+), 247 deletions(-) create mode 100644 pkg/driver/driver_azure_test.go create mode 100644 pkg/driver/driver_test.go diff --git a/kubernetes/machine_classes/alicloud-machine-class.yaml b/kubernetes/machine_classes/alicloud-machine-class.yaml index 848e073c0..579d06fce 100644 --- a/kubernetes/machine_classes/alicloud-machine-class.yaml +++ b/kubernetes/machine_classes/alicloud-machine-class.yaml @@ -12,19 +12,19 @@ spec: zoneID: cn-hangzhou-e # Zone of the region securityGroupID: sg-1234567890 # ID of security group, it has to be within the same VPC of vSwitch vSwitchID: vsw-1234567890 # similar to AWS subnet ID - disks: # Array of disks to be associated with this instance - - name: disk1 - size: 50 - category: cloud_ssd - encrypted: true - description: 50 GB disk description - - - name: disk2 - size: 40 - category: cloud_ssd - encrypted: true - description: 40 GB disk description - + dataDisks: # Array of disks to be associated with this instance + - name: disk1 + size: 50 + category: cloud_ssd + encrypted: false + description: 50 GB disk description + deleteWithInstance: true + - name: disk2 + size: 40 + category: cloud_efficiency + encrypted: true + description: 40 GB disk description + deleteWithInstance: true systemDisk: category: cloud_efficiency # cloud, cloud_efficiency, cloud_ssd, ephemeral_ssd size: 30 # 20-500 diff --git a/kubernetes/machine_classes/aws-machine-class.yaml b/kubernetes/machine_classes/aws-machine-class.yaml index f00e91209..f27db494f 100644 --- a/kubernetes/machine_classes/aws-machine-class.yaml +++ b/kubernetes/machine_classes/aws-machine-class.yaml @@ -25,6 +25,15 @@ spec: namespace: default # Namespace name: test-secret # Name of the secret blockDevices: - - ebs: + - name: /root + ebs: volumeSize: 50 # Size of the root block device volumeType: gp2 # Type of the root block device + encrypted: false + deleteOnTermination: true + - name: /dev/sdb + ebs: + volumeSize: 50 # Size of the root block device + volumeType: gp2 # Type of the root block device + encrypted: true + deleteOnTermination: true \ No newline at end of file diff --git a/kubernetes/machine_classes/azure-machine-class.yaml b/kubernetes/machine_classes/azure-machine-class.yaml index f3d451670..60659f7bc 100644 --- a/kubernetes/machine_classes/azure-machine-class.yaml +++ b/kubernetes/machine_classes/azure-machine-class.yaml @@ -29,6 +29,21 @@ spec: caching: "None" # Caching Strategy (None/ReadOnly/ReadWrite) diskSizeGB: 50 # Size of disk to be created in GB createOption: "FromImage" # Create option for disk (Empty/Attach/FromImage) + DataDisks: + - lun: 0 + caching: None + createOption: Empty + diskSizeGB: 100 + managedDisk: + storageAccountType: Standard_LRS + name: sdb + - lun: 1 + caching: None + createOption: Empty + diskSizeGB: 100 + managedDisk: + storageAccountType: Standard_LRS + name: sdc osProfile: adminUsername: "admin-name" # Admin user name linuxConfiguration: diff --git a/kubernetes/machine_classes/gcp-machine-class.yaml b/kubernetes/machine_classes/gcp-machine-class.yaml index ec3000a73..2557d1f91 100644 --- a/kubernetes/machine_classes/gcp-machine-class.yaml +++ b/kubernetes/machine_classes/gcp-machine-class.yaml @@ -17,6 +17,13 @@ spec: image: projects/coreos-cloud/global/images/image-name-goes-here # Disk image name labels: name: my-disk # Label assigned to the disk + - autoDelete: true + boot: false + sizeGb: 50 + type: pd-standard + image: projects/coreos-cloud/global/images/image-name-goes-here # Disk image name + labels: + name: my-disk labels: name: my-instance # Label assigned to the instance machineType: n1-standard-4 # Type of GCP instance to launch diff --git a/pkg/apis/machine/types.go b/pkg/apis/machine/types.go index 672f71b22..ea12ea267 100644 --- a/pkg/apis/machine/types.go +++ b/pkg/apis/machine/types.go @@ -888,6 +888,7 @@ type AzureHardwareProfile struct { type AzureStorageProfile struct { ImageReference AzureImageReference OsDisk AzureOSDisk + DataDisks []AzureDataDisk } // AzureImageReference is specifies information about the image to use. You can specify information about platform images, @@ -909,6 +910,15 @@ type AzureOSDisk struct { CreateOption string } +type AzureDataDisk struct { + Name string + Lun int32 + Caching string + ManagedDisk AzureManagedDiskParameters + DiskSizeGB int32 + CreateOption string +} + // AzureManagedDiskParameters is the parameters of a managed disk. type AzureManagedDiskParameters struct { ID string @@ -1088,6 +1098,7 @@ type AlicloudMachineClassSpec struct { VSwitchID string PrivateIPAddress string SystemDisk *AlicloudSystemDisk + DataDisks []AlicloudDataDisk InstanceChargeType string InternetChargeType string InternetMaxBandwidthIn *int @@ -1105,6 +1116,16 @@ type AlicloudSystemDisk struct { Size int } +// AlicloudDataDisk describes DataDisk for Alicloud. +type AlicloudDataDisk struct { + Name string + Category string + Description string + Encrypted bool + Size int + DeleteWithInstance bool +} + /********************** PacketMachineClass APIs ***************/ // +genclient diff --git a/pkg/apis/machine/v1alpha1/types.go b/pkg/apis/machine/v1alpha1/types.go index 6f85ac5ef..dac6ae158 100644 --- a/pkg/apis/machine/v1alpha1/types.go +++ b/pkg/apis/machine/v1alpha1/types.go @@ -969,6 +969,7 @@ type AzureHardwareProfile struct { type AzureStorageProfile struct { ImageReference AzureImageReference `json:"imageReference,omitempty"` OsDisk AzureOSDisk `json:"osDisk,omitempty"` + DataDisks []AzureDataDisk `json:"dataDisks,omitempty"` } // AzureImageReference is specifies information about the image to use. You can specify information about platform images, @@ -991,6 +992,15 @@ type AzureOSDisk struct { CreateOption string `json:"createOption,omitempty"` } +type AzureDataDisk struct { + Name string `json:"name,omitempty"` + Lun int32 `json:"lun,omitempty"` + Caching string `json:"caching,omitempty"` + ManagedDisk AzureManagedDiskParameters `json:"managedDisk,omitempty"` + DiskSizeGB int32 `json:"diskSizeGB,omitempty"` + CreateOption string `json:"createOption,omitempty"` +} + // AzureManagedDiskParameters is the parameters of a managed disk. type AzureManagedDiskParameters struct { ID string `json:"id,omitempty"` @@ -1236,7 +1246,7 @@ type AlicloudMachineClassSpec struct { VSwitchID string `json:"vSwitchID"` PrivateIPAddress string `json:"privateIPAddress,omitempty"` SystemDisk *AlicloudSystemDisk `json:"systemDisk,omitempty"` - DataDisks []*AlicloudDataDisk `json:"disks,omitempty"` + DataDisks []AlicloudDataDisk `json:"dataDisks,omitempty"` InstanceChargeType string `json:"instanceChargeType,omitempty"` InternetChargeType string `json:"internetChargeType,omitempty"` InternetMaxBandwidthIn *int `json:"internetMaxBandwidthIn,omitempty"` @@ -1249,12 +1259,13 @@ type AlicloudMachineClassSpec struct { } type AlicloudDataDisk struct { - Name string `json:"name"` - Category string `json:"category"` + Name string `json:"name"` + Category string `json:"category"` // +optional - Description string `json:"description"` - Encrypted bool `json:"encrypted"` - Size int `json:"size"` + Description string `json:"description"` + Encrypted bool `json:"encrypted"` + DeleteWithInstance bool `json:"deleteWithInstance"` + Size int `json:"size"` } // AlicloudSystemDisk describes SystemDisk for Alicloud. diff --git a/pkg/apis/machine/v1alpha1/zz_generated.conversion.go b/pkg/apis/machine/v1alpha1/zz_generated.conversion.go index 127ea01e3..d44ea4ac4 100644 --- a/pkg/apis/machine/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/machine/v1alpha1/zz_generated.conversion.go @@ -108,6 +108,16 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddGeneratedConversionFunc((*AlicloudDataDisk)(nil), (*machine.AlicloudDataDisk)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1alpha1_AlicloudDataDisk_To_machine_AlicloudDataDisk(a.(*AlicloudDataDisk), b.(*machine.AlicloudDataDisk), scope) + }); err != nil { + return err + } + if err := s.AddGeneratedConversionFunc((*machine.AlicloudDataDisk)(nil), (*AlicloudDataDisk)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_machine_AlicloudDataDisk_To_v1alpha1_AlicloudDataDisk(a.(*machine.AlicloudDataDisk), b.(*AlicloudDataDisk), scope) + }); err != nil { + return err + } if err := s.AddGeneratedConversionFunc((*AlicloudMachineClass)(nil), (*machine.AlicloudMachineClass)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha1_AlicloudMachineClass_To_machine_AlicloudMachineClass(a.(*AlicloudMachineClass), b.(*machine.AlicloudMachineClass), scope) }); err != nil { @@ -148,6 +158,16 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddGeneratedConversionFunc((*AzureDataDisk)(nil), (*machine.AzureDataDisk)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk(a.(*AzureDataDisk), b.(*machine.AzureDataDisk), scope) + }); err != nil { + return err + } + if err := s.AddGeneratedConversionFunc((*machine.AzureDataDisk)(nil), (*AzureDataDisk)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_machine_AzureDataDisk_To_v1alpha1_AzureDataDisk(a.(*machine.AzureDataDisk), b.(*AzureDataDisk), scope) + }); err != nil { + return err + } if err := s.AddGeneratedConversionFunc((*AzureHardwareProfile)(nil), (*machine.AzureHardwareProfile)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha1_AzureHardwareProfile_To_machine_AzureHardwareProfile(a.(*AzureHardwareProfile), b.(*machine.AzureHardwareProfile), scope) }); err != nil { @@ -991,6 +1011,36 @@ func Convert_machine_AWSNetworkInterfaceSpec_To_v1alpha1_AWSNetworkInterfaceSpec return autoConvert_machine_AWSNetworkInterfaceSpec_To_v1alpha1_AWSNetworkInterfaceSpec(in, out, s) } +func autoConvert_v1alpha1_AlicloudDataDisk_To_machine_AlicloudDataDisk(in *AlicloudDataDisk, out *machine.AlicloudDataDisk, s conversion.Scope) error { + out.Name = in.Name + out.Category = in.Category + out.Description = in.Description + out.Encrypted = in.Encrypted + out.DeleteWithInstance = in.DeleteWithInstance + out.Size = in.Size + return nil +} + +// Convert_v1alpha1_AlicloudDataDisk_To_machine_AlicloudDataDisk is an autogenerated conversion function. +func Convert_v1alpha1_AlicloudDataDisk_To_machine_AlicloudDataDisk(in *AlicloudDataDisk, out *machine.AlicloudDataDisk, s conversion.Scope) error { + return autoConvert_v1alpha1_AlicloudDataDisk_To_machine_AlicloudDataDisk(in, out, s) +} + +func autoConvert_machine_AlicloudDataDisk_To_v1alpha1_AlicloudDataDisk(in *machine.AlicloudDataDisk, out *AlicloudDataDisk, s conversion.Scope) error { + out.Name = in.Name + out.Category = in.Category + out.Description = in.Description + out.Encrypted = in.Encrypted + out.Size = in.Size + out.DeleteWithInstance = in.DeleteWithInstance + return nil +} + +// Convert_machine_AlicloudDataDisk_To_v1alpha1_AlicloudDataDisk is an autogenerated conversion function. +func Convert_machine_AlicloudDataDisk_To_v1alpha1_AlicloudDataDisk(in *machine.AlicloudDataDisk, out *AlicloudDataDisk, s conversion.Scope) error { + return autoConvert_machine_AlicloudDataDisk_To_v1alpha1_AlicloudDataDisk(in, out, s) +} + func autoConvert_v1alpha1_AlicloudMachineClass_To_machine_AlicloudMachineClass(in *AlicloudMachineClass, out *machine.AlicloudMachineClass, s conversion.Scope) error { out.ObjectMeta = in.ObjectMeta if err := Convert_v1alpha1_AlicloudMachineClassSpec_To_machine_AlicloudMachineClassSpec(&in.Spec, &out.Spec, s); err != nil { @@ -1019,7 +1069,17 @@ func Convert_machine_AlicloudMachineClass_To_v1alpha1_AlicloudMachineClass(in *m func autoConvert_v1alpha1_AlicloudMachineClassList_To_machine_AlicloudMachineClassList(in *AlicloudMachineClassList, out *machine.AlicloudMachineClassList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]machine.AlicloudMachineClass)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]machine.AlicloudMachineClass, len(*in)) + for i := range *in { + if err := Convert_v1alpha1_AlicloudMachineClass_To_machine_AlicloudMachineClass(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -1030,7 +1090,17 @@ func Convert_v1alpha1_AlicloudMachineClassList_To_machine_AlicloudMachineClassLi func autoConvert_machine_AlicloudMachineClassList_To_v1alpha1_AlicloudMachineClassList(in *machine.AlicloudMachineClassList, out *AlicloudMachineClassList, s conversion.Scope) error { out.ListMeta = in.ListMeta - out.Items = *(*[]AlicloudMachineClass)(unsafe.Pointer(&in.Items)) + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make([]AlicloudMachineClass, len(*in)) + for i := range *in { + if err := Convert_machine_AlicloudMachineClass_To_v1alpha1_AlicloudMachineClass(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.Items = nil + } return nil } @@ -1048,6 +1118,17 @@ func autoConvert_v1alpha1_AlicloudMachineClassSpec_To_machine_AlicloudMachineCla out.VSwitchID = in.VSwitchID out.PrivateIPAddress = in.PrivateIPAddress out.SystemDisk = (*machine.AlicloudSystemDisk)(unsafe.Pointer(in.SystemDisk)) + if in.DataDisks != nil { + in, out := &in.DataDisks, &out.DataDisks + *out = make([]machine.AlicloudDataDisk, len(*in)) + for i := range *in { + if err := Convert_v1alpha1_AlicloudDataDisk_To_machine_AlicloudDataDisk(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.DataDisks = nil + } out.InstanceChargeType = in.InstanceChargeType out.InternetChargeType = in.InternetChargeType out.InternetMaxBandwidthIn = (*int)(unsafe.Pointer(in.InternetMaxBandwidthIn)) @@ -1074,6 +1155,17 @@ func autoConvert_machine_AlicloudMachineClassSpec_To_v1alpha1_AlicloudMachineCla out.VSwitchID = in.VSwitchID out.PrivateIPAddress = in.PrivateIPAddress out.SystemDisk = (*AlicloudSystemDisk)(unsafe.Pointer(in.SystemDisk)) + if in.DataDisks != nil { + in, out := &in.DataDisks, &out.DataDisks + *out = make([]AlicloudDataDisk, len(*in)) + for i := range *in { + if err := Convert_machine_AlicloudDataDisk_To_v1alpha1_AlicloudDataDisk(&(*in)[i], &(*out)[i], s); err != nil { + return err + } + } + } else { + out.DataDisks = nil + } out.InstanceChargeType = in.InstanceChargeType out.InternetChargeType = in.InternetChargeType out.InternetMaxBandwidthIn = (*int)(unsafe.Pointer(in.InternetMaxBandwidthIn)) @@ -1113,6 +1205,40 @@ func Convert_machine_AlicloudSystemDisk_To_v1alpha1_AlicloudSystemDisk(in *machi return autoConvert_machine_AlicloudSystemDisk_To_v1alpha1_AlicloudSystemDisk(in, out, s) } +func autoConvert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk(in *AzureDataDisk, out *machine.AzureDataDisk, s conversion.Scope) error { + out.Name = in.Name + out.Lun = in.Lun + out.Caching = in.Caching + if err := Convert_v1alpha1_AzureManagedDiskParameters_To_machine_AzureManagedDiskParameters(&in.ManagedDisk, &out.ManagedDisk, s); err != nil { + return err + } + out.DiskSizeGB = in.DiskSizeGB + out.CreateOption = in.CreateOption + return nil +} + +// Convert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk is an autogenerated conversion function. +func Convert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk(in *AzureDataDisk, out *machine.AzureDataDisk, s conversion.Scope) error { + return autoConvert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk(in, out, s) +} + +func autoConvert_machine_AzureDataDisk_To_v1alpha1_AzureDataDisk(in *machine.AzureDataDisk, out *AzureDataDisk, s conversion.Scope) error { + out.Name = in.Name + out.Lun = in.Lun + out.Caching = in.Caching + if err := Convert_machine_AzureManagedDiskParameters_To_v1alpha1_AzureManagedDiskParameters(&in.ManagedDisk, &out.ManagedDisk, s); err != nil { + return err + } + out.DiskSizeGB = in.DiskSizeGB + out.CreateOption = in.CreateOption + return nil +} + +// Convert_machine_AzureDataDisk_To_v1alpha1_AzureDataDisk is an autogenerated conversion function. +func Convert_machine_AzureDataDisk_To_v1alpha1_AzureDataDisk(in *machine.AzureDataDisk, out *AzureDataDisk, s conversion.Scope) error { + return autoConvert_machine_AzureDataDisk_To_v1alpha1_AzureDataDisk(in, out, s) +} + func autoConvert_v1alpha1_AzureHardwareProfile_To_machine_AzureHardwareProfile(in *AzureHardwareProfile, out *machine.AzureHardwareProfile, s conversion.Scope) error { out.VMSize = in.VMSize return nil @@ -1472,6 +1598,7 @@ func autoConvert_v1alpha1_AzureStorageProfile_To_machine_AzureStorageProfile(in if err := Convert_v1alpha1_AzureOSDisk_To_machine_AzureOSDisk(&in.OsDisk, &out.OsDisk, s); err != nil { return err } + out.DataDisks = *(*[]machine.AzureDataDisk)(unsafe.Pointer(&in.DataDisks)) return nil } @@ -1487,6 +1614,7 @@ func autoConvert_machine_AzureStorageProfile_To_v1alpha1_AzureStorageProfile(in if err := Convert_machine_AzureOSDisk_To_v1alpha1_AzureOSDisk(&in.OsDisk, &out.OsDisk, s); err != nil { return err } + out.DataDisks = *(*[]AzureDataDisk)(unsafe.Pointer(&in.DataDisks)) return nil } diff --git a/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go index d09b7abe2..9bb5d7dea 100644 --- a/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go @@ -198,6 +198,22 @@ func (in *AWSNetworkInterfaceSpec) DeepCopy() *AWSNetworkInterfaceSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AlicloudDataDisk) DeepCopyInto(out *AlicloudDataDisk) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AlicloudDataDisk. +func (in *AlicloudDataDisk) DeepCopy() *AlicloudDataDisk { + if in == nil { + return nil + } + out := new(AlicloudDataDisk) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AlicloudMachineClass) DeepCopyInto(out *AlicloudMachineClass) { *out = *in @@ -266,6 +282,11 @@ func (in *AlicloudMachineClassSpec) DeepCopyInto(out *AlicloudMachineClassSpec) *out = new(AlicloudSystemDisk) **out = **in } + if in.DataDisks != nil { + in, out := &in.DataDisks, &out.DataDisks + *out = make([]AlicloudDataDisk, len(*in)) + copy(*out, *in) + } if in.InternetMaxBandwidthIn != nil { in, out := &in.InternetMaxBandwidthIn, &out.InternetMaxBandwidthIn *out = new(int) @@ -317,6 +338,23 @@ func (in *AlicloudSystemDisk) DeepCopy() *AlicloudSystemDisk { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AzureDataDisk) DeepCopyInto(out *AzureDataDisk) { + *out = *in + out.ManagedDisk = in.ManagedDisk + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AzureDataDisk. +func (in *AzureDataDisk) DeepCopy() *AzureDataDisk { + if in == nil { + return nil + } + out := new(AzureDataDisk) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AzureHardwareProfile) DeepCopyInto(out *AzureHardwareProfile) { *out = *in @@ -603,6 +641,11 @@ func (in *AzureStorageProfile) DeepCopyInto(out *AzureStorageProfile) { *out = *in in.ImageReference.DeepCopyInto(&out.ImageReference) out.OsDisk = in.OsDisk + if in.DataDisks != nil { + in, out := &in.DataDisks, &out.DataDisks + *out = make([]AzureDataDisk, len(*in)) + copy(*out, *in) + } return } diff --git a/pkg/apis/machine/validation/awsmachineclass.go b/pkg/apis/machine/validation/awsmachineclass.go index 2cb1fce8a..209cb7a3a 100644 --- a/pkg/apis/machine/validation/awsmachineclass.go +++ b/pkg/apis/machine/validation/awsmachineclass.go @@ -132,12 +132,19 @@ func validateBlockDevices(blockDevices []machine.AWSBlockDeviceMappingSpec, fldP validVolumeTypes := []string{"gp2", "io1", "st1", "sc1", "standard"} + // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html + const dataDeviceNameFmt string = `^/dev/(sd[a-z]|xvd[a-c][a-z]?)$` + var dataDeviceNameRegexp = regexp.MustCompile("^" + dataDeviceNameFmt + "$") + // if blockDevices is empty, AWS will automatically create a root partition for i, disk := range blockDevices { idxPath := fldPath.Index(i) if disk.DeviceName == "/root" { rootPartitionCount++ + } else if len(blockDevices) > 1 && !dataDeviceNameRegexp.MatchString(disk.DeviceName) { + // if there are multiple devices, non-root devices are expected to adhere to AWS naming conventions + allErrs = append(allErrs, field.Invalid(idxPath.Child("deviceName"), disk.DeviceName, utilvalidation.RegexError(dataDeviceNameFmt, "/dev/sdf"))) } if _, keyExist := deviceNames[disk.DeviceName]; keyExist { @@ -158,6 +165,7 @@ func validateBlockDevices(blockDevices []machine.AWSBlockDeviceMappingSpec, fldP } else if disk.Ebs.VolumeType == "io1" && disk.Ebs.Iops <= 0 { allErrs = append(allErrs, field.Required(idxPath.Child("ebs.iops"), "Please mention a valid ebs volume iops")) } + } if rootPartitionCount > 1 { diff --git a/pkg/apis/machine/validation/azuremachineclass.go b/pkg/apis/machine/validation/azuremachineclass.go index 1499ef84a..ef056dfbd 100644 --- a/pkg/apis/machine/validation/azuremachineclass.go +++ b/pkg/apis/machine/validation/azuremachineclass.go @@ -18,6 +18,7 @@ limitations under the License. package validation import ( + "fmt" /*"strconv" "strings" "regexp" @@ -122,6 +123,36 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.osDisk.createOption"), "OSDisk create option is required")) } + var luns = make(map[int32]int) + + for i, dataDisk := range properties.StorageProfile.DataDisks { + idxPath := fldPath.Child("storageProfile.dataDisks").Index(i) + if _, keyExist := luns[dataDisk.Lun]; keyExist { + luns[dataDisk.Lun]++ + } else { + luns[dataDisk.Lun] = 1 + } + + if dataDisk.Caching == "" { + allErrs = append(allErrs, field.Required(idxPath.Child("caching"), "DataDisk caching is required")) + } + if dataDisk.DiskSizeGB <= 0 { + allErrs = append(allErrs, field.Required(idxPath.Child("diskSizeGB"), "DataDisk size must be positive")) + } + if dataDisk.CreateOption == "" { + allErrs = append(allErrs, field.Required(idxPath.Child("createOption"), "DataDisk create option is required")) + } + if dataDisk.Lun < 0 { + allErrs = append(allErrs, field.Required(idxPath.Child("lun"), "DataDisk Lun is required")) + } + } + + for lun, number := range luns { + if number > 1 { + allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.dataDisks"), fmt.Sprintf("Data Disk Lun '%s' duplicated %d times, Lun must be unique", lun, number))) + } + } + if properties.OsProfile.AdminUsername == "" { allErrs = append(allErrs, field.Required(fldPath.Child("osProfile.adminUsername"), "AdminUsername is required")) } diff --git a/pkg/apis/machine/zz_generated.deepcopy.go b/pkg/apis/machine/zz_generated.deepcopy.go index 925e74f9e..75086fb26 100644 --- a/pkg/apis/machine/zz_generated.deepcopy.go +++ b/pkg/apis/machine/zz_generated.deepcopy.go @@ -198,6 +198,22 @@ func (in *AWSNetworkInterfaceSpec) DeepCopy() *AWSNetworkInterfaceSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AlicloudDataDisk) DeepCopyInto(out *AlicloudDataDisk) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AlicloudDataDisk. +func (in *AlicloudDataDisk) DeepCopy() *AlicloudDataDisk { + if in == nil { + return nil + } + out := new(AlicloudDataDisk) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AlicloudMachineClass) DeepCopyInto(out *AlicloudMachineClass) { *out = *in @@ -266,6 +282,11 @@ func (in *AlicloudMachineClassSpec) DeepCopyInto(out *AlicloudMachineClassSpec) *out = new(AlicloudSystemDisk) **out = **in } + if in.DataDisks != nil { + in, out := &in.DataDisks, &out.DataDisks + *out = make([]AlicloudDataDisk, len(*in)) + copy(*out, *in) + } if in.InternetMaxBandwidthIn != nil { in, out := &in.InternetMaxBandwidthIn, &out.InternetMaxBandwidthIn *out = new(int) @@ -317,6 +338,23 @@ func (in *AlicloudSystemDisk) DeepCopy() *AlicloudSystemDisk { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AzureDataDisk) DeepCopyInto(out *AzureDataDisk) { + *out = *in + out.ManagedDisk = in.ManagedDisk + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AzureDataDisk. +func (in *AzureDataDisk) DeepCopy() *AzureDataDisk { + if in == nil { + return nil + } + out := new(AzureDataDisk) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AzureHardwareProfile) DeepCopyInto(out *AzureHardwareProfile) { *out = *in @@ -603,6 +641,11 @@ func (in *AzureStorageProfile) DeepCopyInto(out *AzureStorageProfile) { *out = *in in.ImageReference.DeepCopyInto(&out.ImageReference) out.OsDisk = in.OsDisk + if in.DataDisks != nil { + in, out := &in.DataDisks, &out.DataDisks + *out = make([]AzureDataDisk, len(*in)) + copy(*out, *in) + } return } diff --git a/pkg/controller/drain_test.go b/pkg/controller/drain_test.go index 9c033fc98..c5b4be830 100644 --- a/pkg/controller/drain_test.go +++ b/pkg/controller/drain_test.go @@ -642,7 +642,7 @@ var _ = Describe("drain", func() { }, attemptEviction: false, terminationGracePeriod: terminationGracePeriodShort, - force: true, + force: true, }, nil, &expectation{ @@ -668,7 +668,7 @@ var _ = Describe("drain", func() { }, attemptEviction: true, terminationGracePeriod: terminationGracePeriodShort, - force: true, + force: true, }, []podDrainHandler{deletePod}, &expectation{ @@ -695,8 +695,8 @@ var _ = Describe("drain", func() { maxEvictRetries: 1, attemptEviction: true, terminationGracePeriod: terminationGracePeriodShort, - force: true, - evictError: apierrors.NewTooManyRequestsError(""), + force: true, + evictError: apierrors.NewTooManyRequestsError(""), }, nil, &expectation{ diff --git a/pkg/driver/driver_alicloud.go b/pkg/driver/driver_alicloud.go index 065691913..3e873f7d0 100644 --- a/pkg/driver/driver_alicloud.go +++ b/pkg/driver/driver_alicloud.go @@ -77,28 +77,7 @@ func (c *AlicloudDriver) Create() (string, string, error) { } if ok := c.AlicloudMachineClass.Spec.DataDisks != nil; ok { - disks := c.AlicloudMachineClass.Spec.DataDisks - var dataDiskRequests []ecs.RunInstancesDataDisk - - for _, disk := range disks { - - dataDiskRequest := ecs.RunInstancesDataDisk{ - Category: disk.Category, - DeleteWithInstance: strconv.FormatBool(true), - Encrypted: strconv.FormatBool(disk.Encrypted), - DiskName: disk.Name, - Description: disk.Description, - } - - dataDiskRequest.Size = fmt.Sprintf("%d", disk.Size) - - if disk.Category == "DiskEphemeralSSD" { - dataDiskRequest.DeleteWithInstance = "" - } - - dataDiskRequests = append(dataDiskRequests, dataDiskRequest) - } - + dataDiskRequests := c.generateDataDiskRequests(c.AlicloudMachineClass.Spec.DataDisks) request.DataDisk = &dataDiskRequests } @@ -133,6 +112,28 @@ func (c *AlicloudDriver) Create() (string, string, error) { return machineID, strings.ToLower(c.idToName(instanceID)), nil } +func (c *AlicloudDriver) generateDataDiskRequests(disks []v1alpha1.AlicloudDataDisk) []ecs.RunInstancesDataDisk { + var dataDiskRequests []ecs.RunInstancesDataDisk + for _, disk := range disks { + + dataDiskRequest := ecs.RunInstancesDataDisk{ + Category: disk.Category, + DeleteWithInstance: strconv.FormatBool(disk.DeleteWithInstance), + Encrypted: strconv.FormatBool(disk.Encrypted), + DiskName: fmt.Sprintf("%s-%s-data-disk", c.MachineName, disk.Name), + Description: disk.Description, + Size: fmt.Sprintf("%d", disk.Size), + } + + if disk.Category == "DiskEphemeralSSD" { + dataDiskRequest.DeleteWithInstance = "" + } + + dataDiskRequests = append(dataDiskRequests, dataDiskRequest) + } + return dataDiskRequests +} + // Delete method is used to delete an alicloud machine func (c *AlicloudDriver) Delete(machineID string) error { result, err := c.getVMDetails(machineID) diff --git a/pkg/driver/driver_alicloud_test.go b/pkg/driver/driver_alicloud_test.go index a1eb8d46b..5460d21ff 100644 --- a/pkg/driver/driver_alicloud_test.go +++ b/pkg/driver/driver_alicloud_test.go @@ -1,133 +1,186 @@ package driver import ( - "testing" + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "strconv" "github.com/aliyun/alibaba-cloud-sdk-go/services/ecs" ) -func TestTagsOrdered(t *testing.T) { - tags := map[string]string{ - "kubernetes.io/cluster/ali-test": "1", - "kubernetes.io/role/worker": "1", - "taga": "tagvala", - "tagb": "tagvalb", - "tagc": "tagvalc", - } - c := &AlicloudDriver{} - res, err := c.toInstanceTags(tags) - if err != nil { - t.Errorf("toInstanceTags in TestTagsOrdered should not generate error: %v", err) - } - - expected := []ecs.RunInstancesTag{ - { - Key: "kubernetes.io/cluster/ali-test", - Value: "1", - }, - { - Key: "kubernetes.io/role/worker", - Value: "1", - }, - { - Key: "taga", - Value: "tagvala", - }, - { - Key: "tagb", - Value: "tagvalb", - }, - { - Key: "tagc", - Value: "tagvalc", - }, - } - checkRunInstanceTags("Function TestTagsOrdered: ", t, res, expected) -} - -func TestNoClusterTags(t *testing.T) { - tags := map[string]string{ - "kubernetes.io/role/worker": "1", - "taga": "tagvala", - "tagb": "tagvalb", - "tagc": "tagvalc", - } - c := &AlicloudDriver{} - _, err := c.toInstanceTags(tags) - if err == nil { - t.Errorf("toInstanceTags in TestRandomOrderTags should return an error") - } -} -func TestRandomOrderTags(t *testing.T) { - tags := map[string]string{ - "taga": "tagvala", - "tagb": "tagvalb", - "kubernetes.io/cluster/ali-test": "1", - "kubernetes.io/role/worker": "1", - "tagc": "tagvalc", - } - c := &AlicloudDriver{} - res, err := c.toInstanceTags(tags) - if err != nil { - t.Errorf("toInstanceTags in TestRandomOrderTags should not generate error: %v", err) - } - - expected := []ecs.RunInstancesTag{ - { - Key: "kubernetes.io/cluster/ali-test", - Value: "1", - }, - { - Key: "kubernetes.io/role/worker", - Value: "1", - }, - { - Key: "taga", - Value: "tagvala", - }, - { - Key: "tagb", - Value: "tagvalb", - }, - { - Key: "tagc", - Value: "tagvalc", - }, - } - checkRunInstanceTags("Function TestRandomOrderTags: ", t, res, expected) -} -func TestIDToName(t *testing.T) { - id := "i-uf69zddmom11ci7est12" - - c := &AlicloudDriver{} - if "iZuf69zddmom11ci7est12Z" != c.idToName(id) { - t.Error("idToName() is not working") - } -} - -// real[2..]'s order is NOT predicted as tags which generated them is a MAP!!! -func checkRunInstanceTags(leadErrMsg string, t *testing.T, real, expected []ecs.RunInstancesTag) { - if len(real) != len(expected) { - t.Errorf("%s: %s", leadErrMsg, "count of generated tags is not as expected") - return - } - - // index 0 and 1 is static - if real[0] != expected[0] { - t.Errorf("%s: tag %s should be at index %d", leadErrMsg, expected[0], 0) - } - if real[1] != expected[1] { - t.Errorf("%s: tag %s should be at index %d", leadErrMsg, expected[1], 1) - } - -found: - for i := 2; i < len(expected); i++ { - for j := 2; j < len(real); j++ { - if expected[i] == real[j] { - continue found +var _ = Describe("Driver AliCloud", func() { + Context("Generate Instance Tags", func() { + + It("Should maintain order of cluster and worker tags", func() { + tags := map[string]string{ + "kubernetes.io/cluster/ali-test": "1", + "kubernetes.io/role/worker": "1", + "taga": "tagvala", + "tagb": "tagvalb", + "tagc": "tagvalc", + } + c := &AlicloudDriver{} + res, err := c.toInstanceTags(tags) + expected := []ecs.RunInstancesTag{ + { + Key: "kubernetes.io/cluster/ali-test", + Value: "1", + }, + { + Key: "kubernetes.io/role/worker", + Value: "1", + }, + { + Key: "taga", + Value: "tagvala", + }, + { + Key: "tagb", + Value: "tagvalb", + }, + { + Key: "tagc", + Value: "tagvalc", + }, + } + Expect(err).ToNot(HaveOccurred()) + Expect(res[0:2]).To(Equal(expected[0:2])) + Expect(res[2:5]).To(ConsistOf(expected[2:5])) + }) + + It("Should fail if no cluster tags", func() { + tags := map[string]string{ + "kubernetes.io/role/worker": "1", + "taga": "tagvala", + "tagb": "tagvalb", + "tagc": "tagvalc", + } + c := &AlicloudDriver{} + _, err := c.toInstanceTags(tags) + Expect(err).To(HaveOccurred()) + }) + + It("should order cluster and worker tags", func() { + tags := map[string]string{ + "taga": "tagvala", + "tagb": "tagvalb", + "kubernetes.io/cluster/ali-test": "1", + "kubernetes.io/role/worker": "1", + "tagc": "tagvalc", } - } - t.Errorf("%s: tag %s is not in real tags", leadErrMsg, expected[i]) - return - } -} + c := &AlicloudDriver{} + res, err := c.toInstanceTags(tags) + + expected := []ecs.RunInstancesTag{ + { + Key: "kubernetes.io/cluster/ali-test", + Value: "1", + }, + { + Key: "kubernetes.io/role/worker", + Value: "1", + }, + { + Key: "taga", + Value: "tagvala", + }, + { + Key: "tagb", + Value: "tagvalb", + }, + { + Key: "tagc", + Value: "tagvalc", + }, + } + Expect(err).ToNot(HaveOccurred()) + Expect(res[0:2]).To(Equal(expected[0:2])) + Expect(res[2:5]).To(ConsistOf(expected[2:5])) + }) + + It("Should generate name from ID", func() { + id := "i-uf69zddmom11ci7est12" + expectedName := "iZuf69zddmom11ci7est12Z" + c := &AlicloudDriver{} + res := c.idToName(id) + Expect(res).To(Equal(expectedName)) + }) + + }) + + Context("Generate Data Disk Requests", func() { + + It("should generate multiple data disk requests", func() { + c := &AlicloudDriver{} + c.MachineName = "machinename" + dataDisks := []v1alpha1.AlicloudDataDisk{ + { + Name: "dd1", + Category: "cloud_efficiency", + Description: "this is a disk", + DeleteWithInstance: true, + Encrypted: true, + Size: 100, + }, + { + Name: "dd2", + Category: "cloud_ssd", + Description: "this is also a disk", + DeleteWithInstance: false, + Encrypted: false, + Size: 50, + }, + } + + generatedDataDisksRequests := c.generateDataDiskRequests(dataDisks) + expectedDataDiskRequests := []ecs.RunInstancesDataDisk{ + { + Size: "100", + Category: "cloud_efficiency", + Encrypted: strconv.FormatBool(true), + DiskName: "machinename-dd1-data-disk", + Description: "this is a disk", + DeleteWithInstance: strconv.FormatBool(true), + }, + { + Size: "50", + Category: "cloud_ssd", + Encrypted: strconv.FormatBool(false), + DiskName: "machinename-dd2-data-disk", + Description: "this is also a disk", + DeleteWithInstance: strconv.FormatBool(false), + }, + } + + Expect(generatedDataDisksRequests).To(Equal(expectedDataDiskRequests)) + }) + + It("should not encrypt or delete with instance by default", func() { + c := &AlicloudDriver{} + c.MachineName = "machinename" + dataDisks := []v1alpha1.AlicloudDataDisk{ + { + Name: "dd1", + Category: "cloud_efficiency", + Description: "this is a disk", + Size: 100, + }, + } + + generatedDataDisksRequests := c.generateDataDiskRequests(dataDisks) + expectedDataDiskRequests := []ecs.RunInstancesDataDisk{ + { + Size: "100", + Category: "cloud_efficiency", + Encrypted: strconv.FormatBool(false), + DiskName: "machinename-dd1-data-disk", + Description: "this is a disk", + DeleteWithInstance: strconv.FormatBool(false), + }, + } + + Expect(generatedDataDisksRequests).To(Equal(expectedDataDiskRequests)) + }) + }) +}) diff --git a/pkg/driver/driver_aws_test.go b/pkg/driver/driver_aws_test.go index 0319293ec..bcf456005 100644 --- a/pkg/driver/driver_aws_test.go +++ b/pkg/driver/driver_aws_test.go @@ -18,21 +18,14 @@ limitations under the License. package driver import ( - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" - "testing" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" v1alpha1 "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" ) -func TestAWSDriverSuite(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Driver AWS Suite") -} - var _ = Describe("Driver AWS", func() { Context("GenerateSecurityGroups Driver AWS Spec", func() { @@ -98,7 +91,8 @@ var _ = Describe("Driver AWS", func() { }, } - Expect(tagsGenerated).To(Equal(expectedTags)) + Expect(tagsGenerated.ResourceType).To(Equal(expectedTags.ResourceType)) + Expect(tagsGenerated.Tags).To(ConsistOf(expectedTags.Tags)) Expect(err).ToNot(HaveOccurred()) }) @@ -124,7 +118,7 @@ var _ = Describe("Driver AWS", func() { Context("GenerateBlockDevices Driver AWS Spec", func() { - It("should convert mutliples blockDevices successfully", func() { + It("should convert multiple blockDevices successfully", func() { awsDriver := &AWSDriver{} disks := []v1alpha1.AWSBlockDeviceMappingSpec{ { @@ -221,5 +215,35 @@ var _ = Describe("Driver AWS", func() { Expect(err).ToNot(HaveOccurred()) }) + It("should not encrypt or delete on termination blockDevices by default", func() { + awsDriver := &AWSDriver{} + disks := []v1alpha1.AWSBlockDeviceMappingSpec{ + { + Ebs: v1alpha1.AWSEbsBlockDeviceSpec{ + VolumeSize: 32, + VolumeType: "gp2", + }, + }, + } + + rootDevice := aws.String("/dev/sda") + disksGenerated, err := awsDriver.generateBlockDevices(disks, rootDevice) + expectedDisks := []*ec2.BlockDeviceMapping{ + { + DeviceName: aws.String("/dev/sda"), + Ebs: &ec2.EbsBlockDevice{ + DeleteOnTermination: aws.Bool(false), + Encrypted: aws.Bool(false), + VolumeSize: aws.Int64(32), + Iops: nil, + VolumeType: aws.String("gp2"), + }, + }, + } + + Expect(disksGenerated).To(Equal(expectedDisks)) + Expect(err).ToNot(HaveOccurred()) + }) + }) }) diff --git a/pkg/driver/driver_azure.go b/pkg/driver/driver_azure.go index cd52dd34e..ac4684613 100644 --- a/pkg/driver/driver_azure.go +++ b/pkg/driver/driver_azure.go @@ -25,7 +25,7 @@ import ( "strings" "sync" - v1alpha1 "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" "github.com/gardener/machine-controller-manager/pkg/metrics" "github.com/prometheus/client_golang/prometheus" corev1 "k8s.io/api/core/v1" @@ -99,6 +99,9 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID tagList[idx] = to.StringPtr(element) } + azureDataDisks := d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks + dataDisks := d.generateDataDisks(vmName, azureDataDisks) + publisher, offer, sku, version := getAzureImageDetails(d) VMParameters := compute.VirtualMachine{ @@ -124,6 +127,7 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID DiskSizeGB: &d.AzureMachineClass.Spec.Properties.StorageProfile.OsDisk.DiskSizeGB, CreateOption: compute.DiskCreateOptionTypes(d.AzureMachineClass.Spec.Properties.StorageProfile.OsDisk.CreateOption), }, + DataDisks: &dataDisks, }, OsProfile: &compute.OSProfile{ ComputerName: &vmName, @@ -166,6 +170,27 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID return VMParameters } +func (d *AzureDriver) generateDataDisks(vmName string, azureDataDisks []v1alpha1.AzureDataDisk) []compute.DataDisk { + var dataDisks []compute.DataDisk + for _, azureDataDisk := range azureDataDisks { + dataDiskName := dependencyNameFromVMNameAndDependancy(azureDataDisk.Name, vmName, dataDiskSuffix) + dataDiskLun := azureDataDisk.Lun + dataDiskSize := azureDataDisk.DiskSizeGB + dataDisk := compute.DataDisk{ + Lun: &dataDiskLun, + Name: &dataDiskName, + Caching: compute.CachingTypes(azureDataDisk.Caching), + ManagedDisk: &compute.ManagedDiskParameters{ + StorageAccountType: compute.StorageAccountTypes(azureDataDisk.ManagedDisk.StorageAccountType), + }, + DiskSizeGB: &dataDiskSize, + CreateOption: compute.DiskCreateOptionTypes(azureDataDisk.CreateOption), + } + dataDisks = append(dataDisks, dataDisk) + } + return dataDisks +} + func getAzureImageDetails(d *AzureDriver) (publisher, offer, sku, version string) { splits := strings.Split(*d.AzureMachineClass.Spec.Properties.StorageProfile.ImageReference.URN, ":") publisher = splits[0] @@ -200,12 +225,14 @@ func (d *AzureDriver) Delete(machineID string) error { var ( ctx = context.Background() vmName = decodeMachineID(machineID) + azDataDiskNames = getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) nicName = dependencyNameFromVMName(vmName, nicSuffix) diskName = dependencyNameFromVMName(vmName, diskSuffix) resourceGroupName = d.AzureMachineClass.Spec.ResourceGroup + dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) ) - return clients.deleteVMNicDisk(ctx, resourceGroupName, vmName, nicName, diskName) + return clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) } // GetExisting method is used to fetch the machineID for an azure machine @@ -331,8 +358,10 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { vnetName = d.AzureMachineClass.Spec.SubnetInfo.VnetName vnetResourceGroup = resourceGroupName subnetName = d.AzureMachineClass.Spec.SubnetInfo.SubnetName + azDataDiskNames = getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) nicName = dependencyNameFromVMName(vmName, nicSuffix) diskName = dependencyNameFromVMName(vmName, diskSuffix) + dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) ) clients, err := d.setup() @@ -372,7 +401,7 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { NICFuture, err := clients.nic.CreateOrUpdate(ctx, resourceGroupName, *NICParameters.Name, NICParameters) if err != nil { // Since machine creation failed, delete any infra resources created - deleteErr := clients.deleteVMNicDisk(ctx, resourceGroupName, vmName, nicName, diskName) + deleteErr := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) if deleteErr != nil { glog.Errorf("Error occurred during resource clean up: %s", deleteErr) } @@ -384,7 +413,7 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { err = NICFuture.WaitForCompletionRef(ctx, clients.nic.Client) if err != nil { // Since machine creation failed, delete any infra resources created - deleteErr := clients.deleteVMNicDisk(ctx, resourceGroupName, vmName, nicName, diskName) + deleteErr := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) if deleteErr != nil { glog.Errorf("Error occurred during resource clean up: %s", deleteErr) } @@ -397,7 +426,7 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { NIC, err := NICFuture.Result(clients.nic) if err != nil { // Since machine creation failed, delete any infra resources created - deleteErr := clients.deleteVMNicDisk(ctx, resourceGroupName, vmName, nicName, diskName) + deleteErr := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) if deleteErr != nil { glog.Errorf("Error occurred during resource clean up: %s", deleteErr) } @@ -416,7 +445,7 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { VMFuture, err := clients.vm.CreateOrUpdate(ctx, resourceGroupName, *VMParameters.Name, VMParameters) if err != nil { //Since machine creation failed, delete any infra resources created - deleteErr := clients.deleteVMNicDisk(ctx, resourceGroupName, vmName, nicName, diskName) + deleteErr := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) if deleteErr != nil { glog.Errorf("Error occurred during resource clean up: %s", deleteErr) } @@ -428,7 +457,7 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { err = VMFuture.WaitForCompletionRef(ctx, clients.vm.Client) if err != nil { // Since machine creation failed, delete any infra resources created - deleteErr := clients.deleteVMNicDisk(ctx, resourceGroupName, vmName, nicName, diskName) + deleteErr := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) if deleteErr != nil { glog.Errorf("Error occurred during resource clean up: %s", deleteErr) } @@ -440,7 +469,7 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { VM, err := VMFuture.Result(clients.vm) if err != nil { // Since machine creation failed, delete any infra resources created - deleteErr := clients.deleteVMNicDisk(ctx, resourceGroupName, vmName, nicName, diskName) + deleteErr := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) if deleteErr != nil { glog.Errorf("Error occurred during resource clean up: %s", deleteErr) } @@ -682,7 +711,7 @@ func (clients *azureDriverClients) fetchAttachedVMfromDisk(ctx context.Context, return *disk.ManagedBy, nil } -func (clients *azureDriverClients) deleteVMNicDisk(ctx context.Context, resourceGroupName string, VMName string, nicName string, diskName string) error { +func (clients *azureDriverClients) deleteVMNicDisks(ctx context.Context, resourceGroupName string, VMName string, nicName string, diskName string, dataDiskNames []string) error { // We try to fetch the VM, detach its data disks and finally delete it if vm, vmErr := clients.vm.Get(ctx, resourceGroupName, VMName, ""); vmErr == nil { @@ -698,6 +727,8 @@ func (clients *azureDriverClients) deleteVMNicDisk(ctx context.Context, resource return onARMAPIErrorFail(prometheusServiceVM, vmErr, "vm.Get") } + var deleters []func() error + // Fetch the NIC and deleted it nicDeleter := func() error { if vmHoldingNic, err := clients.fetchAttachedVMfromNIC(ctx, resourceGroupName, nicName); err != nil { @@ -713,8 +744,22 @@ func (clients *azureDriverClients) deleteVMNicDisk(ctx context.Context, resource return clients.deleteNIC(ctx, resourceGroupName, nicName) } + deleters = append(deleters, nicDeleter) + // Fetch the system disk and delete it - diskDeleter := func() error { + diskDeleter := clients.getDeleterForDisk(ctx, resourceGroupName, diskName) + deleters = append(deleters, diskDeleter) + + for _, dataDiskName := range dataDiskNames { + dataDiskDeleter := clients.getDeleterForDisk(ctx, resourceGroupName, dataDiskName) + deleters = append(deleters, dataDiskDeleter) + } + + return runInParallel(deleters) +} + +func (clients *azureDriverClients) getDeleterForDisk(ctx context.Context, resourceGroupName string, diskName string) func() error { + return func() error { if vmHoldingDisk, err := clients.fetchAttachedVMfromDisk(ctx, resourceGroupName, diskName); err != nil { if notFound(err) { // Resource doesn't exist, no need to delete @@ -727,8 +772,6 @@ func (clients *azureDriverClients) deleteVMNicDisk(ctx context.Context, resource return clients.deleteDisk(ctx, resourceGroupName, diskName) } - - return runInParallel(nicDeleter, diskDeleter) } // waitForDataDiskDetachment waits for data disks to be detached @@ -803,8 +846,8 @@ func (clients *azureDriverClients) deleteNIC(ctx context.Context, resourceGroupN } func (clients *azureDriverClients) deleteDisk(ctx context.Context, resourceGroupName string, diskName string) error { - glog.V(2).Infof("System disk delete started for %q", diskName) - defer glog.V(2).Infof("System disk deleted for %q", diskName) + glog.V(2).Infof("Disk delete started for %q", diskName) + defer glog.V(2).Infof("Disk deleted for %q", diskName) future, err := clients.disk.Delete(ctx, resourceGroupName, diskName) if err != nil { @@ -814,7 +857,7 @@ func (clients *azureDriverClients) deleteDisk(ctx context.Context, resourceGroup if err != nil { return onARMAPIErrorFail(prometheusServiceDisk, err, "disk.Delete") } - onARMAPISuccess(prometheusServiceDisk, "OS-Disk deletion was successful for %s", diskName) + onARMAPISuccess(prometheusServiceDisk, "Disk deletion was successful for %s", diskName) return nil } @@ -859,7 +902,7 @@ func onErrorFail(err error, format string, v ...interface{}) error { return err } -func runInParallel(funcs ...(func() error)) error { +func runInParallel(funcs []func() error) error { // // Execute multiple functions (which return an error) as go functions concurrently. // @@ -903,10 +946,31 @@ func decodeMachineID(id string) string { } const ( - nicSuffix = "-nic" - diskSuffix = "-os-disk" + nicSuffix = "-nic" + diskSuffix = "-os-disk" + dataDiskSuffix = "-data-disk" ) +func getAzureDataDiskNames(azureDataDisks []v1alpha1.AzureDataDisk) []string { + azureDataDiskNames := make([]string, len(azureDataDisks)) + for i, v := range azureDataDisks { + azureDataDiskNames[i] = v.Name + } + return azureDataDiskNames +} + +func dependencyNamesFromVMName(dependencies []string, vmname, suffix string) []string { + dependencyNames := make([]string, len(dependencies)) + for i, dependency := range dependencies { + dependencyNames[i] = dependencyNameFromVMNameAndDependancy(dependency, vmname, suffix) + } + return dependencyNames +} + +func dependencyNameFromVMNameAndDependancy(dependency, vmName, suffix string) string { + return vmName + "-" + dependency + suffix +} + func dependencyNameFromVMName(vmName, suffix string) string { return vmName + suffix } diff --git a/pkg/driver/driver_azure_test.go b/pkg/driver/driver_azure_test.go new file mode 100644 index 000000000..1f1895fd6 --- /dev/null +++ b/pkg/driver/driver_azure_test.go @@ -0,0 +1,90 @@ +/* +Copyright (c) 2020 SAP SE or an SAP affiliate company. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package driver + +import ( + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute" + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("Driver Azure", func() { + + Context("GenerateDataDisks Driver Azure Spec", func() { + + It("should convert multiple dataDisks successfully", func() { + azureDriver := &AzureDriver{} + vmName := "vm" + lun1 := int32(1) + lun2 := int32(2) + size1 := int32(10) + size2 := int32(100) + expectedName1 := "vm-sdb-data-disk" + expectedName2 := "vm-sdc-data-disk" + disks := []v1alpha1.AzureDataDisk{ + { + Name: "sdb", + Caching: "None", + ManagedDisk: v1alpha1.AzureManagedDiskParameters{ + StorageAccountType: "Premium_LRS", + }, + DiskSizeGB: size1, + CreateOption: "Empty", + Lun: lun1, + }, + { + Name: "sdc", + Caching: "None", + ManagedDisk: v1alpha1.AzureManagedDiskParameters{ + StorageAccountType: "Standard_LRS", + }, + DiskSizeGB: size2, + CreateOption: "FromImage", + Lun: lun2, + }, + } + + disksGenerated := azureDriver.generateDataDisks(vmName, disks) + expectedDisks := []compute.DataDisk{ + { + Lun: &lun1, + Name: &expectedName1, + Caching: compute.CachingTypes("None"), + ManagedDisk: &compute.ManagedDiskParameters{ + StorageAccountType: compute.StorageAccountTypes("Premium_LRS"), + }, + DiskSizeGB: &size1, + CreateOption: compute.DiskCreateOptionTypes("Empty"), + }, + { + Lun: &lun2, + Name: &expectedName2, + Caching: compute.CachingTypes("None"), + ManagedDisk: &compute.ManagedDiskParameters{ + StorageAccountType: compute.StorageAccountTypes("Standard_LRS"), + }, + DiskSizeGB: &size2, + CreateOption: compute.DiskCreateOptionTypes("FromImage"), + }, + } + + Expect(disksGenerated).To(Equal(expectedDisks)) + }) + + }) +}) diff --git a/pkg/driver/driver_test.go b/pkg/driver/driver_test.go new file mode 100644 index 000000000..3900f77b9 --- /dev/null +++ b/pkg/driver/driver_test.go @@ -0,0 +1,10 @@ +package driver + +import "testing" +import . "github.com/onsi/ginkgo" +import . "github.com/onsi/gomega" + +func TestDriverSuite(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Driver Suite") +} diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index cfe470bc5..ca93c5d6f 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -360,6 +360,52 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, Dependencies: []string{}, }, + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudDataDisk": { + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{ + "name": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "category": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "description": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "encrypted": { + SchemaProps: spec.SchemaProps{ + Type: []string{"boolean"}, + Format: "", + }, + }, + "deleteWithInstance": { + SchemaProps: spec.SchemaProps{ + Type: []string{"boolean"}, + Format: "", + }, + }, + "size": { + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + Format: "int32", + }, + }, + }, + Required: []string{"name", "category", "encrypted", "deleteWithInstance", "size"}, + }, + }, + Dependencies: []string{}, + }, "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudMachineClass": { Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ @@ -489,6 +535,18 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Ref: ref("github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudSystemDisk"), }, }, + "dataDisks": { + SchemaProps: spec.SchemaProps{ + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Ref: ref("github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudDataDisk"), + }, + }, + }, + }, + }, "instanceChargeType": { SchemaProps: spec.SchemaProps{ Type: []string{"string"}, @@ -554,7 +612,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, }, Dependencies: []string{ - "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudSystemDisk", "k8s.io/api/core/v1.SecretReference"}, + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudDataDisk", "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudSystemDisk", "k8s.io/api/core/v1.SecretReference"}, }, "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AlicloudSystemDisk": { Schema: spec.Schema{ @@ -579,6 +637,51 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, Dependencies: []string{}, }, + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureDataDisk": { + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{ + "name": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "lun": { + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + Format: "int32", + }, + }, + "caching": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "managedDisk": { + SchemaProps: spec.SchemaProps{ + Ref: ref("github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureManagedDiskParameters"), + }, + }, + "diskSizeGB": { + SchemaProps: spec.SchemaProps{ + Type: []string{"integer"}, + Format: "int32", + }, + }, + "createOption": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, + Dependencies: []string{ + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureManagedDiskParameters"}, + }, "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureHardwareProfile": { Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ @@ -977,11 +1080,23 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Ref: ref("github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureOSDisk"), }, }, + "dataDisks": { + SchemaProps: spec.SchemaProps{ + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Ref: ref("github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureDataDisk"), + }, + }, + }, + }, + }, }, }, }, Dependencies: []string{ - "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureImageReference", "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureOSDisk"}, + "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureDataDisk", "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureImageReference", "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureOSDisk"}, }, "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureSubResource": { Schema: spec.Schema{ @@ -1876,13 +1991,6 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA SchemaProps: spec.SchemaProps{ Description: "MachineDeploymentSpec is the specification of the desired behavior of the MachineDeployment.", Properties: map[string]spec.Schema{ - "replicas": { - SchemaProps: spec.SchemaProps{ - Description: "Number of desired machines. This is a pointer to distinguish between explicit zero and not specified. Defaults to 0.", - Type: []string{"integer"}, - Format: "int32", - }, - }, "selector": { SchemaProps: spec.SchemaProps{ Description: "Label selector for machines. Existing MachineSets whose machines are selected by this will be the ones affected by this MachineDeployment.", @@ -1959,41 +2067,6 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Format: "int64", }, }, - "replicas": { - SchemaProps: spec.SchemaProps{ - Description: "Total number of non-terminated machines targeted by this MachineDeployment (their labels match the selector).", - Type: []string{"integer"}, - Format: "int32", - }, - }, - "updatedReplicas": { - SchemaProps: spec.SchemaProps{ - Description: "Total number of non-terminated machines targeted by this MachineDeployment that have the desired template spec.", - Type: []string{"integer"}, - Format: "int32", - }, - }, - "readyReplicas": { - SchemaProps: spec.SchemaProps{ - Description: "Total number of ready machines targeted by this MachineDeployment.", - Type: []string{"integer"}, - Format: "int32", - }, - }, - "availableReplicas": { - SchemaProps: spec.SchemaProps{ - Description: "Total number of available machines (ready for at least minReadySeconds) targeted by this MachineDeployment.", - Type: []string{"integer"}, - Format: "int32", - }, - }, - "unavailableReplicas": { - SchemaProps: spec.SchemaProps{ - Description: "Total number of unavailable machines targeted by this MachineDeployment. This is the total number of machines that are still required for the MachineDeployment to have 100% available capacity. They may either be machines that are running but not yet available or machines that still have not been created.", - Type: []string{"integer"}, - Format: "int32", - }, - }, "conditions": { VendorExtensible: spec.VendorExtensible{ Extensions: spec.Extensions{ @@ -9834,7 +9907,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Properties: map[string]spec.Schema{ "replicas": { SchemaProps: spec.SchemaProps{ - Description: "Replicas is the number of desired replicas. This is a pointer to distinguish between explicit zero and unspecified. Defaults to 0. More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller", + Description: "Replicas is the number of desired replicas. This is a pointer to distinguish between explicit zero and unspecified. Defaults to 1. More info: https://kubernetes.io/docs/concepts/workloads/controllers/replicationcontroller#what-is-a-replicationcontroller", Type: []string{"integer"}, Format: "int32", }, From 0df6b24f91d0e25d0234ff1e0f9f4e864f981b48 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Mon, 10 Feb 2020 18:17:01 +0200 Subject: [PATCH 3/9] fix review comments, update tests and add validation tests --- .../validation/alicloudmachineclass.go | 29 ++++ .../validation/alicloudmachineclass_test.go | 126 ++++++++++++++ .../machine/validation/awsmachineclass.go | 2 +- .../validation/awsmachineclass_test.go | 19 +- .../machine/validation/azuremachineclass.go | 50 +++--- .../validation/azuremachineclass_test.go | 164 ++++++++++++++++++ .../validation/validation_suite_test.go | 12 ++ pkg/driver/driver_alicloud.go | 2 +- pkg/driver/driver_azure.go | 40 +++-- pkg/driver/driver_azure_test.go | 4 +- 10 files changed, 393 insertions(+), 55 deletions(-) create mode 100644 pkg/apis/machine/validation/alicloudmachineclass_test.go create mode 100644 pkg/apis/machine/validation/azuremachineclass_test.go create mode 100644 pkg/apis/machine/validation/validation_suite_test.go diff --git a/pkg/apis/machine/validation/alicloudmachineclass.go b/pkg/apis/machine/validation/alicloudmachineclass.go index c5e3bd74e..2bef4ab8d 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass.go +++ b/pkg/apis/machine/validation/alicloudmachineclass.go @@ -18,6 +18,7 @@ limitations under the License. package validation import ( + "fmt" "strings" "github.com/gardener/machine-controller-manager/pkg/apis/machine" @@ -63,6 +64,34 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl allErrs = append(allErrs, field.Required(fldPath.Child("keyPairName"), "KeyPairName is required")) } + if spec.DataDisks != nil { + names := map[string]int{} + for i, dataDisk := range spec.DataDisks { + idxPath := fldPath.Child("dataDisks").Index(i) + if _, keyExist := names[dataDisk.Name]; keyExist { + names[dataDisk.Name]++ + } else { + names[dataDisk.Name] = 1 + } + + if dataDisk.Name == "" { + allErrs = append(allErrs, field.Required(idxPath.Child("name"), "Data Disk name is required")) + } + if dataDisk.Size <= 0 { + allErrs = append(allErrs, field.Required(idxPath.Child("size"), "Data Disk size must be positive")) + } + if dataDisk.Category == "" { + allErrs = append(allErrs, field.Required(idxPath.Child("category"), "Data Disk category is required")) + } + } + + for name, number := range names { + if number > 1 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("dataDisks"), name, fmt.Sprintf("Data Disk Name '%s' duplicated %d times, Name must be unique", name, number))) + } + } + } + allErrs = append(allErrs, validateSecretRef(spec.SecretRef, field.NewPath("spec.secretRef"))...) allErrs = append(allErrs, validateAlicloudClassSpecTags(spec.Tags, field.NewPath("spec.tags"))...) diff --git a/pkg/apis/machine/validation/alicloudmachineclass_test.go b/pkg/apis/machine/validation/alicloudmachineclass_test.go new file mode 100644 index 000000000..724419ea8 --- /dev/null +++ b/pkg/apis/machine/validation/alicloudmachineclass_test.go @@ -0,0 +1,126 @@ +package validation + +import ( + "github.com/gardener/machine-controller-manager/pkg/apis/machine" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +func getAliCloudMachineSpec() *machine.AlicloudMachineClassSpec { + bandwidth := 5 + return &machine.AlicloudMachineClassSpec{ + ImageID: "coreos_1745_7_0_64_30G_alibase_20180705.vhd", + InstanceType: "ecs.n1.medium", + Region: "cn-hangzhou", + ZoneID: "cn-hangzhou-e", + SecurityGroupID: "sg-1234567890", + VSwitchID: "vsw-1234567890", + SystemDisk: &machine.AlicloudSystemDisk{ + Category: "cloud_efficiency", + Size: 75, + }, + DataDisks: []machine.AlicloudDataDisk{ + { + Name: "dd1", + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, + DeleteWithInstance: true, + }, + { + Name: "dd2", + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, + DeleteWithInstance: true, + }, + }, + InstanceChargeType: "PostPaid", + InternetChargeType: "PayByTraffic", + InternetMaxBandwidthIn: &bandwidth, + InternetMaxBandwidthOut: &bandwidth, + SpotStrategy: "NoSpot", + KeyPairName: "my-ssh-key", + Tags: map[string]string{ + "kubernetes.io/cluster/shoot": "1", + "kubernetes.io/role/role": "1", + }, + SecretRef: &corev1.SecretReference{ + Name: "test-secret", + Namespace: "test-namespace", + }, + } +} + +var _ = Describe("AliCloudMachineClass Validation", func() { + + Context("Validate AliCloudMachineClass Spec", func() { + + It("should validate an object successfully", func() { + + spec := getAliCloudMachineSpec() + err := validateAlicloudMachineClassSpec(spec, field.NewPath("spec")) + + Expect(err).To(Equal(field.ErrorList{})) + }) + + It("should get an error on DataDisks validation", func() { + spec := getAliCloudMachineSpec() + spec.DataDisks = []machine.AlicloudDataDisk{ + { + Name: "", + Category: "", + Size: -1, + Encrypted: true, + DeleteWithInstance: true, + }, + { + Name: "dd1", + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, + DeleteWithInstance: true, + }, + { + Name: "dd1", + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, + DeleteWithInstance: true, + }, + } + + errList := validateAlicloudMachineClassSpec(spec, field.NewPath("spec")) + + errExpected := field.ErrorList{ + { + Type: field.ErrorTypeRequired, + Field: "spec.dataDisks[0].name", + BadValue: "", + Detail: "Data Disk name is required", + }, + { + Type: field.ErrorTypeRequired, + Field: "spec.dataDisks[0].size", + BadValue: "", + Detail: "Data Disk size must be positive", + }, + { + Type: field.ErrorTypeRequired, + Field: "spec.dataDisks[0].category", + BadValue: "", + Detail: "Data Disk category is required", + }, + { + Type: field.ErrorTypeInvalid, + Field: "spec.dataDisks", + BadValue: "dd1", + Detail: "Data Disk Name 'dd1' duplicated 2 times, Name must be unique", + }, + } + Expect(errList).To(ConsistOf(errExpected)) + }) + }) +}) diff --git a/pkg/apis/machine/validation/awsmachineclass.go b/pkg/apis/machine/validation/awsmachineclass.go index 209cb7a3a..1f1fd8f99 100644 --- a/pkg/apis/machine/validation/awsmachineclass.go +++ b/pkg/apis/machine/validation/awsmachineclass.go @@ -133,7 +133,7 @@ func validateBlockDevices(blockDevices []machine.AWSBlockDeviceMappingSpec, fldP validVolumeTypes := []string{"gp2", "io1", "st1", "sc1", "standard"} // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html - const dataDeviceNameFmt string = `^/dev/(sd[a-z]|xvd[a-c][a-z]?)$` + const dataDeviceNameFmt string = `/dev/(sd[a-z]|xvd[a-c][a-z]?)` var dataDeviceNameRegexp = regexp.MustCompile("^" + dataDeviceNameFmt + "$") // if blockDevices is empty, AWS will automatically create a root partition diff --git a/pkg/apis/machine/validation/awsmachineclass_test.go b/pkg/apis/machine/validation/awsmachineclass_test.go index 9235f60f5..33321bdbc 100644 --- a/pkg/apis/machine/validation/awsmachineclass_test.go +++ b/pkg/apis/machine/validation/awsmachineclass_test.go @@ -18,20 +18,13 @@ limitations under the License. package validation import ( + "github.com/gardener/machine-controller-manager/pkg/apis/machine" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "testing" - - "github.com/gardener/machine-controller-manager/pkg/apis/machine" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation/field" ) -func TestAWSMachineClassSuite(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "AWSMachineClass Suite") -} - func getSpec() *machine.AWSMachineClassSpec { return &machine.AWSMachineClassSpec{ AMI: "ami-99fn8a892f94e765a", @@ -291,14 +284,14 @@ var _ = Describe("AWSMachineClass Validation", func() { spec := getSpec() spec.BlockDevices = []machine.AWSBlockDeviceMappingSpec{ { - DeviceName: "/dev/vdga", + DeviceName: "/dev/sdf", Ebs: machine.AWSEbsBlockDeviceSpec{ VolumeSize: 10, VolumeType: "gp2", }, }, { - DeviceName: "/dev/vdga", + DeviceName: "/dev/sdf", Ebs: machine.AWSEbsBlockDeviceSpec{ VolumeSize: 15, VolumeType: "gp2", @@ -319,7 +312,7 @@ var _ = Describe("AWSMachineClass Validation", func() { Type: "FieldValueRequired", Field: "spec.blockDevices", BadValue: "", - Detail: "Device name '/dev/vdga' duplicated 2 times, DeviceName must be uniq", + Detail: "Device name '/dev/sdf' duplicated 2 times, DeviceName must be uniq", }, } Expect(err).To(Equal(errExpected)) @@ -329,7 +322,7 @@ var _ = Describe("AWSMachineClass Validation", func() { spec := getSpec() spec.BlockDevices = []machine.AWSBlockDeviceMappingSpec{ { - DeviceName: "/dev/vdga", + DeviceName: "/dev/sdf", Ebs: machine.AWSEbsBlockDeviceSpec{ VolumeSize: 10, VolumeType: "gp2-invalid", @@ -378,7 +371,7 @@ var _ = Describe("AWSMachineClass Validation", func() { }, }, { - DeviceName: "/dev/vdga", + DeviceName: "/dev/sdf", Ebs: machine.AWSEbsBlockDeviceSpec{ VolumeSize: 15, VolumeType: "gp2", diff --git a/pkg/apis/machine/validation/azuremachineclass.go b/pkg/apis/machine/validation/azuremachineclass.go index f75011319..6cd299155 100644 --- a/pkg/apis/machine/validation/azuremachineclass.go +++ b/pkg/apis/machine/validation/azuremachineclass.go @@ -115,36 +115,36 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.osDisk.createOption"), "OSDisk create option is required")) } - var luns = make(map[int32]int) - - for i, dataDisk := range properties.StorageProfile.DataDisks { - idxPath := fldPath.Child("storageProfile.dataDisks").Index(i) - if _, keyExist := luns[dataDisk.Lun]; keyExist { - luns[dataDisk.Lun]++ - } else { - luns[dataDisk.Lun] = 1 - } + if properties.StorageProfile.DataDisks != nil { + luns := map[int32]int{} + for i, dataDisk := range properties.StorageProfile.DataDisks { + idxPath := fldPath.Child("storageProfile.dataDisks").Index(i) + if _, keyExist := luns[dataDisk.Lun]; keyExist { + luns[dataDisk.Lun]++ + } else { + luns[dataDisk.Lun] = 1 + } - if dataDisk.Caching == "" { - allErrs = append(allErrs, field.Required(idxPath.Child("caching"), "DataDisk caching is required")) - } - if dataDisk.DiskSizeGB <= 0 { - allErrs = append(allErrs, field.Required(idxPath.Child("diskSizeGB"), "DataDisk size must be positive")) - } - if dataDisk.CreateOption == "" { - allErrs = append(allErrs, field.Required(idxPath.Child("createOption"), "DataDisk create option is required")) - } - if dataDisk.Lun < 0 { - allErrs = append(allErrs, field.Required(idxPath.Child("lun"), "DataDisk Lun is required")) + if dataDisk.Caching == "" { + allErrs = append(allErrs, field.Required(idxPath.Child("caching"), "DataDisk caching is required")) + } + if dataDisk.DiskSizeGB <= 0 { + allErrs = append(allErrs, field.Required(idxPath.Child("diskSizeGB"), "DataDisk size must be positive")) + } + if dataDisk.CreateOption == "" { + allErrs = append(allErrs, field.Required(idxPath.Child("createOption"), "DataDisk create option is required")) + } + if dataDisk.Lun < 0 { + allErrs = append(allErrs, field.Required(idxPath.Child("lun"), "DataDisk Lun is required")) + } } - } - for lun, number := range luns { - if number > 1 { - allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.dataDisks"), fmt.Sprintf("Data Disk Lun '%s' duplicated %d times, Lun must be unique", lun, number))) + for lun, number := range luns { + if number > 1 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("storageProfile.dataDisks"), lun, fmt.Sprintf("Data Disk Lun '%d' duplicated %d times, Lun must be unique", lun, number))) + } } } - if properties.OsProfile.AdminUsername == "" { allErrs = append(allErrs, field.Required(fldPath.Child("osProfile.adminUsername"), "AdminUsername is required")) } diff --git a/pkg/apis/machine/validation/azuremachineclass_test.go b/pkg/apis/machine/validation/azuremachineclass_test.go new file mode 100644 index 000000000..2c05ac394 --- /dev/null +++ b/pkg/apis/machine/validation/azuremachineclass_test.go @@ -0,0 +1,164 @@ +package validation + +import ( + "github.com/gardener/machine-controller-manager/pkg/apis/machine" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +func getAzureMachineSpec() *machine.AzureMachineClassSpec { + urn := "CoreOS:CoreOS:Stable:2303.3.0" + zone := 1 + return &machine.AzureMachineClassSpec{ + Location: "westeurope", + Properties: machine.AzureVirtualMachineProperties{ + HardwareProfile: machine.AzureHardwareProfile{ + VMSize: "Standard_A1_v2", + }, + OsProfile: machine.AzureOSProfile{ + AdminUsername: "core", + LinuxConfiguration: machine.AzureLinuxConfiguration{ + DisablePasswordAuthentication: true, + SSH: machine.AzureSSHConfiguration{ + PublicKeys: machine.AzureSSHPublicKey{ + KeyData: "this-is-a-key", + Path: "/home/core/.ssh/authorized_keys", + }, + }, + }, + }, + StorageProfile: machine.AzureStorageProfile{ + ImageReference: machine.AzureImageReference{ + URN: &urn, + }, + OsDisk: machine.AzureOSDisk{ + Caching: "None", + CreateOption: "FromImage", + DiskSizeGB: 75, + ManagedDisk: machine.AzureManagedDiskParameters{ + StorageAccountType: "Standard_LRS", + }, + }, + DataDisks: []machine.AzureDataDisk{ + { + Lun: 0, + Caching: "None", + CreateOption: "Empty", + DiskSizeGB: 75, + ManagedDisk: machine.AzureManagedDiskParameters{ + StorageAccountType: "Standard_LRS", + }, + }, + { + Lun: 1, + Caching: "None", + CreateOption: "Empty", + DiskSizeGB: 75, + ManagedDisk: machine.AzureManagedDiskParameters{ + StorageAccountType: "Standard_LRS", + }, + }, + }, + }, + Zone: &zone, + }, + ResourceGroup: "resourcegroup", + SubnetInfo: machine.AzureSubnetInfo{ + SubnetName: "subnetnamne", VnetName: "vnetname", + }, + Tags: map[string]string{ + "kubernetes.io-cluster-shoot": "1", + "kubernetes.io-role-node": "1", + }, + SecretRef: &corev1.SecretReference{ + Name: "test-secret", + Namespace: "test-namespace", + }, + } +} + +var _ = Describe("AzureMachineClass Validation", func() { + + Context("Validate AzureMachineClass Spec", func() { + + It("should validate an object successfully", func() { + + spec := getAzureMachineSpec() + err := validateAzureMachineClassSpec(spec, field.NewPath("spec")) + + Expect(err).To(Equal(field.ErrorList{})) + }) + + It("should get an error on DataDisks validation", func() { + spec := getAzureMachineSpec() + spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ + { + Name: "sdb", + }, + } + + errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) + + errExpected := field.ErrorList{ + { + Type: field.ErrorTypeRequired, + Field: "spec.properties.storageProfile.dataDisks[0].caching", + BadValue: "", + Detail: "DataDisk caching is required", + }, + { + Type: field.ErrorTypeRequired, + Field: "spec.properties.storageProfile.dataDisks[0].diskSizeGB", + BadValue: "", + Detail: "DataDisk size must be positive", + }, + { + Type: field.ErrorTypeRequired, + Field: "spec.properties.storageProfile.dataDisks[0].createOption", + BadValue: "", + Detail: "DataDisk create option is required", + }, + } + Expect(errList).To(ConsistOf(errExpected)) + + }) + + It("should get an error on duplicated DataDisks lun ", func() { + spec := getAzureMachineSpec() + spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ + { + Lun: 1, + Caching: "None", + CreateOption: "Empty", + DiskSizeGB: 75, + ManagedDisk: machine.AzureManagedDiskParameters{ + StorageAccountType: "Standard_LRS", + }, + }, + { + Lun: 1, + Caching: "None", + CreateOption: "Empty", + DiskSizeGB: 75, + ManagedDisk: machine.AzureManagedDiskParameters{ + StorageAccountType: "Standard_LRS", + }, + }, + } + + errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) + + errExpected := field.ErrorList{ + { + Type: field.ErrorTypeInvalid, + Field: "spec.properties.storageProfile.dataDisks", + BadValue: int32(1), + Detail: "Data Disk Lun '1' duplicated 2 times, Lun must be unique", + }, + } + Expect(errList).To(ConsistOf(errExpected)) + }) + }) +}) diff --git a/pkg/apis/machine/validation/validation_suite_test.go b/pkg/apis/machine/validation/validation_suite_test.go new file mode 100644 index 000000000..37c9ac1fc --- /dev/null +++ b/pkg/apis/machine/validation/validation_suite_test.go @@ -0,0 +1,12 @@ +package validation + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "testing" +) + +func TestValidation(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Machine API Validation Suite") +} diff --git a/pkg/driver/driver_alicloud.go b/pkg/driver/driver_alicloud.go index a8be23561..faffab68a 100644 --- a/pkg/driver/driver_alicloud.go +++ b/pkg/driver/driver_alicloud.go @@ -76,7 +76,7 @@ func (c *AlicloudDriver) Create() (string, string, error) { request.InternetMaxBandwidthOut = requests.NewInteger(*c.AlicloudMachineClass.Spec.InternetMaxBandwidthOut) } - if ok := c.AlicloudMachineClass.Spec.DataDisks != nil; ok { + if c.AlicloudMachineClass.Spec.DataDisks != nil && len(c.AlicloudMachineClass.Spec.DataDisks) > 0 { dataDiskRequests := c.generateDataDiskRequests(c.AlicloudMachineClass.Spec.DataDisks) request.DataDisk = &dataDiskRequests } diff --git a/pkg/driver/driver_azure.go b/pkg/driver/driver_azure.go index 1c6304d6a..3e2842510 100644 --- a/pkg/driver/driver_azure.go +++ b/pkg/driver/driver_azure.go @@ -171,7 +171,7 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID VMParameters.Identity = &compute.VirtualMachineIdentity{ Type: compute.ResourceIdentityTypeUserAssigned, UserAssignedIdentities: map[string]*compute.VirtualMachineIdentityUserAssignedIdentitiesValue{ - *d.AzureMachineClass.Spec.Properties.IdentityID: &compute.VirtualMachineIdentityUserAssignedIdentitiesValue{}, + *d.AzureMachineClass.Spec.Properties.IdentityID: {}, }, } } @@ -182,7 +182,7 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID func (d *AzureDriver) generateDataDisks(vmName string, azureDataDisks []v1alpha1.AzureDataDisk) []compute.DataDisk { var dataDisks []compute.DataDisk for _, azureDataDisk := range azureDataDisks { - dataDiskName := dependencyNameFromVMNameAndDependancy(azureDataDisk.Name, vmName, dataDiskSuffix) + dataDiskName := dependencyNameFromVMNameAndDependency(getAzureDataDiskName(azureDataDisk), vmName, dataDiskSuffix) dataDiskLun := azureDataDisk.Lun dataDiskSize := azureDataDisk.DiskSizeGB dataDisk := compute.DataDisk{ @@ -234,13 +234,17 @@ func (d *AzureDriver) Delete(machineID string) error { var ( ctx = context.Background() vmName = decodeMachineID(machineID) - azDataDiskNames = getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) nicName = dependencyNameFromVMName(vmName, nicSuffix) diskName = dependencyNameFromVMName(vmName, diskSuffix) resourceGroupName = d.AzureMachineClass.Spec.ResourceGroup - dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) ) + var dataDiskNames []string + if d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks != nil && len(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) > 0 { + azDataDiskNames := getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) + dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) + } + return clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) } @@ -367,10 +371,8 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { vnetName = d.AzureMachineClass.Spec.SubnetInfo.VnetName vnetResourceGroup = resourceGroupName subnetName = d.AzureMachineClass.Spec.SubnetInfo.SubnetName - azDataDiskNames = getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) nicName = dependencyNameFromVMName(vmName, nicSuffix) diskName = dependencyNameFromVMName(vmName, diskSuffix) - dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) ) clients, err := d.setup() @@ -383,6 +385,12 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { vnetResourceGroup = *d.AzureMachineClass.Spec.SubnetInfo.VnetResourceGroup } + var dataDiskNames []string + if d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks != nil && len(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) > 0 { + azDataDiskNames := getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) + dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) + } + /* Subnet fetching */ @@ -759,9 +767,11 @@ func (clients *azureDriverClients) deleteVMNicDisks(ctx context.Context, resourc diskDeleter := clients.getDeleterForDisk(ctx, resourceGroupName, diskName) deleters = append(deleters, diskDeleter) - for _, dataDiskName := range dataDiskNames { - dataDiskDeleter := clients.getDeleterForDisk(ctx, resourceGroupName, dataDiskName) - deleters = append(deleters, dataDiskDeleter) + if dataDiskNames != nil { + for _, dataDiskName := range dataDiskNames { + dataDiskDeleter := clients.getDeleterForDisk(ctx, resourceGroupName, dataDiskName) + deleters = append(deleters, dataDiskDeleter) + } } return runInParallel(deleters) @@ -962,21 +972,25 @@ const ( func getAzureDataDiskNames(azureDataDisks []v1alpha1.AzureDataDisk) []string { azureDataDiskNames := make([]string, len(azureDataDisks)) - for i, v := range azureDataDisks { - azureDataDiskNames[i] = v.Name + for i, disk := range azureDataDisks { + azureDataDiskNames[i] = getAzureDataDiskName(disk) } return azureDataDiskNames } +func getAzureDataDiskName(disk v1alpha1.AzureDataDisk) string { + return fmt.Sprintf("%s-%d", disk.Name, disk.Lun) +} + func dependencyNamesFromVMName(dependencies []string, vmname, suffix string) []string { dependencyNames := make([]string, len(dependencies)) for i, dependency := range dependencies { - dependencyNames[i] = dependencyNameFromVMNameAndDependancy(dependency, vmname, suffix) + dependencyNames[i] = dependencyNameFromVMNameAndDependency(dependency, vmname, suffix) } return dependencyNames } -func dependencyNameFromVMNameAndDependancy(dependency, vmName, suffix string) string { +func dependencyNameFromVMNameAndDependency(dependency, vmName, suffix string) string { return vmName + "-" + dependency + suffix } diff --git a/pkg/driver/driver_azure_test.go b/pkg/driver/driver_azure_test.go index 1f1895fd6..9c847f465 100644 --- a/pkg/driver/driver_azure_test.go +++ b/pkg/driver/driver_azure_test.go @@ -34,8 +34,8 @@ var _ = Describe("Driver Azure", func() { lun2 := int32(2) size1 := int32(10) size2 := int32(100) - expectedName1 := "vm-sdb-data-disk" - expectedName2 := "vm-sdc-data-disk" + expectedName1 := "vm-sdb-1-data-disk" + expectedName2 := "vm-sdc-2-data-disk" disks := []v1alpha1.AzureDataDisk{ { Name: "sdb", From 2e8ecc693bea7d4eb35b926eae35706fee64c9bb Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Mon, 17 Feb 2020 14:20:23 +0200 Subject: [PATCH 4/9] fix review comments --- .../machine_classes/azure-machine-class.yaml | 10 ++-- pkg/apis/machine/types.go | 13 +++-- pkg/apis/machine/v1alpha1/types.go | 13 +++-- .../v1alpha1/zz_generated.conversion.go | 14 ++---- .../machine/v1alpha1/zz_generated.deepcopy.go | 10 +++- .../validation/alicloudmachineclass.go | 4 +- .../validation/alicloudmachineclass_test.go | 21 ++++---- .../machine/validation/awsmachineclass.go | 2 +- .../validation/awsmachineclass_test.go | 38 +++++++++++++++ .../machine/validation/azuremachineclass.go | 4 +- .../validation/azuremachineclass_test.go | 48 +++++++------------ pkg/apis/machine/zz_generated.deepcopy.go | 10 +++- pkg/driver/driver_alicloud.go | 17 ++++--- pkg/driver/driver_alicloud_test.go | 10 ++-- pkg/driver/driver_azure.go | 6 +-- pkg/driver/driver_azure_test.go | 28 +++++------ pkg/openapi/openapi_generated.go | 13 ++--- 17 files changed, 143 insertions(+), 118 deletions(-) diff --git a/kubernetes/machine_classes/azure-machine-class.yaml b/kubernetes/machine_classes/azure-machine-class.yaml index 6784c6e39..163ec48f8 100644 --- a/kubernetes/machine_classes/azure-machine-class.yaml +++ b/kubernetes/machine_classes/azure-machine-class.yaml @@ -29,20 +29,16 @@ spec: caching: "None" # Caching Strategy (None/ReadOnly/ReadWrite) diskSizeGB: 50 # Size of disk to be created in GB createOption: "FromImage" # Create option for disk (Empty/Attach/FromImage) - DataDisks: + dataDisks: - lun: 0 caching: None - createOption: Empty diskSizeGB: 100 - managedDisk: - storageAccountType: Standard_LRS + storageAccountType: Standard_LRS name: sdb - lun: 1 caching: None - createOption: Empty diskSizeGB: 100 - managedDisk: - storageAccountType: Standard_LRS + storageAccountType: Standard_LRS name: sdc osProfile: adminUsername: "admin-name" # Admin user name diff --git a/pkg/apis/machine/types.go b/pkg/apis/machine/types.go index 49ceea728..c1c2ca399 100644 --- a/pkg/apis/machine/types.go +++ b/pkg/apis/machine/types.go @@ -912,12 +912,11 @@ type AzureOSDisk struct { } type AzureDataDisk struct { - Name string - Lun int32 - Caching string - ManagedDisk AzureManagedDiskParameters - DiskSizeGB int32 - CreateOption string + Name string + Lun int32 + Caching string + StorageAccountType string + DiskSizeGB int32 } // AzureManagedDiskParameters is the parameters of a managed disk. @@ -1124,7 +1123,7 @@ type AlicloudDataDisk struct { Description string Encrypted bool Size int - DeleteWithInstance bool + DeleteWithInstance *bool } /********************** PacketMachineClass APIs ***************/ diff --git a/pkg/apis/machine/v1alpha1/types.go b/pkg/apis/machine/v1alpha1/types.go index b94144cf4..faf45abc3 100644 --- a/pkg/apis/machine/v1alpha1/types.go +++ b/pkg/apis/machine/v1alpha1/types.go @@ -994,12 +994,11 @@ type AzureOSDisk struct { } type AzureDataDisk struct { - Name string `json:"name,omitempty"` - Lun int32 `json:"lun,omitempty"` - Caching string `json:"caching,omitempty"` - ManagedDisk AzureManagedDiskParameters `json:"managedDisk,omitempty"` - DiskSizeGB int32 `json:"diskSizeGB,omitempty"` - CreateOption string `json:"createOption,omitempty"` + Name string `json:"name,omitempty"` + Lun int32 `json:"lun,omitempty"` + Caching string `json:"caching,omitempty"` + StorageAccountType string `json:"storageAccountType,omitempty"` + DiskSizeGB int32 `json:"diskSizeGB,omitempty"` } // AzureManagedDiskParameters is the parameters of a managed disk. @@ -1265,7 +1264,7 @@ type AlicloudDataDisk struct { // +optional Description string `json:"description"` Encrypted bool `json:"encrypted"` - DeleteWithInstance bool `json:"deleteWithInstance"` + DeleteWithInstance *bool `json:"deleteWithInstance"` Size int `json:"size"` } diff --git a/pkg/apis/machine/v1alpha1/zz_generated.conversion.go b/pkg/apis/machine/v1alpha1/zz_generated.conversion.go index 8b22c98b6..67a15ae01 100644 --- a/pkg/apis/machine/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/machine/v1alpha1/zz_generated.conversion.go @@ -1016,7 +1016,7 @@ func autoConvert_v1alpha1_AlicloudDataDisk_To_machine_AlicloudDataDisk(in *Alicl out.Category = in.Category out.Description = in.Description out.Encrypted = in.Encrypted - out.DeleteWithInstance = in.DeleteWithInstance + out.DeleteWithInstance = (*bool)(unsafe.Pointer(in.DeleteWithInstance)) out.Size = in.Size return nil } @@ -1032,7 +1032,7 @@ func autoConvert_machine_AlicloudDataDisk_To_v1alpha1_AlicloudDataDisk(in *machi out.Description = in.Description out.Encrypted = in.Encrypted out.Size = in.Size - out.DeleteWithInstance = in.DeleteWithInstance + out.DeleteWithInstance = (*bool)(unsafe.Pointer(in.DeleteWithInstance)) return nil } @@ -1209,11 +1209,8 @@ func autoConvert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk(in *AzureDataDi out.Name = in.Name out.Lun = in.Lun out.Caching = in.Caching - if err := Convert_v1alpha1_AzureManagedDiskParameters_To_machine_AzureManagedDiskParameters(&in.ManagedDisk, &out.ManagedDisk, s); err != nil { - return err - } + out.StorageAccountType = in.StorageAccountType out.DiskSizeGB = in.DiskSizeGB - out.CreateOption = in.CreateOption return nil } @@ -1226,11 +1223,8 @@ func autoConvert_machine_AzureDataDisk_To_v1alpha1_AzureDataDisk(in *machine.Azu out.Name = in.Name out.Lun = in.Lun out.Caching = in.Caching - if err := Convert_machine_AzureManagedDiskParameters_To_v1alpha1_AzureManagedDiskParameters(&in.ManagedDisk, &out.ManagedDisk, s); err != nil { - return err - } + out.StorageAccountType = in.StorageAccountType out.DiskSizeGB = in.DiskSizeGB - out.CreateOption = in.CreateOption return nil } diff --git a/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go index 606d90066..d4ab95650 100644 --- a/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go @@ -201,6 +201,11 @@ func (in *AWSNetworkInterfaceSpec) DeepCopy() *AWSNetworkInterfaceSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AlicloudDataDisk) DeepCopyInto(out *AlicloudDataDisk) { *out = *in + if in.DeleteWithInstance != nil { + in, out := &in.DeleteWithInstance, &out.DeleteWithInstance + *out = new(bool) + **out = **in + } return } @@ -285,7 +290,9 @@ func (in *AlicloudMachineClassSpec) DeepCopyInto(out *AlicloudMachineClassSpec) if in.DataDisks != nil { in, out := &in.DataDisks, &out.DataDisks *out = make([]AlicloudDataDisk, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.InternetMaxBandwidthIn != nil { in, out := &in.InternetMaxBandwidthIn, &out.InternetMaxBandwidthIn @@ -341,7 +348,6 @@ func (in *AlicloudSystemDisk) DeepCopy() *AlicloudSystemDisk { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AzureDataDisk) DeepCopyInto(out *AzureDataDisk) { *out = *in - out.ManagedDisk = in.ManagedDisk return } diff --git a/pkg/apis/machine/validation/alicloudmachineclass.go b/pkg/apis/machine/validation/alicloudmachineclass.go index 2bef4ab8d..a7de2c51d 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass.go +++ b/pkg/apis/machine/validation/alicloudmachineclass.go @@ -77,8 +77,8 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl if dataDisk.Name == "" { allErrs = append(allErrs, field.Required(idxPath.Child("name"), "Data Disk name is required")) } - if dataDisk.Size <= 0 { - allErrs = append(allErrs, field.Required(idxPath.Child("size"), "Data Disk size must be positive")) + if dataDisk.Size < 20 || dataDisk.Size > 32768 { + allErrs = append(allErrs, field.Required(idxPath.Child("size"), "Data Disk size must be between 20 and 32768 GB")) } if dataDisk.Category == "" { allErrs = append(allErrs, field.Required(idxPath.Child("category"), "Data Disk category is required")) diff --git a/pkg/apis/machine/validation/alicloudmachineclass_test.go b/pkg/apis/machine/validation/alicloudmachineclass_test.go index 724419ea8..99f6f8fff 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass_test.go +++ b/pkg/apis/machine/validation/alicloudmachineclass_test.go @@ -10,6 +10,8 @@ import ( func getAliCloudMachineSpec() *machine.AlicloudMachineClassSpec { bandwidth := 5 + deleteVol1 := true + deleteVol2 := false return &machine.AlicloudMachineClassSpec{ ImageID: "coreos_1745_7_0_64_30G_alibase_20180705.vhd", InstanceType: "ecs.n1.medium", @@ -27,14 +29,14 @@ func getAliCloudMachineSpec() *machine.AlicloudMachineClassSpec { Category: "cloud_efficiency", Size: 75, Encrypted: true, - DeleteWithInstance: true, + DeleteWithInstance: &deleteVol1, }, { Name: "dd2", Category: "cloud_efficiency", Size: 75, Encrypted: true, - DeleteWithInstance: true, + DeleteWithInstance: &deleteVol2, }, }, InstanceChargeType: "PostPaid", @@ -72,23 +74,20 @@ var _ = Describe("AliCloudMachineClass Validation", func() { { Name: "", Category: "", - Size: -1, + Size: 10, Encrypted: true, - DeleteWithInstance: true, }, { Name: "dd1", Category: "cloud_efficiency", - Size: 75, + Size: 32769, Encrypted: true, - DeleteWithInstance: true, }, { Name: "dd1", Category: "cloud_efficiency", Size: 75, Encrypted: true, - DeleteWithInstance: true, }, } @@ -105,7 +104,7 @@ var _ = Describe("AliCloudMachineClass Validation", func() { Type: field.ErrorTypeRequired, Field: "spec.dataDisks[0].size", BadValue: "", - Detail: "Data Disk size must be positive", + Detail: "Data Disk size must be between 20 and 32768 GB", }, { Type: field.ErrorTypeRequired, @@ -113,6 +112,12 @@ var _ = Describe("AliCloudMachineClass Validation", func() { BadValue: "", Detail: "Data Disk category is required", }, + { + Type: field.ErrorTypeRequired, + Field: "spec.dataDisks[1].size", + BadValue: "", + Detail: "Data Disk size must be between 20 and 32768 GB", + }, { Type: field.ErrorTypeInvalid, Field: "spec.dataDisks", diff --git a/pkg/apis/machine/validation/awsmachineclass.go b/pkg/apis/machine/validation/awsmachineclass.go index 1f1fd8f99..8fee9385d 100644 --- a/pkg/apis/machine/validation/awsmachineclass.go +++ b/pkg/apis/machine/validation/awsmachineclass.go @@ -144,7 +144,7 @@ func validateBlockDevices(blockDevices []machine.AWSBlockDeviceMappingSpec, fldP rootPartitionCount++ } else if len(blockDevices) > 1 && !dataDeviceNameRegexp.MatchString(disk.DeviceName) { // if there are multiple devices, non-root devices are expected to adhere to AWS naming conventions - allErrs = append(allErrs, field.Invalid(idxPath.Child("deviceName"), disk.DeviceName, utilvalidation.RegexError(dataDeviceNameFmt, "/dev/sdf"))) + allErrs = append(allErrs, field.Invalid(idxPath.Child("deviceName"), disk.DeviceName, utilvalidation.RegexError(fmt.Sprintf("Device name given: %s does not match the expected pattern", disk.DeviceName), dataDeviceNameFmt))) } if _, keyExist := deviceNames[disk.DeviceName]; keyExist { diff --git a/pkg/apis/machine/validation/awsmachineclass_test.go b/pkg/apis/machine/validation/awsmachineclass_test.go index 33321bdbc..792a4eaab 100644 --- a/pkg/apis/machine/validation/awsmachineclass_test.go +++ b/pkg/apis/machine/validation/awsmachineclass_test.go @@ -318,6 +318,44 @@ var _ = Describe("AWSMachineClass Validation", func() { Expect(err).To(Equal(errExpected)) }) + It("should get an error on BlockDevices validation - invalid device name", func() { + spec := getSpec() + spec.BlockDevices = []machine.AWSBlockDeviceMappingSpec{ + { + DeviceName: "/dev/vdga", + Ebs: machine.AWSEbsBlockDeviceSpec{ + VolumeSize: 10, + VolumeType: "gp2", + }, + }, + { + DeviceName: "/dev/sdf", + Ebs: machine.AWSEbsBlockDeviceSpec{ + VolumeSize: 15, + VolumeType: "gp2", + }, + }, + } + + err := validateAWSMachineClassSpec(spec, field.NewPath("spec")) + + errExpected := field.ErrorList{ + { + Type: "FieldValueInvalid", + Field: "spec.blockDevices[0].deviceName", + BadValue: "/dev/vdga", + Detail: "Device name given: /dev/vdga does not match the expected pattern (regex used for validation is '/dev/(sd[a-z]|xvd[a-c][a-z]?)')", + }, + { + Type: "FieldValueRequired", + Field: "spec.blockDevices", + BadValue: "", + Detail: "Device name with '/root' partition must be specified", + }, + } + Expect(err).To(Equal(errExpected)) + }) + It("should get an error on BlockDevices validation - invalid volumeType", func() { spec := getSpec() spec.BlockDevices = []machine.AWSBlockDeviceMappingSpec{ diff --git a/pkg/apis/machine/validation/azuremachineclass.go b/pkg/apis/machine/validation/azuremachineclass.go index 6cd299155..dbda3218d 100644 --- a/pkg/apis/machine/validation/azuremachineclass.go +++ b/pkg/apis/machine/validation/azuremachineclass.go @@ -131,8 +131,8 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f if dataDisk.DiskSizeGB <= 0 { allErrs = append(allErrs, field.Required(idxPath.Child("diskSizeGB"), "DataDisk size must be positive")) } - if dataDisk.CreateOption == "" { - allErrs = append(allErrs, field.Required(idxPath.Child("createOption"), "DataDisk create option is required")) + if dataDisk.StorageAccountType == "" { + allErrs = append(allErrs, field.Required(idxPath.Child("storageAccountType"), "DataDisk storage account type is required")) } if dataDisk.Lun < 0 { allErrs = append(allErrs, field.Required(idxPath.Child("lun"), "DataDisk Lun is required")) diff --git a/pkg/apis/machine/validation/azuremachineclass_test.go b/pkg/apis/machine/validation/azuremachineclass_test.go index 2c05ac394..8483d623c 100644 --- a/pkg/apis/machine/validation/azuremachineclass_test.go +++ b/pkg/apis/machine/validation/azuremachineclass_test.go @@ -43,22 +43,16 @@ func getAzureMachineSpec() *machine.AzureMachineClassSpec { }, DataDisks: []machine.AzureDataDisk{ { - Lun: 0, - Caching: "None", - CreateOption: "Empty", - DiskSizeGB: 75, - ManagedDisk: machine.AzureManagedDiskParameters{ - StorageAccountType: "Standard_LRS", - }, + Lun: 0, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", }, { - Lun: 1, - Caching: "None", - CreateOption: "Empty", - DiskSizeGB: 75, - ManagedDisk: machine.AzureManagedDiskParameters{ - StorageAccountType: "Standard_LRS", - }, + Lun: 1, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", }, }, }, @@ -116,9 +110,9 @@ var _ = Describe("AzureMachineClass Validation", func() { }, { Type: field.ErrorTypeRequired, - Field: "spec.properties.storageProfile.dataDisks[0].createOption", + Field: "spec.properties.storageProfile.dataDisks[0].storageAccountType", BadValue: "", - Detail: "DataDisk create option is required", + Detail: "DataDisk storage account type is required", }, } Expect(errList).To(ConsistOf(errExpected)) @@ -129,22 +123,16 @@ var _ = Describe("AzureMachineClass Validation", func() { spec := getAzureMachineSpec() spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ { - Lun: 1, - Caching: "None", - CreateOption: "Empty", - DiskSizeGB: 75, - ManagedDisk: machine.AzureManagedDiskParameters{ - StorageAccountType: "Standard_LRS", - }, + Lun: 1, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", }, { - Lun: 1, - Caching: "None", - CreateOption: "Empty", - DiskSizeGB: 75, - ManagedDisk: machine.AzureManagedDiskParameters{ - StorageAccountType: "Standard_LRS", - }, + Lun: 1, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", }, } diff --git a/pkg/apis/machine/zz_generated.deepcopy.go b/pkg/apis/machine/zz_generated.deepcopy.go index 5c86adcb0..4eab8105c 100644 --- a/pkg/apis/machine/zz_generated.deepcopy.go +++ b/pkg/apis/machine/zz_generated.deepcopy.go @@ -201,6 +201,11 @@ func (in *AWSNetworkInterfaceSpec) DeepCopy() *AWSNetworkInterfaceSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AlicloudDataDisk) DeepCopyInto(out *AlicloudDataDisk) { *out = *in + if in.DeleteWithInstance != nil { + in, out := &in.DeleteWithInstance, &out.DeleteWithInstance + *out = new(bool) + **out = **in + } return } @@ -285,7 +290,9 @@ func (in *AlicloudMachineClassSpec) DeepCopyInto(out *AlicloudMachineClassSpec) if in.DataDisks != nil { in, out := &in.DataDisks, &out.DataDisks *out = make([]AlicloudDataDisk, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.InternetMaxBandwidthIn != nil { in, out := &in.InternetMaxBandwidthIn, &out.InternetMaxBandwidthIn @@ -341,7 +348,6 @@ func (in *AlicloudSystemDisk) DeepCopy() *AlicloudSystemDisk { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AzureDataDisk) DeepCopyInto(out *AzureDataDisk) { *out = *in - out.ManagedDisk = in.ManagedDisk return } diff --git a/pkg/driver/driver_alicloud.go b/pkg/driver/driver_alicloud.go index faffab68a..4aadcc765 100644 --- a/pkg/driver/driver_alicloud.go +++ b/pkg/driver/driver_alicloud.go @@ -117,12 +117,17 @@ func (c *AlicloudDriver) generateDataDiskRequests(disks []v1alpha1.AlicloudDataD for _, disk := range disks { dataDiskRequest := ecs.RunInstancesDataDisk{ - Category: disk.Category, - DeleteWithInstance: strconv.FormatBool(disk.DeleteWithInstance), - Encrypted: strconv.FormatBool(disk.Encrypted), - DiskName: fmt.Sprintf("%s-%s-data-disk", c.MachineName, disk.Name), - Description: disk.Description, - Size: fmt.Sprintf("%d", disk.Size), + Category: disk.Category, + Encrypted: strconv.FormatBool(disk.Encrypted), + DiskName: fmt.Sprintf("%s-%s-data-disk", c.MachineName, disk.Name), + Description: disk.Description, + Size: fmt.Sprintf("%d", disk.Size), + } + + if disk.DeleteWithInstance != nil { + dataDiskRequest.DeleteWithInstance = strconv.FormatBool(*disk.DeleteWithInstance) + } else { + dataDiskRequest.DeleteWithInstance = strconv.FormatBool(true) } if disk.Category == "DiskEphemeralSSD" { diff --git a/pkg/driver/driver_alicloud_test.go b/pkg/driver/driver_alicloud_test.go index b181f9f82..8e4219247 100644 --- a/pkg/driver/driver_alicloud_test.go +++ b/pkg/driver/driver_alicloud_test.go @@ -130,12 +130,14 @@ var _ = Describe("Driver AliCloud", func() { It("should generate multiple data disk requests", func() { c := &AlicloudDriver{} c.MachineName = "machinename" + vol1delete := true + vol2delete := false dataDisks := []v1alpha1.AlicloudDataDisk{ { Name: "dd1", Category: "cloud_efficiency", Description: "this is a disk", - DeleteWithInstance: true, + DeleteWithInstance: &vol1delete, Encrypted: true, Size: 100, }, @@ -143,7 +145,7 @@ var _ = Describe("Driver AliCloud", func() { Name: "dd2", Category: "cloud_ssd", Description: "this is also a disk", - DeleteWithInstance: false, + DeleteWithInstance: &vol2delete, Encrypted: false, Size: 50, }, @@ -172,7 +174,7 @@ var _ = Describe("Driver AliCloud", func() { Expect(generatedDataDisksRequests).To(Equal(expectedDataDiskRequests)) }) - It("should not encrypt or delete with instance by default", func() { + It("should not encrypt and delete with instance by default", func() { c := &AlicloudDriver{} c.MachineName = "machinename" dataDisks := []v1alpha1.AlicloudDataDisk{ @@ -192,7 +194,7 @@ var _ = Describe("Driver AliCloud", func() { Encrypted: strconv.FormatBool(false), DiskName: "machinename-dd1-data-disk", Description: "this is a disk", - DeleteWithInstance: strconv.FormatBool(false), + DeleteWithInstance: strconv.FormatBool(true), }, } diff --git a/pkg/driver/driver_azure.go b/pkg/driver/driver_azure.go index 3e2842510..02a6df4c2 100644 --- a/pkg/driver/driver_azure.go +++ b/pkg/driver/driver_azure.go @@ -171,7 +171,7 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID VMParameters.Identity = &compute.VirtualMachineIdentity{ Type: compute.ResourceIdentityTypeUserAssigned, UserAssignedIdentities: map[string]*compute.VirtualMachineIdentityUserAssignedIdentitiesValue{ - *d.AzureMachineClass.Spec.Properties.IdentityID: {}, + *d.AzureMachineClass.Spec.Properties.IdentityID: &compute.VirtualMachineIdentityUserAssignedIdentitiesValue{}, }, } } @@ -190,10 +190,10 @@ func (d *AzureDriver) generateDataDisks(vmName string, azureDataDisks []v1alpha1 Name: &dataDiskName, Caching: compute.CachingTypes(azureDataDisk.Caching), ManagedDisk: &compute.ManagedDiskParameters{ - StorageAccountType: compute.StorageAccountTypes(azureDataDisk.ManagedDisk.StorageAccountType), + StorageAccountType: compute.StorageAccountTypes(azureDataDisk.StorageAccountType), }, DiskSizeGB: &dataDiskSize, - CreateOption: compute.DiskCreateOptionTypes(azureDataDisk.CreateOption), + CreateOption: compute.DiskCreateOptionTypesEmpty, } dataDisks = append(dataDisks, dataDisk) } diff --git a/pkg/driver/driver_azure_test.go b/pkg/driver/driver_azure_test.go index 9c847f465..ddfb3bc0e 100644 --- a/pkg/driver/driver_azure_test.go +++ b/pkg/driver/driver_azure_test.go @@ -38,24 +38,18 @@ var _ = Describe("Driver Azure", func() { expectedName2 := "vm-sdc-2-data-disk" disks := []v1alpha1.AzureDataDisk{ { - Name: "sdb", - Caching: "None", - ManagedDisk: v1alpha1.AzureManagedDiskParameters{ - StorageAccountType: "Premium_LRS", - }, - DiskSizeGB: size1, - CreateOption: "Empty", - Lun: lun1, + Name: "sdb", + Caching: "None", + StorageAccountType: "Premium_LRS", + DiskSizeGB: size1, + Lun: lun1, }, { - Name: "sdc", - Caching: "None", - ManagedDisk: v1alpha1.AzureManagedDiskParameters{ - StorageAccountType: "Standard_LRS", - }, - DiskSizeGB: size2, - CreateOption: "FromImage", - Lun: lun2, + Name: "sdc", + Caching: "None", + StorageAccountType: "Standard_LRS", + DiskSizeGB: size2, + Lun: lun2, }, } @@ -79,7 +73,7 @@ var _ = Describe("Driver Azure", func() { StorageAccountType: compute.StorageAccountTypes("Standard_LRS"), }, DiskSizeGB: &size2, - CreateOption: compute.DiskCreateOptionTypes("FromImage"), + CreateOption: compute.DiskCreateOptionTypes("Empty"), }, } diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index 8e2b787b3..7292eacca 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -1042,9 +1042,10 @@ func schema_pkg_apis_machine_v1alpha1_AzureDataDisk(ref common.ReferenceCallback Format: "", }, }, - "managedDisk": { + "storageAccountType": { SchemaProps: spec.SchemaProps{ - Ref: ref("github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureManagedDiskParameters"), + Type: []string{"string"}, + Format: "", }, }, "diskSizeGB": { @@ -1053,17 +1054,9 @@ func schema_pkg_apis_machine_v1alpha1_AzureDataDisk(ref common.ReferenceCallback Format: "int32", }, }, - "createOption": { - SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", - }, - }, }, }, }, - Dependencies: []string{ - "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1.AzureManagedDiskParameters"}, } } From 74e5191843f83d8636ce6a7a46e5a9f699d574ac Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Sun, 29 Mar 2020 10:07:42 +0300 Subject: [PATCH 5/9] fix review comments --- pkg/apis/machine/types.go | 2 +- pkg/apis/machine/v1alpha1/types.go | 2 +- .../v1alpha1/zz_generated.conversion.go | 4 +- .../machine/v1alpha1/zz_generated.deepcopy.go | 9 ++- .../validation/alicloudmachineclass.go | 20 ++++-- .../validation/alicloudmachineclass_test.go | 12 ++++ .../machine/validation/azuremachineclass.go | 35 ++++++---- .../validation/azuremachineclass_test.go | 70 ++++++++++++++++--- pkg/apis/machine/zz_generated.deepcopy.go | 9 ++- pkg/driver/driver_aws_test.go | 11 ++- pkg/driver/driver_azure.go | 52 ++++++++++---- pkg/driver/driver_azure_test.go | 55 ++++++++++++++- pkg/driver/driver_test.go | 4 +- 13 files changed, 227 insertions(+), 58 deletions(-) diff --git a/pkg/apis/machine/types.go b/pkg/apis/machine/types.go index 3bc3898d6..36219155f 100644 --- a/pkg/apis/machine/types.go +++ b/pkg/apis/machine/types.go @@ -913,7 +913,7 @@ type AzureOSDisk struct { type AzureDataDisk struct { Name string - Lun int32 + Lun *int32 Caching string StorageAccountType string DiskSizeGB int32 diff --git a/pkg/apis/machine/v1alpha1/types.go b/pkg/apis/machine/v1alpha1/types.go index 5ffeab7c2..7d2bd36e5 100644 --- a/pkg/apis/machine/v1alpha1/types.go +++ b/pkg/apis/machine/v1alpha1/types.go @@ -995,7 +995,7 @@ type AzureOSDisk struct { type AzureDataDisk struct { Name string `json:"name,omitempty"` - Lun int32 `json:"lun,omitempty"` + Lun *int32 `json:"lun,omitempty"` Caching string `json:"caching,omitempty"` StorageAccountType string `json:"storageAccountType,omitempty"` DiskSizeGB int32 `json:"diskSizeGB,omitempty"` diff --git a/pkg/apis/machine/v1alpha1/zz_generated.conversion.go b/pkg/apis/machine/v1alpha1/zz_generated.conversion.go index 5e4b2a039..c939e9ca9 100644 --- a/pkg/apis/machine/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/machine/v1alpha1/zz_generated.conversion.go @@ -1207,7 +1207,7 @@ func Convert_machine_AlicloudSystemDisk_To_v1alpha1_AlicloudSystemDisk(in *machi func autoConvert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk(in *AzureDataDisk, out *machine.AzureDataDisk, s conversion.Scope) error { out.Name = in.Name - out.Lun = in.Lun + out.Lun = (*int32)(unsafe.Pointer(in.Lun)) out.Caching = in.Caching out.StorageAccountType = in.StorageAccountType out.DiskSizeGB = in.DiskSizeGB @@ -1221,7 +1221,7 @@ func Convert_v1alpha1_AzureDataDisk_To_machine_AzureDataDisk(in *AzureDataDisk, func autoConvert_machine_AzureDataDisk_To_v1alpha1_AzureDataDisk(in *machine.AzureDataDisk, out *AzureDataDisk, s conversion.Scope) error { out.Name = in.Name - out.Lun = in.Lun + out.Lun = (*int32)(unsafe.Pointer(in.Lun)) out.Caching = in.Caching out.StorageAccountType = in.StorageAccountType out.DiskSizeGB = in.DiskSizeGB diff --git a/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go index e8072cf73..d498e64d4 100644 --- a/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/machine/v1alpha1/zz_generated.deepcopy.go @@ -355,6 +355,11 @@ func (in *AlicloudSystemDisk) DeepCopy() *AlicloudSystemDisk { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AzureDataDisk) DeepCopyInto(out *AzureDataDisk) { *out = *in + if in.Lun != nil { + in, out := &in.Lun, &out.Lun + *out = new(int32) + **out = **in + } return } @@ -662,7 +667,9 @@ func (in *AzureStorageProfile) DeepCopyInto(out *AzureStorageProfile) { if in.DataDisks != nil { in, out := &in.DataDisks, &out.DataDisks *out = make([]AzureDataDisk, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } return } diff --git a/pkg/apis/machine/validation/alicloudmachineclass.go b/pkg/apis/machine/validation/alicloudmachineclass.go index a7de2c51d..ebb0318c8 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass.go +++ b/pkg/apis/machine/validation/alicloudmachineclass.go @@ -19,6 +19,8 @@ package validation import ( "fmt" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" + "regexp" "strings" "github.com/gardener/machine-controller-manager/pkg/apis/machine" @@ -64,18 +66,25 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl allErrs = append(allErrs, field.Required(fldPath.Child("keyPairName"), "KeyPairName is required")) } + const dataDiskNameFmt string = `[a-zA-Z0-9\.\-_:]+` + var dataDiskNameRegexp = regexp.MustCompile("^" + dataDiskNameFmt + "$") + + if spec.DataDisks != nil { names := map[string]int{} for i, dataDisk := range spec.DataDisks { idxPath := fldPath.Child("dataDisks").Index(i) - if _, keyExist := names[dataDisk.Name]; keyExist { - names[dataDisk.Name]++ - } else { - names[dataDisk.Name] = 1 - } if dataDisk.Name == "" { allErrs = append(allErrs, field.Required(idxPath.Child("name"), "Data Disk name is required")) + } else if !dataDiskNameRegexp.MatchString(dataDisk.Name) { + allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), dataDisk.Name, utilvalidation.RegexError(fmt.Sprintf("Disk name given: %s does not match the expected pattern", dataDisk.Name), dataDiskNameFmt))) + } else { + if _, keyExist := names[dataDisk.Name]; keyExist { + names[dataDisk.Name]++ + } else { + names[dataDisk.Name] = 1 + } } if dataDisk.Size < 20 || dataDisk.Size > 32768 { allErrs = append(allErrs, field.Required(idxPath.Child("size"), "Data Disk size must be between 20 and 32768 GB")) @@ -83,6 +92,7 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl if dataDisk.Category == "" { allErrs = append(allErrs, field.Required(idxPath.Child("category"), "Data Disk category is required")) } + } for name, number := range names { diff --git a/pkg/apis/machine/validation/alicloudmachineclass_test.go b/pkg/apis/machine/validation/alicloudmachineclass_test.go index 99f6f8fff..53ec9c797 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass_test.go +++ b/pkg/apis/machine/validation/alicloudmachineclass_test.go @@ -89,6 +89,12 @@ var _ = Describe("AliCloudMachineClass Validation", func() { Size: 75, Encrypted: true, }, + { + Name: "bad$#%name", + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, + }, } errList := validateAlicloudMachineClassSpec(spec, field.NewPath("spec")) @@ -124,6 +130,12 @@ var _ = Describe("AliCloudMachineClass Validation", func() { BadValue: "dd1", Detail: "Data Disk Name 'dd1' duplicated 2 times, Name must be unique", }, + { + Type: field.ErrorTypeInvalid, + Field: "spec.dataDisks[3].name", + BadValue: "bad$#%name", + Detail: "Disk name given: bad$#%name does not match the expected pattern (regex used for validation is '[a-zA-Z0-9\\.\\-_:]+')", + }, } Expect(errList).To(ConsistOf(errExpected)) }) diff --git a/pkg/apis/machine/validation/azuremachineclass.go b/pkg/apis/machine/validation/azuremachineclass.go index dbda3218d..8af48e9bd 100644 --- a/pkg/apis/machine/validation/azuremachineclass.go +++ b/pkg/apis/machine/validation/azuremachineclass.go @@ -19,6 +19,7 @@ package validation import ( "fmt" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "strings" "github.com/gardener/machine-controller-manager/pkg/apis/machine" @@ -105,9 +106,6 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f } } - if properties.StorageProfile.OsDisk.Caching == "" { - allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.osDisk.caching"), "OSDisk caching is required")) - } if properties.StorageProfile.OsDisk.DiskSizeGB <= 0 { allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.osDisk.diskSizeGB"), "OSDisk size must be positive")) } @@ -116,27 +114,36 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f } if properties.StorageProfile.DataDisks != nil { + + if len(properties.StorageProfile.DataDisks) > 16 { + allErrs = append(allErrs, field.TooMany(fldPath.Child("storageProfile.dataDisks"), len(properties.StorageProfile.DataDisks), 16)) + } + luns := map[int32]int{} + nilLuns := false for i, dataDisk := range properties.StorageProfile.DataDisks { idxPath := fldPath.Child("storageProfile.dataDisks").Index(i) - if _, keyExist := luns[dataDisk.Lun]; keyExist { - luns[dataDisk.Lun]++ + + if dataDisk.Lun != nil { + lun := dataDisk.Lun + if *lun < 0 || *lun > 64 { + allErrs = append(allErrs, field.Invalid(idxPath.Child("lun"), *lun, utilvalidation.InclusiveRangeError(0, 15))) + } + if _, keyExist := luns[*lun]; keyExist { + luns[*lun]++ + } else { + luns[*lun] = 1 + } } else { - luns[dataDisk.Lun] = 1 + nilLuns = true } - if dataDisk.Caching == "" { - allErrs = append(allErrs, field.Required(idxPath.Child("caching"), "DataDisk caching is required")) - } if dataDisk.DiskSizeGB <= 0 { allErrs = append(allErrs, field.Required(idxPath.Child("diskSizeGB"), "DataDisk size must be positive")) } if dataDisk.StorageAccountType == "" { allErrs = append(allErrs, field.Required(idxPath.Child("storageAccountType"), "DataDisk storage account type is required")) } - if dataDisk.Lun < 0 { - allErrs = append(allErrs, field.Required(idxPath.Child("lun"), "DataDisk Lun is required")) - } } for lun, number := range luns { @@ -144,6 +151,10 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f allErrs = append(allErrs, field.Invalid(fldPath.Child("storageProfile.dataDisks"), lun, fmt.Sprintf("Data Disk Lun '%d' duplicated %d times, Lun must be unique", lun, number))) } } + + if nilLuns == true && len(luns) > 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.dataDisks"), "All Data Disk Luns must be set")) + } } if properties.OsProfile.AdminUsername == "" { allErrs = append(allErrs, field.Required(fldPath.Child("osProfile.adminUsername"), "AdminUsername is required")) diff --git a/pkg/apis/machine/validation/azuremachineclass_test.go b/pkg/apis/machine/validation/azuremachineclass_test.go index 8483d623c..863217aeb 100644 --- a/pkg/apis/machine/validation/azuremachineclass_test.go +++ b/pkg/apis/machine/validation/azuremachineclass_test.go @@ -11,6 +11,8 @@ import ( func getAzureMachineSpec() *machine.AzureMachineClassSpec { urn := "CoreOS:CoreOS:Stable:2303.3.0" zone := 1 + lun0 := int32(0) + lun1 := int32(1) return &machine.AzureMachineClassSpec{ Location: "westeurope", Properties: machine.AzureVirtualMachineProperties{ @@ -43,13 +45,13 @@ func getAzureMachineSpec() *machine.AzureMachineClassSpec { }, DataDisks: []machine.AzureDataDisk{ { - Lun: 0, + Lun: &lun0, Caching: "None", DiskSizeGB: 75, StorageAccountType: "Standard_LRS", }, { - Lun: 1, + Lun: &lun1, Caching: "None", DiskSizeGB: 75, StorageAccountType: "Standard_LRS", @@ -96,12 +98,6 @@ var _ = Describe("AzureMachineClass Validation", func() { errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) errExpected := field.ErrorList{ - { - Type: field.ErrorTypeRequired, - Field: "spec.properties.storageProfile.dataDisks[0].caching", - BadValue: "", - Detail: "DataDisk caching is required", - }, { Type: field.ErrorTypeRequired, Field: "spec.properties.storageProfile.dataDisks[0].diskSizeGB", @@ -121,15 +117,16 @@ var _ = Describe("AzureMachineClass Validation", func() { It("should get an error on duplicated DataDisks lun ", func() { spec := getAzureMachineSpec() + lun1 := int32(1) spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ { - Lun: 1, + Lun: &lun1, Caching: "None", DiskSizeGB: 75, StorageAccountType: "Standard_LRS", }, { - Lun: 1, + Lun: &lun1, Caching: "None", DiskSizeGB: 75, StorageAccountType: "Standard_LRS", @@ -148,5 +145,58 @@ var _ = Describe("AzureMachineClass Validation", func() { } Expect(errList).To(ConsistOf(errExpected)) }) + + It("should get an error on partially unset luns ", func() { + spec := getAzureMachineSpec() + lun1 := int32(1) + spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ + { + Lun: &lun1, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", + }, + { + Lun: nil, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", + }, + } + + errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) + + errExpected := field.ErrorList{ + { + Type: field.ErrorTypeRequired, + Field: "spec.properties.storageProfile.dataDisks", + BadValue: "", + Detail: "All Data Disk Luns must be set", + }, + } + Expect(errList).To(ConsistOf(errExpected)) + }) + + It("should allow all luns to be nil ", func() { + spec := getAzureMachineSpec() + spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ + { + Lun: nil, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", + }, + { + Lun: nil, + Caching: "None", + DiskSizeGB: 75, + StorageAccountType: "Standard_LRS", + }, + } + + errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) + Expect(errList).To(BeEmpty()) + }) + }) }) diff --git a/pkg/apis/machine/zz_generated.deepcopy.go b/pkg/apis/machine/zz_generated.deepcopy.go index 113657ec1..b4a932fbf 100644 --- a/pkg/apis/machine/zz_generated.deepcopy.go +++ b/pkg/apis/machine/zz_generated.deepcopy.go @@ -355,6 +355,11 @@ func (in *AlicloudSystemDisk) DeepCopy() *AlicloudSystemDisk { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AzureDataDisk) DeepCopyInto(out *AzureDataDisk) { *out = *in + if in.Lun != nil { + in, out := &in.Lun, &out.Lun + *out = new(int32) + **out = **in + } return } @@ -662,7 +667,9 @@ func (in *AzureStorageProfile) DeepCopyInto(out *AzureStorageProfile) { if in.DataDisks != nil { in, out := &in.DataDisks, &out.DataDisks *out = make([]AzureDataDisk, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } return } diff --git a/pkg/driver/driver_aws_test.go b/pkg/driver/driver_aws_test.go index 0d7640266..6a0706af0 100644 --- a/pkg/driver/driver_aws_test.go +++ b/pkg/driver/driver_aws_test.go @@ -216,7 +216,7 @@ var _ = Describe("Driver AWS", func() { Expect(err).ToNot(HaveOccurred()) }) - It("should not encrypt or delete on termination blockDevices by default", func() { + It("should not encrypt blockDevices by default", func() { awsDriver := &AWSDriver{} disks := []v1alpha1.AWSBlockDeviceMappingSpec{ { @@ -233,11 +233,10 @@ var _ = Describe("Driver AWS", func() { { DeviceName: aws.String("/dev/sda"), Ebs: &ec2.EbsBlockDevice{ - DeleteOnTermination: aws.Bool(false), - Encrypted: aws.Bool(false), - VolumeSize: aws.Int64(32), - Iops: nil, - VolumeType: aws.String("gp2"), + Encrypted: aws.Bool(false), + VolumeSize: aws.Int64(32), + Iops: nil, + VolumeType: aws.String("gp2"), }, }, } diff --git a/pkg/driver/driver_azure.go b/pkg/driver/driver_azure.go index cdee1e988..372841604 100644 --- a/pkg/driver/driver_azure.go +++ b/pkg/driver/driver_azure.go @@ -183,14 +183,31 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID func (d *AzureDriver) generateDataDisks(vmName string, azureDataDisks []v1alpha1.AzureDataDisk) []compute.DataDisk { var dataDisks []compute.DataDisk - for _, azureDataDisk := range azureDataDisks { - dataDiskName := dependencyNameFromVMNameAndDependency(getAzureDataDiskName(azureDataDisk), vmName, dataDiskSuffix) - dataDiskLun := azureDataDisk.Lun + for i, azureDataDisk := range azureDataDisks { + + var dataDiskLun *int32 + if azureDataDisk.Lun != nil { + dataDiskLun = azureDataDisk.Lun + } else { + lun := int32(i) + dataDiskLun = &lun + } + + dataDiskName := dependencyNameFromVMNameAndDependency(getAzureDataDiskName(azureDataDisk.Name, dataDiskLun), vmName, dataDiskSuffix) + + var caching compute.CachingTypes + if azureDataDisk.Caching != "" { + caching = compute.CachingTypes(azureDataDisk.Caching) + } else { + caching = compute.CachingTypesNone + } + dataDiskSize := azureDataDisk.DiskSizeGB + dataDisk := compute.DataDisk{ - Lun: &dataDiskLun, + Lun: dataDiskLun, Name: &dataDiskName, - Caching: compute.CachingTypes(azureDataDisk.Caching), + Caching: caching, ManagedDisk: &compute.ManagedDiskParameters{ StorageAccountType: compute.StorageAccountTypes(azureDataDisk.StorageAccountType), }, @@ -247,7 +264,7 @@ func (d *AzureDriver) Delete(machineID string) error { dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) } - if err := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames) ; err != nil { + if err := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames); err != nil { return err } @@ -261,7 +278,8 @@ func (d *AzureDriver) Delete(machineID string) error { return clients.checkOrphanDisks(ctx, resourceGroupName, vmName) }, 3, time.Second*30) } - return runInParallel(orphanNicChecker, orphanDiskChecker) + orphanCheckers := []func() error{orphanNicChecker, orphanDiskChecker} + return runInParallel(orphanCheckers) } // GetExisting method is used to fetch the machineID for an azure machine @@ -760,8 +778,6 @@ func (clients *azureDriverClients) deleteVMNicDisks(ctx context.Context, resourc return onARMAPIErrorFail(prometheusServiceVM, vmErr, "vm.Get") } - var deleters []func() error - // Fetch the NIC and deleted it nicDeleter := func() error { if vmHoldingNic, err := clients.fetchAttachedVMfromNIC(ctx, resourceGroupName, nicName); err != nil { @@ -777,11 +793,10 @@ func (clients *azureDriverClients) deleteVMNicDisks(ctx context.Context, resourc return clients.deleteNIC(ctx, resourceGroupName, nicName) } - deleters = append(deleters, nicDeleter) - // Fetch the system disk and delete it diskDeleter := clients.getDeleterForDisk(ctx, resourceGroupName, diskName) - deleters = append(deleters, diskDeleter) + + deleters := []func() error{nicDeleter, diskDeleter} if dataDiskNames != nil { for _, dataDiskName := range dataDiskNames { @@ -1024,13 +1039,20 @@ const ( func getAzureDataDiskNames(azureDataDisks []v1alpha1.AzureDataDisk) []string { azureDataDiskNames := make([]string, len(azureDataDisks)) for i, disk := range azureDataDisks { - azureDataDiskNames[i] = getAzureDataDiskName(disk) + var diskLun *int32 + if disk.Lun != nil { + diskLun = disk.Lun + } else { + lun := int32(i) + diskLun = &lun + } + azureDataDiskNames[i] = getAzureDataDiskName(disk.Name, diskLun) } return azureDataDiskNames } -func getAzureDataDiskName(disk v1alpha1.AzureDataDisk) string { - return fmt.Sprintf("%s-%d", disk.Name, disk.Lun) +func getAzureDataDiskName(name string, lun *int32) string { + return fmt.Sprintf("%s-%d", name, *lun) } func dependencyNamesFromVMName(dependencies []string, vmname, suffix string) []string { diff --git a/pkg/driver/driver_azure_test.go b/pkg/driver/driver_azure_test.go index ddfb3bc0e..0d6a79b0a 100644 --- a/pkg/driver/driver_azure_test.go +++ b/pkg/driver/driver_azure_test.go @@ -17,7 +17,7 @@ limitations under the License. package driver import ( - "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute" + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2019-07-01/compute" "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -42,14 +42,63 @@ var _ = Describe("Driver Azure", func() { Caching: "None", StorageAccountType: "Premium_LRS", DiskSizeGB: size1, - Lun: lun1, + Lun: &lun1, }, { Name: "sdc", Caching: "None", StorageAccountType: "Standard_LRS", DiskSizeGB: size2, - Lun: lun2, + Lun: &lun2, + }, + } + + disksGenerated := azureDriver.generateDataDisks(vmName, disks) + expectedDisks := []compute.DataDisk{ + { + Lun: &lun1, + Name: &expectedName1, + Caching: compute.CachingTypes("None"), + ManagedDisk: &compute.ManagedDiskParameters{ + StorageAccountType: compute.StorageAccountTypes("Premium_LRS"), + }, + DiskSizeGB: &size1, + CreateOption: compute.DiskCreateOptionTypes("Empty"), + }, + { + Lun: &lun2, + Name: &expectedName2, + Caching: compute.CachingTypes("None"), + ManagedDisk: &compute.ManagedDiskParameters{ + StorageAccountType: compute.StorageAccountTypes("Standard_LRS"), + }, + DiskSizeGB: &size2, + CreateOption: compute.DiskCreateOptionTypes("Empty"), + }, + } + + Expect(disksGenerated).To(Equal(expectedDisks)) + }) + + It("should convert multiple dataDisks successfully with default caching and luns", func() { + azureDriver := &AzureDriver{} + vmName := "vm" + lun1 := int32(0) + lun2 := int32(1) + size1 := int32(10) + size2 := int32(100) + expectedName1 := "vm-sdb-0-data-disk" + expectedName2 := "vm-sdc-1-data-disk" + disks := []v1alpha1.AzureDataDisk{ + { + Name: "sdb", + StorageAccountType: "Premium_LRS", + DiskSizeGB: size1, + }, + { + Name: "sdc", + StorageAccountType: "Standard_LRS", + DiskSizeGB: size2, }, } diff --git a/pkg/driver/driver_test.go b/pkg/driver/driver_test.go index 3900f77b9..fa039e2f8 100644 --- a/pkg/driver/driver_test.go +++ b/pkg/driver/driver_test.go @@ -1,6 +1,8 @@ package driver -import "testing" +import ( + "testing" +) import . "github.com/onsi/ginkgo" import . "github.com/onsi/gomega" From 4c011c5ab156eed36331aecd2db76490451d0436 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Wed, 15 Apr 2020 17:01:07 +0300 Subject: [PATCH 6/9] fix code review comments --- pkg/apis/machine/v1alpha1/types.go | 12 ++-- .../validation/alicloudmachineclass.go | 5 +- .../validation/alicloudmachineclass_test.go | 64 +++++++++++-------- .../machine/validation/azuremachineclass.go | 22 +++---- .../validation/azuremachineclass_test.go | 60 ++--------------- pkg/driver/driver_azure.go | 25 +++----- pkg/driver/driver_azure_test.go | 19 +++++- 7 files changed, 91 insertions(+), 116 deletions(-) diff --git a/pkg/apis/machine/v1alpha1/types.go b/pkg/apis/machine/v1alpha1/types.go index e0bf4b715..cb304cb24 100644 --- a/pkg/apis/machine/v1alpha1/types.go +++ b/pkg/apis/machine/v1alpha1/types.go @@ -1259,13 +1259,13 @@ type AlicloudMachineClassSpec struct { } type AlicloudDataDisk struct { - Name string `json:"name"` - Category string `json:"category"` + Name string `json:"name,omitEmpty"` + Category string `json:"category,omitEmpty"` // +optional - Description string `json:"description"` - Encrypted bool `json:"encrypted"` - DeleteWithInstance *bool `json:"deleteWithInstance"` - Size int `json:"size"` + Description string `json:"description,omitEmpty"` + Encrypted bool `json:"encrypted,omitEmpty"` + DeleteWithInstance *bool `json:"deleteWithInstance,omitEmpty"` + Size int `json:"size,omitEmpty"` } // AlicloudSystemDisk describes SystemDisk for Alicloud. diff --git a/pkg/apis/machine/validation/alicloudmachineclass.go b/pkg/apis/machine/validation/alicloudmachineclass.go index ebb0318c8..aeb1617f6 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass.go +++ b/pkg/apis/machine/validation/alicloudmachineclass.go @@ -79,6 +79,8 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl allErrs = append(allErrs, field.Required(idxPath.Child("name"), "Data Disk name is required")) } else if !dataDiskNameRegexp.MatchString(dataDisk.Name) { allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), dataDisk.Name, utilvalidation.RegexError(fmt.Sprintf("Disk name given: %s does not match the expected pattern", dataDisk.Name), dataDiskNameFmt))) + } else if len(dataDisk.Name) > 64 { + allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), dataDisk.Name, "Data Disk Name length must be between 1 and 64")) } else { if _, keyExist := names[dataDisk.Name]; keyExist { names[dataDisk.Name]++ @@ -87,12 +89,11 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl } } if dataDisk.Size < 20 || dataDisk.Size > 32768 { - allErrs = append(allErrs, field.Required(idxPath.Child("size"), "Data Disk size must be between 20 and 32768 GB")) + allErrs = append(allErrs, field.Invalid(idxPath.Child("size"), dataDisk.Size, utilvalidation.InclusiveRangeError(20, 32768))) } if dataDisk.Category == "" { allErrs = append(allErrs, field.Required(idxPath.Child("category"), "Data Disk category is required")) } - } for name, number := range names { diff --git a/pkg/apis/machine/validation/alicloudmachineclass_test.go b/pkg/apis/machine/validation/alicloudmachineclass_test.go index 53ec9c797..8b75ac761 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass_test.go +++ b/pkg/apis/machine/validation/alicloudmachineclass_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation/field" + "strings" ) func getAliCloudMachineSpec() *machine.AlicloudMachineClassSpec { @@ -70,30 +71,37 @@ var _ = Describe("AliCloudMachineClass Validation", func() { It("should get an error on DataDisks validation", func() { spec := getAliCloudMachineSpec() + longname := strings.Repeat("longname", 10) spec.DataDisks = []machine.AlicloudDataDisk{ { - Name: "", - Category: "", - Size: 10, - Encrypted: true, + Name: "", + Category: "", + Size: 10, + Encrypted: true, }, { - Name: "dd1", - Category: "cloud_efficiency", - Size: 32769, - Encrypted: true, + Name: "dd1", + Category: "cloud_efficiency", + Size: 32769, + Encrypted: true, }, { - Name: "dd1", - Category: "cloud_efficiency", - Size: 75, - Encrypted: true, + Name: "dd1", + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, }, { - Name: "bad$#%name", - Category: "cloud_efficiency", - Size: 75, - Encrypted: true, + Name: "bad$#%name", + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, + }, + { + Name: longname, + Category: "cloud_efficiency", + Size: 75, + Encrypted: true, }, } @@ -107,10 +115,10 @@ var _ = Describe("AliCloudMachineClass Validation", func() { Detail: "Data Disk name is required", }, { - Type: field.ErrorTypeRequired, + Type: field.ErrorTypeInvalid, Field: "spec.dataDisks[0].size", - BadValue: "", - Detail: "Data Disk size must be between 20 and 32768 GB", + BadValue: 10, + Detail: "must be between 20 and 32768, inclusive", }, { Type: field.ErrorTypeRequired, @@ -119,10 +127,10 @@ var _ = Describe("AliCloudMachineClass Validation", func() { Detail: "Data Disk category is required", }, { - Type: field.ErrorTypeRequired, + Type: field.ErrorTypeInvalid, Field: "spec.dataDisks[1].size", - BadValue: "", - Detail: "Data Disk size must be between 20 and 32768 GB", + BadValue: 32769, + Detail: "must be between 20 and 32768, inclusive", }, { Type: field.ErrorTypeInvalid, @@ -131,10 +139,16 @@ var _ = Describe("AliCloudMachineClass Validation", func() { Detail: "Data Disk Name 'dd1' duplicated 2 times, Name must be unique", }, { - Type: field.ErrorTypeInvalid, - Field: "spec.dataDisks[3].name", + Type: field.ErrorTypeInvalid, + Field: "spec.dataDisks[3].name", BadValue: "bad$#%name", - Detail: "Disk name given: bad$#%name does not match the expected pattern (regex used for validation is '[a-zA-Z0-9\\.\\-_:]+')", + Detail: "Disk name given: bad$#%name does not match the expected pattern (regex used for validation is '[a-zA-Z0-9\\.\\-_:]+')", + }, + { + Type: field.ErrorTypeInvalid, + Field: "spec.dataDisks[4].name", + BadValue: longname, + Detail: "Data Disk Name length must be between 1 and 64", }, } Expect(errList).To(ConsistOf(errExpected)) diff --git a/pkg/apis/machine/validation/azuremachineclass.go b/pkg/apis/machine/validation/azuremachineclass.go index 0a9ad5edf..140bb0483 100644 --- a/pkg/apis/machine/validation/azuremachineclass.go +++ b/pkg/apis/machine/validation/azuremachineclass.go @@ -117,27 +117,27 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f if properties.StorageProfile.DataDisks != nil { - if len(properties.StorageProfile.DataDisks) > 16 { - allErrs = append(allErrs, field.TooMany(fldPath.Child("storageProfile.dataDisks"), len(properties.StorageProfile.DataDisks), 16)) + if len(properties.StorageProfile.DataDisks) > 64 { + allErrs = append(allErrs, field.TooMany(fldPath.Child("storageProfile.dataDisks"), len(properties.StorageProfile.DataDisks), 64)) } luns := map[int32]int{} - nilLuns := false for i, dataDisk := range properties.StorageProfile.DataDisks { idxPath := fldPath.Child("storageProfile.dataDisks").Index(i) - if dataDisk.Lun != nil { - lun := dataDisk.Lun - if *lun < 0 || *lun > 64 { - allErrs = append(allErrs, field.Invalid(idxPath.Child("lun"), *lun, utilvalidation.InclusiveRangeError(0, 15))) + lun := dataDisk.Lun + + if lun == nil { + allErrs = append(allErrs, field.Required(idxPath.Child("lun"), "DataDisk Lun is required")) + } else { + if *lun < 0 || *lun > 63 { + allErrs = append(allErrs, field.Invalid(idxPath.Child("lun"), *lun, utilvalidation.InclusiveRangeError(0, 63))) } if _, keyExist := luns[*lun]; keyExist { luns[*lun]++ } else { luns[*lun] = 1 } - } else { - nilLuns = true } if dataDisk.DiskSizeGB <= 0 { @@ -153,10 +153,6 @@ func validateAzureProperties(properties machine.AzureVirtualMachineProperties, f allErrs = append(allErrs, field.Invalid(fldPath.Child("storageProfile.dataDisks"), lun, fmt.Sprintf("Data Disk Lun '%d' duplicated %d times, Lun must be unique", lun, number))) } } - - if nilLuns == true && len(luns) > 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("storageProfile.dataDisks"), "All Data Disk Luns must be set")) - } } if properties.OsProfile.AdminUsername == "" { allErrs = append(allErrs, field.Required(fldPath.Child("osProfile.adminUsername"), "AdminUsername is required")) diff --git a/pkg/apis/machine/validation/azuremachineclass_test.go b/pkg/apis/machine/validation/azuremachineclass_test.go index 863217aeb..df59e6bb2 100644 --- a/pkg/apis/machine/validation/azuremachineclass_test.go +++ b/pkg/apis/machine/validation/azuremachineclass_test.go @@ -98,6 +98,12 @@ var _ = Describe("AzureMachineClass Validation", func() { errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) errExpected := field.ErrorList{ + { + Type: field.ErrorTypeRequired, + Field: "spec.properties.storageProfile.dataDisks[0].lun", + BadValue: "", + Detail: "DataDisk Lun is required", + }, { Type: field.ErrorTypeRequired, Field: "spec.properties.storageProfile.dataDisks[0].diskSizeGB", @@ -115,7 +121,7 @@ var _ = Describe("AzureMachineClass Validation", func() { }) - It("should get an error on duplicated DataDisks lun ", func() { + It("should get an error on duplicated DataDisks lun", func() { spec := getAzureMachineSpec() lun1 := int32(1) spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ @@ -146,57 +152,5 @@ var _ = Describe("AzureMachineClass Validation", func() { Expect(errList).To(ConsistOf(errExpected)) }) - It("should get an error on partially unset luns ", func() { - spec := getAzureMachineSpec() - lun1 := int32(1) - spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ - { - Lun: &lun1, - Caching: "None", - DiskSizeGB: 75, - StorageAccountType: "Standard_LRS", - }, - { - Lun: nil, - Caching: "None", - DiskSizeGB: 75, - StorageAccountType: "Standard_LRS", - }, - } - - errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) - - errExpected := field.ErrorList{ - { - Type: field.ErrorTypeRequired, - Field: "spec.properties.storageProfile.dataDisks", - BadValue: "", - Detail: "All Data Disk Luns must be set", - }, - } - Expect(errList).To(ConsistOf(errExpected)) - }) - - It("should allow all luns to be nil ", func() { - spec := getAzureMachineSpec() - spec.Properties.StorageProfile.DataDisks = []machine.AzureDataDisk{ - { - Lun: nil, - Caching: "None", - DiskSizeGB: 75, - StorageAccountType: "Standard_LRS", - }, - { - Lun: nil, - Caching: "None", - DiskSizeGB: 75, - StorageAccountType: "Standard_LRS", - }, - } - - errList := validateAzureMachineClassSpec(spec, field.NewPath("spec")) - Expect(errList).To(BeEmpty()) - }) - }) }) diff --git a/pkg/driver/driver_azure.go b/pkg/driver/driver_azure.go index 1c6350e9c..9c67657b2 100644 --- a/pkg/driver/driver_azure.go +++ b/pkg/driver/driver_azure.go @@ -209,7 +209,7 @@ func (d *AzureDriver) generateDataDisks(vmName string, azureDataDisks []v1alpha1 dataDiskLun = &lun } - dataDiskName := dependencyNameFromVMNameAndDependency(getAzureDataDiskName(azureDataDisk.Name, dataDiskLun), vmName, dataDiskSuffix) + dataDiskName := dependencyNameFromVMNameAndDependency(getAzureDataDiskPrefix(azureDataDisk.Name, dataDiskLun), vmName, dataDiskSuffix) var caching compute.CachingTypes if azureDataDisk.Caching != "" { @@ -267,8 +267,7 @@ func (d *AzureDriver) Delete(machineID string) error { var dataDiskNames []string if d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks != nil && len(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) > 0 { - azDataDiskNames := getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) - dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) + dataDiskNames = getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks, vmName, dataDiskSuffix) } if err := clients.deleteVMNicDisks(ctx, resourceGroupName, vmName, nicName, diskName, dataDiskNames); err != nil { @@ -428,8 +427,7 @@ func (d *AzureDriver) createVMNicDisk() (*compute.VirtualMachine, error) { var dataDiskNames []string if d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks != nil && len(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) > 0 { - azDataDiskNames := getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) - dataDiskNames = dependencyNamesFromVMName(azDataDiskNames, vmName, dataDiskSuffix) + dataDiskNames = getAzureDataDiskNames(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks, vmName, dataDiskSuffix) } /* @@ -1013,7 +1011,7 @@ const ( dataDiskSuffix = "-data-disk" ) -func getAzureDataDiskNames(azureDataDisks []v1alpha1.AzureDataDisk) []string { +func getAzureDataDiskNames(azureDataDisks []v1alpha1.AzureDataDisk, vmname, suffix string) []string { azureDataDiskNames := make([]string, len(azureDataDisks)) for i, disk := range azureDataDisks { var diskLun *int32 @@ -1023,21 +1021,16 @@ func getAzureDataDiskNames(azureDataDisks []v1alpha1.AzureDataDisk) []string { lun := int32(i) diskLun = &lun } - azureDataDiskNames[i] = getAzureDataDiskName(disk.Name, diskLun) + azureDataDiskNames[i] = dependencyNameFromVMNameAndDependency(getAzureDataDiskPrefix(disk.Name, diskLun), vmname, suffix) } return azureDataDiskNames } -func getAzureDataDiskName(name string, lun *int32) string { - return fmt.Sprintf("%s-%d", name, *lun) -} - -func dependencyNamesFromVMName(dependencies []string, vmname, suffix string) []string { - dependencyNames := make([]string, len(dependencies)) - for i, dependency := range dependencies { - dependencyNames[i] = dependencyNameFromVMNameAndDependency(dependency, vmname, suffix) +func getAzureDataDiskPrefix(name string, lun *int32) string { + if name != "" { + return fmt.Sprintf("%s-%d", name, *lun) } - return dependencyNames + return fmt.Sprintf("%d", *lun) } func dependencyNameFromVMNameAndDependency(dependency, vmName, suffix string) string { diff --git a/pkg/driver/driver_azure_test.go b/pkg/driver/driver_azure_test.go index 0d6a79b0a..b88ac8bf3 100644 --- a/pkg/driver/driver_azure_test.go +++ b/pkg/driver/driver_azure_test.go @@ -85,10 +85,12 @@ var _ = Describe("Driver Azure", func() { vmName := "vm" lun1 := int32(0) lun2 := int32(1) + lun3 := int32(42) size1 := int32(10) size2 := int32(100) expectedName1 := "vm-sdb-0-data-disk" - expectedName2 := "vm-sdc-1-data-disk" + expectedName2 := "vm-1-data-disk" + expectedName3 := "vm-sdc-42-data-disk" disks := []v1alpha1.AzureDataDisk{ { Name: "sdb", @@ -96,6 +98,11 @@ var _ = Describe("Driver Azure", func() { DiskSizeGB: size1, }, { + StorageAccountType: "Standard_LRS", + DiskSizeGB: size2, + }, + { + Lun: &lun3, Name: "sdc", StorageAccountType: "Standard_LRS", DiskSizeGB: size2, @@ -124,6 +131,16 @@ var _ = Describe("Driver Azure", func() { DiskSizeGB: &size2, CreateOption: compute.DiskCreateOptionTypes("Empty"), }, + { + Lun: &lun3, + Name: &expectedName3, + Caching: compute.CachingTypes("None"), + ManagedDisk: &compute.ManagedDiskParameters{ + StorageAccountType: compute.StorageAccountTypes("Standard_LRS"), + }, + DiskSizeGB: &size2, + CreateOption: compute.DiskCreateOptionTypes("Empty"), + }, } Expect(disksGenerated).To(Equal(expectedDisks)) From 8614d2dcfcfea12d49a01bc3e78054c50f9fec4c Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Sun, 19 Apr 2020 09:25:59 +0300 Subject: [PATCH 7/9] review fixes --- pkg/driver/driver_azure.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/driver/driver_azure.go b/pkg/driver/driver_azure.go index 9c67657b2..05f031ad3 100644 --- a/pkg/driver/driver_azure.go +++ b/pkg/driver/driver_azure.go @@ -103,9 +103,6 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID imageReference := getImageReference(d) - azureDataDisks := d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks - dataDisks := d.generateDataDisks(vmName, azureDataDisks) - VMParameters := compute.VirtualMachine{ Name: &vmName, Location: &location, @@ -124,7 +121,6 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID DiskSizeGB: &d.AzureMachineClass.Spec.Properties.StorageProfile.OsDisk.DiskSizeGB, CreateOption: compute.DiskCreateOptionTypes(d.AzureMachineClass.Spec.Properties.StorageProfile.OsDisk.CreateOption), }, - DataDisks: &dataDisks, }, OsProfile: &compute.OSProfile{ ComputerName: &vmName, @@ -156,6 +152,11 @@ func (d *AzureDriver) getVMParameters(vmName string, networkInterfaceReferenceID Tags: tagList, } + if d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks != nil && len(d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) > 0 { + dataDisks := d.generateDataDisks(vmName, d.AzureMachineClass.Spec.Properties.StorageProfile.DataDisks) + VMParameters.StorageProfile.DataDisks = &dataDisks + } + if d.AzureMachineClass.Spec.Properties.Zone != nil { VMParameters.Zones = &[]string{strconv.Itoa(*d.AzureMachineClass.Spec.Properties.Zone)} } else if d.AzureMachineClass.Spec.Properties.AvailabilitySet != nil { From ddd71ae676eb2a8aea3d3adfaa276bd0b5ed4f61 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Thu, 23 Apr 2020 07:50:10 +0300 Subject: [PATCH 8/9] fix alicloud datadisk regex --- pkg/apis/machine/validation/alicloudmachineclass.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/machine/validation/alicloudmachineclass.go b/pkg/apis/machine/validation/alicloudmachineclass.go index aeb1617f6..1380c3c4e 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass.go +++ b/pkg/apis/machine/validation/alicloudmachineclass.go @@ -66,7 +66,7 @@ func validateAlicloudMachineClassSpec(spec *machine.AlicloudMachineClassSpec, fl allErrs = append(allErrs, field.Required(fldPath.Child("keyPairName"), "KeyPairName is required")) } - const dataDiskNameFmt string = `[a-zA-Z0-9\.\-_:]+` + const dataDiskNameFmt string = `[a-zA-Z][a-zA-Z0-9\.\-_:]+` var dataDiskNameRegexp = regexp.MustCompile("^" + dataDiskNameFmt + "$") From ede44ddf842d4de6cd749e15b52c8535f8b9e7e4 Mon Sep 17 00:00:00 2001 From: Guy Daich Date: Thu, 23 Apr 2020 08:10:33 +0300 Subject: [PATCH 9/9] fix test --- pkg/apis/machine/validation/alicloudmachineclass_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/machine/validation/alicloudmachineclass_test.go b/pkg/apis/machine/validation/alicloudmachineclass_test.go index 8b75ac761..5b534f5d2 100644 --- a/pkg/apis/machine/validation/alicloudmachineclass_test.go +++ b/pkg/apis/machine/validation/alicloudmachineclass_test.go @@ -142,7 +142,7 @@ var _ = Describe("AliCloudMachineClass Validation", func() { Type: field.ErrorTypeInvalid, Field: "spec.dataDisks[3].name", BadValue: "bad$#%name", - Detail: "Disk name given: bad$#%name does not match the expected pattern (regex used for validation is '[a-zA-Z0-9\\.\\-_:]+')", + Detail: "Disk name given: bad$#%name does not match the expected pattern (regex used for validation is '[a-zA-Z][a-zA-Z0-9\\.\\-_:]+')", }, { Type: field.ErrorTypeInvalid,