diff --git a/apis/v1beta1/types.go b/apis/v1beta1/types.go index f7bcaefb58..4652d201dd 100644 --- a/apis/v1beta1/types.go +++ b/apis/v1beta1/types.go @@ -220,8 +220,30 @@ type VSphereDisk struct { // SizeGiB is the size of the disk in GiB. // +kubebuilder:validation:Required SizeGiB int32 `json:"sizeGiB"` + // ProvisioningMode specifies the provisioning type to be used by this vSphere data disk. + // If not set, the setting will be provided by the default storage policy. + // +optional + ProvisioningMode ProvisioningMode `json:"provisioningMode,omitempty"` } +// ProvisioningMode represents the various provisioning types available to a VMs disk. +// +kubebuilder:validation:Enum=Thin;Thick;EagerlyZeroed +type ProvisioningMode string + +var ( + // ThinProvisioningMode creates the disk using thin provisioning. This means a sparse (allocate on demand) + // format with additional space optimizations. + ThinProvisioningMode ProvisioningMode = "Thin" + + // ThickProvisioningMode creates the disk with all space allocated. + ThickProvisioningMode ProvisioningMode = "Thick" + + // EagerlyZeroedProvisioningMode creates the disk using eager zero provisioning. An eager zeroed thick disk + // has all space allocated and wiped clean of any previous contents on the physical media at + // creation time. Such disks may take longer time during creation compared to other disk formats. + EagerlyZeroedProvisioningMode ProvisioningMode = "EagerlyZeroed" +) + // VSphereMachineTemplateResource describes the data needed to create a VSphereMachine from a template. type VSphereMachineTemplateResource struct { diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml index 70097e9400..baecc54089 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachines.yaml @@ -986,6 +986,15 @@ spec: Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to clearly identify purpose of the disk. type: string + provisioningMode: + description: |- + ProvisioningMode specifies the provisioning type to be used by this vSphere data disk. + If not set, the setting will be provided by the default storage policy. + enum: + - Thin + - Thick + - EagerlyZeroed + type: string sizeGiB: description: SizeGiB is the size of the disk in GiB. format: int32 diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml index 4801fef034..531a8d07c3 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspheremachinetemplates.yaml @@ -856,6 +856,15 @@ spec: Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to clearly identify purpose of the disk. type: string + provisioningMode: + description: |- + ProvisioningMode specifies the provisioning type to be used by this vSphere data disk. + If not set, the setting will be provided by the default storage policy. + enum: + - Thin + - Thick + - EagerlyZeroed + type: string sizeGiB: description: SizeGiB is the size of the disk in GiB. format: int32 diff --git a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml index 438341d44c..d6af59211f 100644 --- a/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml +++ b/config/default/crd/bases/infrastructure.cluster.x-k8s.io_vspherevms.yaml @@ -1076,6 +1076,15 @@ spec: Name is used to identify the disk definition. Name is required and needs to be unique so that it can be used to clearly identify purpose of the disk. type: string + provisioningMode: + description: |- + ProvisioningMode specifies the provisioning type to be used by this vSphere data disk. + If not set, the setting will be provided by the default storage policy. + enum: + - Thin + - Thick + - EagerlyZeroed + type: string sizeGiB: description: SizeGiB is the size of the disk in GiB. format: int32 diff --git a/docs/proposal/20241003-data-disk-support.md b/docs/proposal/20241003-data-disk-support.md index 628088fb22..403a5d5f96 100644 --- a/docs/proposal/20241003-data-disk-support.md +++ b/docs/proposal/20241003-data-disk-support.md @@ -6,7 +6,8 @@ title: CAPV Additional Data Disks When Creating New Machines authors: - "@vr4manta" reviewers: - - TBD + - "@chrischdi" + - "@neolit123" creation-date: 2024-10-03 last-updated: 2024-10-03 status: implementable @@ -113,9 +114,33 @@ type VSphereDisk struct { // SizeGiB is the size of the disk (in GiB). // +kubebuilder:validation:Required SizeGiB int32 `json:"sizeGiB"` + // ProvisioningMode specifies the provisioning type to be used by this vSphere data disk. + // If not set, the setting will be provided by the default storage policy. + // +optional + ProvisioningMode ProvisioningMode `json:"provisioningMode,omitempty"` } ``` +Provisioning type currently will be represented by the following configuration options: + +```go +type ProvisioningType string + +var ( + // ThinProvisioningMode creates the disk using thin provisioning. This means a sparse (allocate on demand) + // format with additional space optimizations. + ThinProvisioningMode ProvisioningMode = "Thin" + + // ThickProvisioningMode creates the disk with all space allocated. + ThickProvisioningMode ProvisioningMode = "Thick" + + // EagerlyZeroedProvisioningMode creates the disk using eager zero provisioning. An eager zeroed thick disk + // has all space allocated and wiped clean of any previous contents on the physical media at + // creation time. Such disks may take longer time during creation compared to other disk formats. + EagerlyZeroedProvisioningMode ProvisioningMode = "EagerlyZeroed" +) +``` + The above type has been added to the machine spec section ```go @@ -148,8 +173,10 @@ spec: dataDisks: - name: images sizeGiB: 50 + provisioningType: Thin - name: swap sizeGiB: 90 + provisioningType: Thick cloneMode: linkedClone datacenter: cidatacenter datastore: /cidatacenter/datastore/vsanDatastore @@ -175,8 +202,10 @@ spec: dataDisks: - name: images sizeGiB: 50 + provisioningType: Thin - name: swap sizeGiB: 90 + provisioningType: Thick datacenter: cidatacenter datastore: /cidatacenter/datastore/vsanDatastore diskGiB: 128 @@ -193,7 +222,7 @@ spec: ``` -In the above examples, two data disks will be created during the clone process and placed after the OS disk. The example shows a 50 GiB with name `images` and a 90 GiB disk with name `swap` being configured and added. +In the above examples, two data disks will be created during the clone process and placed after the OS disk. The example shows a 50 GiB with name `images` that will be thin provisioned and a 90 GiB disk with name `swap` that will be thick provisioned being configured and added. For each `dataDisks` definition, the clone procedure will attempt to generate a device VirtualDeviceConfigSpec that will be used to create the new disk device. Each disk will be attached in the order in which they are defined in the template. All disks defined in the vSphere OVA template will come first with the new disks being attached after. diff --git a/pkg/services/govmomi/vcenter/clone.go b/pkg/services/govmomi/vcenter/clone.go index 3a032b218d..23c9594c27 100644 --- a/pkg/services/govmomi/vcenter/clone.go +++ b/pkg/services/govmomi/vcenter/clone.go @@ -433,18 +433,34 @@ func createDataDisks(ctx context.Context, dataDiskDefs []infrav1.VSphereDisk, de for i, dataDisk := range dataDiskDefs { log.V(2).Info("Adding disk", "name", dataDisk.Name, "spec", dataDisk) + backing := &types.VirtualDiskFlatVer2BackingInfo{ + DiskMode: string(types.VirtualDiskModePersistent), + VirtualDeviceFileBackingInfo: types.VirtualDeviceFileBackingInfo{ + FileName: "", + }, + } + + // Set provisioning type for the new data disk. + // Currently, if ThinProvisioned is not set, GOVC will set default to false. We may want to change this behavior + // to match what template image OS disk has configured to make them match if not set. + switch dataDisk.ProvisioningMode { + case infrav1.ThinProvisioningMode: + backing.ThinProvisioned = types.NewBool(true) + case infrav1.ThickProvisioningMode: + backing.ThinProvisioned = types.NewBool(false) + case infrav1.EagerlyZeroedProvisioningMode: + backing.ThinProvisioned = types.NewBool(false) + backing.EagerlyScrub = types.NewBool(true) + default: + log.V(2).Info("No provisioning type detected. Leaving configuration empty.") + } + dev := &types.VirtualDisk{ VirtualDevice: types.VirtualDevice{ // Key needs to be unique and cannot match another new disk being added. So we'll use the index as an // input to NewKey. NewKey() will always return same value since our new devices are not part of devices yet. - Key: devices.NewKey() - int32(i), - Backing: &types.VirtualDiskFlatVer2BackingInfo{ - DiskMode: string(types.VirtualDiskModePersistent), - ThinProvisioned: types.NewBool(true), - VirtualDeviceFileBackingInfo: types.VirtualDeviceFileBackingInfo{ - FileName: "", - }, - }, + Key: devices.NewKey() - int32(i), + Backing: backing, ControllerKey: controller.GetVirtualController().Key, }, CapacityInKB: int64(dataDisk.SizeGiB) * 1024 * 1024, diff --git a/pkg/services/govmomi/vcenter/clone_test.go b/pkg/services/govmomi/vcenter/clone_test.go index f8182d7564..c85b16a6c1 100644 --- a/pkg/services/govmomi/vcenter/clone_test.go +++ b/pkg/services/govmomi/vcenter/clone_test.go @@ -22,6 +22,7 @@ import ( "fmt" "testing" + "github.com/onsi/gomega" "github.com/vmware/govmomi/object" "github.com/vmware/govmomi/simulator" _ "github.com/vmware/govmomi/vapi/simulator" // run init func to register the tagging API endpoints. @@ -184,44 +185,72 @@ func TestCreateDataDisks(t *testing.T) { name: "Add data disk with 1 ova disk", devices: deviceList, controller: controller, - dataDisks: createDataDiskDefinitions(1), + dataDisks: createDataDiskDefinitions(1, nil), expectedUnitNumber: []int{1}, }, { name: "Add data disk with 2 ova disk", devices: createAdditionalDisks(deviceList, controller, 1), controller: controller, - dataDisks: createDataDiskDefinitions(1), + dataDisks: createDataDiskDefinitions(1, nil), expectedUnitNumber: []int{2}, }, { name: "Add multiple data disk with 1 ova disk", devices: deviceList, controller: controller, - dataDisks: createDataDiskDefinitions(2), + dataDisks: createDataDiskDefinitions(2, nil), expectedUnitNumber: []int{1, 2}, }, { name: "Add too many data disks with 1 ova disk", devices: deviceList, controller: controller, - dataDisks: createDataDiskDefinitions(30), + dataDisks: createDataDiskDefinitions(30, nil), err: "all unit numbers are already in-use", }, { name: "Add data disk with no ova disk", devices: nil, controller: nil, - dataDisks: createDataDiskDefinitions(1), + dataDisks: createDataDiskDefinitions(1, nil), err: "Invalid disk count: 0", }, { name: "Add too many data disks with 1 ova disk", devices: deviceList, controller: controller, - dataDisks: createDataDiskDefinitions(40), + dataDisks: createDataDiskDefinitions(40, nil), err: "all unit numbers are already in-use", }, + { + name: "Create data disk with Thin provisioning", + devices: deviceList, + controller: controller, + dataDisks: createDataDiskDefinitions(1, &infrav1.ThinProvisioningMode), + expectedUnitNumber: []int{1}, + }, + { + name: "Create data disk with Thick provisioning", + devices: deviceList, + controller: controller, + dataDisks: createDataDiskDefinitions(1, &infrav1.ThickProvisioningMode), + expectedUnitNumber: []int{1}, + }, + { + name: "Create data disk with EagerZeroed provisioning", + devices: deviceList, + controller: controller, + dataDisks: createDataDiskDefinitions(1, &infrav1.EagerlyZeroedProvisioningMode), + expectedUnitNumber: []int{1}, + }, + { + name: "Create data disk without provisioning type set", + devices: deviceList, + controller: controller, + dataDisks: createDataDiskDefinitions(1, nil), + expectedUnitNumber: []int{1}, + }, } for _, test := range testCases { @@ -229,6 +258,8 @@ func TestCreateDataDisks(t *testing.T) { t.Run(tc.name, func(t *testing.T) { var funcError error + g := gomega.NewWithT(t) + // Create the data disks newDisks, funcError := createDataDisks(ctx.TODO(), tc.dataDisks, tc.devices) if (tc.err != "" && funcError == nil) || (tc.err == "" && funcError != nil) || (funcError != nil && tc.err != funcError.Error()) { @@ -255,6 +286,24 @@ func TestCreateDataDisks(t *testing.T) { if tc.err == "" && unitNumber != int32(tc.expectedUnitNumber[index]) { t.Fatalf("Expected to get unitNumber '%d' error from assignUnitNumber, got: '%d'", tc.expectedUnitNumber[index], unitNumber) } + + // Check to see if the provision type matches. + backingInfo := disk.GetVirtualDeviceConfigSpec().Device.GetVirtualDevice().Backing.(*types.VirtualDiskFlatVer2BackingInfo) + switch tc.dataDisks[index].ProvisioningMode { + case infrav1.ThinProvisioningMode: + g.Expect(backingInfo.ThinProvisioned).To(gomega.Equal(types.NewBool(true))) + g.Expect(backingInfo.EagerlyScrub).To(gomega.BeNil()) + case infrav1.ThickProvisioningMode: + g.Expect(backingInfo.ThinProvisioned).To(gomega.Equal(types.NewBool(false))) + g.Expect(backingInfo.EagerlyScrub).To(gomega.BeNil()) + case infrav1.EagerlyZeroedProvisioningMode: + g.Expect(backingInfo.ThinProvisioned).To(gomega.Equal(types.NewBool(false))) + g.Expect(backingInfo.EagerlyScrub).To(gomega.Equal(types.NewBool(true))) + default: + // Currently, if not set, GOVC will set default to false. We may want to change this behavior to match what template image OS disk is to make them match if not set. + g.Expect(backingInfo.ThinProvisioned).To(gomega.BeNil()) + g.Expect(backingInfo.EagerlyScrub).To(gomega.BeNil()) + } } } }) @@ -296,7 +345,7 @@ func createVirtualDisk(key int32, controller types.BaseVirtualController, diskSi return dev } -func createDataDiskDefinitions(numOfDataDisks int) []infrav1.VSphereDisk { +func createDataDiskDefinitions(numOfDataDisks int, provisionType *infrav1.ProvisioningMode) []infrav1.VSphereDisk { disks := []infrav1.VSphereDisk{} for i := 0; i < numOfDataDisks; i++ { @@ -304,6 +353,9 @@ func createDataDiskDefinitions(numOfDataDisks int) []infrav1.VSphereDisk { Name: fmt.Sprintf("disk_%d", i), SizeGiB: 10 * int32(i), } + if provisionType != nil { + disk.ProvisioningMode = *provisionType + } disks = append(disks, disk) } return disks diff --git a/test/e2e/data/infrastructure-vsphere-govmomi/main/multi-disk/data-disks-patch.yaml b/test/e2e/data/infrastructure-vsphere-govmomi/main/multi-disk/data-disks-patch.yaml index fcb61923e9..914f2e2c65 100644 --- a/test/e2e/data/infrastructure-vsphere-govmomi/main/multi-disk/data-disks-patch.yaml +++ b/test/e2e/data/infrastructure-vsphere-govmomi/main/multi-disk/data-disks-patch.yaml @@ -8,9 +8,11 @@ spec: spec: dataDisks: - name: "disk_1" - sizeGiB: 10 + sizeGiB: 1 + provisioningType: "Thin" - name: "disk_2" - sizeGiB: 20 + sizeGiB: 2 + provisioningType: "Thick" --- apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 kind: VSphereMachineTemplate @@ -22,4 +24,13 @@ spec: spec: dataDisks: - name: "disk_1" - sizeGiB: 20 + sizeGiB: 1 + provisioningType: "Thin" + - name: "disk_2" + sizeGiB: 2 + provisioningType: "Thick" + - name: "disk_3" + sizeGiB: 3 + provisioningType: "EagerlyZeroed" + - name: "disk_4" + sizeGiB: 4 diff --git a/test/e2e/multi_disk_test.go b/test/e2e/multi_disk_test.go index 848e70dba1..161fed667c 100644 --- a/test/e2e/multi_disk_test.go +++ b/test/e2e/multi_disk_test.go @@ -27,6 +27,8 @@ import ( capi_e2e "sigs.k8s.io/cluster-api/test/e2e" "sigs.k8s.io/cluster-api/test/framework" . "sigs.k8s.io/cluster-api/test/framework/ginkgoextensions" + + infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1" ) type diskSpecInput struct { @@ -83,7 +85,7 @@ func verifyDisks(ctx context.Context, input diskSpecInput) { for _, vm := range vms.Items { // vSphere machine object should have the data disks configured. We will add +1 to the count since the os image // needs to be included for comparison. - Byf("VM %s Spec has %d DataDisks defined", vm.Name, len(vm.Spec.DataDisks)) + Byf("VM %s Spec has %d DataDisk(s) defined", vm.Name, len(vm.Spec.DataDisks)) diskCount := 1 + len(vm.Spec.DataDisks) Expect(diskCount).ToNot(Equal(1), "Total disk count should be larger than 1 for this test") @@ -96,5 +98,40 @@ func verifyDisks(ctx context.Context, input diskSpecInput) { // We expect control plane VMs to have 3 disks, and the compute VMs will have 2. disks := devices.SelectByType((*types.VirtualDisk)(nil)) Expect(disks).To(HaveLen(diskCount), fmt.Sprintf("Disk count of VM should be %d", diskCount)) + + // Check each disk to see if its provisioning type matches the expected + var osDiskBackingInfo *types.VirtualDiskFlatVer2BackingInfo + for diskIndex, disk := range disks { + // Skip first disk since it is the OS + if diskIndex == 0 { + osDiskBackingInfo = disk.GetVirtualDevice().Backing.(*types.VirtualDiskFlatVer2BackingInfo) + continue + } + + // Get the backing info and perform check + diskConfig := vm.Spec.DataDisks[diskIndex-1] + backingInfo := disk.GetVirtualDevice().Backing.(*types.VirtualDiskFlatVer2BackingInfo) + By(fmt.Sprintf("Checking provision type \"%v\"", diskConfig.ProvisioningMode)) + switch diskConfig.ProvisioningMode { + case infrav1.ThinProvisioningMode: + Expect(backingInfo.ThinProvisioned).To(Equal(types.NewBool(true)), "ThinProvisioned should be true for resulting disk when data disk provisionType is ThinProvisioned") + Expect(backingInfo.EagerlyScrub).To(Equal(types.NewBool(false)), "EagerlyScrub should be false for resulting disk when data disk provisionType is ThinProvisioned") + case infrav1.ThickProvisioningMode: + Expect(backingInfo.ThinProvisioned).To(Equal(types.NewBool(false)), "ThinProvisioned should be false for resulting disk when data disk provisionType is ThickProvisioned") + Expect(backingInfo.EagerlyScrub).To(Equal(types.NewBool(false)), "EagerlyScrub should be false for resulting disk when data disk provisionType is ThickProvisioned") + case infrav1.EagerlyZeroedProvisioningMode: + Expect(backingInfo.ThinProvisioned).To(Equal(types.NewBool(false)), "ThinProvisioned should be false for resulting disk when data disk provisionType is EagerlyZeroed") + Expect(backingInfo.EagerlyScrub).To(Equal(types.NewBool(true)), "EagerlyScrub should be true for resulting disk when data disk provisionType is EagerlyZeroed") + default: + // Currently, the settings for default behavior of disks during clone can come from templates settings, + // the default storage policy, or the clone's settings. Our tests will compare against os disk to make + // sure they are the same. + Expect(backingInfo.ThinProvisioned).To(Equal(osDiskBackingInfo.ThinProvisioned), "ThinProvisioned should match OS disk for resulting disk when data disk provisionType is not set") + Expect(backingInfo.EagerlyScrub).To(Equal(osDiskBackingInfo.EagerlyScrub), "EagerlyScrub should match OS disk for resulting disk when data disk provisionType is not set") + } + + // Check disk size + Expect((disk.(*types.VirtualDisk)).CapacityInKB).To(Equal(int64(diskConfig.SizeGiB*1024*1024)), "Resulting disk size should match the size configured for that data disk") + } } }