✨ Add ability to configure data disk provision type during clone (#3332)

* Updated proposal to include provisioning types * Added data disk provisioning types * Added data disk provisioning types e2e
kubernetes-sigs · Feb 3, 2025 · 38f6b67 · 38f6b67
1 parent ed30520
commit 38f6b67
Show file tree

Hide file tree

Showing 9 changed files with 215 additions and 21 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,51 +185,81 @@ 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 {
 		tc := test
 		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,14 +345,17 @@ 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++ {
 		disk := 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
@@ -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