feat: chunk size policy

Signed-off-by: Jakob Möller <[email protected]>

api/v1alpha1/lvmcluster_test.go

@@ -11,6 +11,7 @@ import (
 
 	v1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	k8sresource "k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 )
@@ -60,6 +61,12 @@ var _ = Describe("webhook acceptance tests", func() {
 	It("minimum viable create", func(ctx SpecContext) {
 		resource := defaultLVMClusterInUniqueNamespace(ctx)
 		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig).ToNot(BeNil())
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSizeCalculationPolicy).
+			To(Equal(ChunkSizeCalculationPolicyStatic))
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize).To(BeNil())
+
 		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
 	})
 
@@ -409,6 +416,42 @@ var _ = Describe("webhook acceptance tests", func() {
 		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
 	})
 
+	It("updating ThinPoolConfig.ChunkSizeCalculationPolicy is not allowed", func(ctx SpecContext) {
+		resource := defaultLVMClusterInUniqueNamespace(ctx)
+		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+
+		updated := resource.DeepCopy()
+
+		updated.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSizeCalculationPolicy = ChunkSizeCalculationPolicyHost
+
+		err := k8sClient.Update(ctx, updated)
+		Expect(err).To(HaveOccurred())
+		Expect(err).To(Satisfy(k8serrors.IsForbidden))
+		statusError := &k8serrors.StatusError{}
+		Expect(errors.As(err, &statusError)).To(BeTrue())
+		Expect(statusError.Status().Message).To(ContainSubstring(ErrThinPoolConfigCannotBeChanged.Error()))
+
+		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+	})
+
+	It("updating ThinPoolConfig.ChunkSize is not allowed", func(ctx SpecContext) {
+		resource := defaultLVMClusterInUniqueNamespace(ctx)
+		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+
+		updated := resource.DeepCopy()
+
+		updated.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize = ptr.To(k8sresource.MustParse("500Ki"))
+
+		err := k8sClient.Update(ctx, updated)
+		Expect(err).To(HaveOccurred())
+		Expect(err).To(Satisfy(k8serrors.IsForbidden))
+		statusError := &k8serrors.StatusError{}
+		Expect(errors.As(err, &statusError)).To(BeTrue())
+		Expect(statusError.Status().Message).To(ContainSubstring(ErrThinPoolConfigCannotBeChanged.Error()))
+
+		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+	})
+
 	It("device paths cannot be added to device class in update", func(ctx SpecContext) {
 		resource := defaultLVMClusterInUniqueNamespace(ctx)
 		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
@@ -514,4 +557,61 @@ var _ = Describe("webhook acceptance tests", func() {
 
 		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
 	})
+
+	It("chunk size change before create", func(ctx SpecContext) {
+		resource := defaultLVMClusterInUniqueNamespace(ctx)
+		resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize = ptr.To(k8sresource.MustParse("256Ki"))
+		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig).ToNot(BeNil())
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSizeCalculationPolicy).
+			To(Equal(ChunkSizeCalculationPolicyStatic))
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize).ToNot(BeNil())
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize.String()).To(Equal("256Ki"))
+
+		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+	})
+
+	It("host chunk policy with chunk size is forbidden", func(ctx SpecContext) {
+		resource := defaultLVMClusterInUniqueNamespace(ctx)
+		resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSizeCalculationPolicy = ChunkSizeCalculationPolicyHost
+		resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize = ptr.To(k8sresource.MustParse("256Ki"))
+		err := k8sClient.Create(ctx, resource)
+		Expect(err).To(HaveOccurred())
+		Expect(err).To(Satisfy(k8serrors.IsForbidden))
+	})
+
+	It("thin pool chunk size <64KiB is forbidden", func(ctx SpecContext) {
+		resource := defaultLVMClusterInUniqueNamespace(ctx)
+
+		resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize = ptr.To(k8sresource.MustParse("32Ki"))
+
+		err := k8sClient.Create(ctx, resource)
+		Expect(err).To(HaveOccurred())
+		Expect(err).To(Satisfy(k8serrors.IsForbidden))
+	})
+
+	It("thin pool chunk size >1Gi is forbidden", func(ctx SpecContext) {
+		resource := defaultLVMClusterInUniqueNamespace(ctx)
+
+		resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize = ptr.To(k8sresource.MustParse("2Gi"))
+
+		err := k8sClient.Create(ctx, resource)
+		Expect(err).To(HaveOccurred())
+		Expect(err).To(Satisfy(k8serrors.IsForbidden))
+	})
+
+	It("host chunk policy without chunk size is ok", func(ctx SpecContext) {
+		resource := defaultLVMClusterInUniqueNamespace(ctx)
+		resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSizeCalculationPolicy = ChunkSizeCalculationPolicyHost
+		Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig).ToNot(BeNil())
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSizeCalculationPolicy).
+			To(Equal(ChunkSizeCalculationPolicyHost))
+		Expect(resource.Spec.Storage.DeviceClasses[0].ThinPoolConfig.ChunkSize).To(BeNil())
+
+		Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+	})
+
 })

api/v1alpha1/lvmcluster_types.go

@@ -18,6 +18,7 @@ package v1alpha1
 
 import (
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -50,8 +51,40 @@ type ThinPoolConfig struct {
 	// +kubebuilder:validation:Required
 	// +required
 	OverprovisionRatio int `json:"overprovisionRatio"`
+
+	// ChunkSizeCalculationPolicy specifies the policy to calculate the chunk size for the underlying volume.
+	// When set to Host, the chunk size is calculated based on the lvm2 host setting on the node.
+	// When set to Static, the chunk size is calculated based on the static size attribute provided within ChunkSize.
+	// +kubebuilder:default=Static
+	// +kubebuilder:validation:Enum=Host;Static
+	// +required
+	ChunkSizeCalculationPolicy ChunkSizeCalculationPolicy `json:"chunkSizeCalculationPolicy,omitempty"`
+
+	// ChunkSize specifies the statically calculated chunk size for the thin pool.
+	// Thus, It is only used when the ChunkSizeCalculationPolicy is set to Static.
+	// No ChunkSize with a ChunkSizeCalculationPolicy set to Static will result in a default chunk size of 128Ki.
+	// It can be between 64Ki and 1Gi due to the underlying limitations of lvm2.
+	// +optional
+	ChunkSize *resource.Quantity `json:"chunkSize,omitempty"`
 }
 
+// ChunkSizeCalculationPolicy specifies the policy to calculate the chunk size for the underlying volume.
+// for more information, see man lvm.
+type ChunkSizeCalculationPolicy string
+
+const (
+	// ChunkSizeCalculationPolicyHost calculates the chunk size based on the lvm2 host setting on the node.
+	ChunkSizeCalculationPolicyHost ChunkSizeCalculationPolicy = "Host"
+	// ChunkSizeCalculationPolicyStatic calculates the chunk size based on a static size attribute.
+	ChunkSizeCalculationPolicyStatic ChunkSizeCalculationPolicy = "Static"
+)
+
+var (
+	ChunkSizeMinimum = resource.MustParse("64Ki")
+	ChunkSizeDefault = resource.MustParse("128Ki")
+	ChunkSizeMaximum = resource.MustParse("1Gi")
+)
+
 type DeviceFilesystemType string
 
 const (

api/v1alpha1/lvmcluster_webhook.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/openshift/lvm-operator/internal/cluster"
@@ -114,6 +115,11 @@ func (v *lvmClusterValidator) ValidateCreate(ctx context.Context, obj runtime.Ob
 		return warnings, err
 	}
 
+	err = v.verifyChunkSize(l)
+	if err != nil {
+		return warnings, err
+	}
+
 	return warnings, nil
 }
 
@@ -172,7 +178,7 @@ func (v *lvmClusterValidator) ValidateUpdate(_ context.Context, old, new runtime
 
 		if (newThinPoolConfig != nil && oldThinPoolConfig == nil && !errors.Is(err, ErrDeviceClassNotFound)) ||
 			(newThinPoolConfig == nil && oldThinPoolConfig != nil) {
-			return warnings, fmt.Errorf("ThinPoolConfig can not be changed")
+			return warnings, ErrThinPoolConfigCannotBeChanged
 		}
 
 		if newThinPoolConfig != nil && oldThinPoolConfig != nil {
@@ -182,6 +188,10 @@ func (v *lvmClusterValidator) ValidateUpdate(_ context.Context, old, new runtime
 				return warnings, fmt.Errorf("ThinPoolConfig.SizePercent is invalid: %w", ErrThinPoolConfigCannotBeChanged)
 			} else if newThinPoolConfig.OverprovisionRatio != oldThinPoolConfig.OverprovisionRatio {
 				return warnings, fmt.Errorf("ThinPoolConfig.OverprovisionRatio is invalid: %w", ErrThinPoolConfigCannotBeChanged)
+			} else if newThinPoolConfig.ChunkSizeCalculationPolicy != oldThinPoolConfig.ChunkSizeCalculationPolicy {
+				return warnings, fmt.Errorf("ThinPoolConfig.ChunkSizeCalculationPolicy is invalid: %w", ErrThinPoolConfigCannotBeChanged)
+			} else if !reflect.DeepEqual(newThinPoolConfig.ChunkSize, oldThinPoolConfig.ChunkSize) {
+				return warnings, fmt.Errorf("ThinPoolConfig.ChunkSize is invalid: %w", ErrThinPoolConfigCannotBeChanged)
 			}
 		}
 
@@ -216,17 +226,14 @@ func (v *lvmClusterValidator) ValidateUpdate(_ context.Context, old, new runtime
 			return warnings, ErrDevicePathsCannotBeAddedInUpdate
 		}
 
-		// Validate all the old paths still exist
-		err := validateDevicePathsStillExist(oldDevices, newDevices)
-		if err != nil {
+		if err := validateDevicePathsStillExist(oldDevices, newDevices); err != nil {
 			return warnings, fmt.Errorf("invalid: required device paths were deleted from the LVMCluster: %w", err)
 		}
 
-		// Validate all the old optional paths still exist
-		err = validateDevicePathsStillExist(oldOptionalDevices, newOptionalDevices)
-		if err != nil {
+		if err := validateDevicePathsStillExist(oldOptionalDevices, newOptionalDevices); err != nil {
 			return warnings, fmt.Errorf("invalid: optional device paths were deleted from the LVMCluster: %w", err)
 		}
+
 	}
 
 	return warnings, nil
@@ -447,3 +454,25 @@ func (v *lvmClusterValidator) verifyFstype(l *LVMCluster) error {
 
 	return nil
 }
+
+func (v *lvmClusterValidator) verifyChunkSize(l *LVMCluster) error {
+	for _, dc := range l.Spec.Storage.DeviceClasses {
+		if dc.ThinPoolConfig == nil {
+			continue
+		}
+		if dc.ThinPoolConfig.ChunkSizeCalculationPolicy == ChunkSizeCalculationPolicyHost && dc.ThinPoolConfig.ChunkSize != nil {
+			return fmt.Errorf("chunk size can not be set when chunk size calculation policy is set to Host")
+		}
+
+		if dc.ThinPoolConfig.ChunkSize != nil {
+			if dc.ThinPoolConfig.ChunkSize.Cmp(ChunkSizeMinimum) < 0 {
+				return fmt.Errorf("chunk size must be greater than or equal to %s", ChunkSizeMinimum.String())
+			}
+			if dc.ThinPoolConfig.ChunkSize.Cmp(ChunkSizeMaximum) > 0 {
+				return fmt.Errorf("chunk size must be less than or equal to %s", ChunkSizeMaximum.String())
+			}
+		}
+	}
+
+	return nil
+}

bundle/manifests/lvm.topolvm.io_lvmclusters.yaml

@@ -187,6 +187,27 @@ spec:
                             create a thin pool in the LVM volume group. If you exclude
                             this field, logical volumes are thick provisioned.
                           properties:
+                            chunkSize:
+                              anyOf:
+                              - type: integer
+                              - type: string
+                              description: |-
+                                ChunkSize specifies the statically calculated chunk size for the thin pool.
+                                Thus, It is only used when the ChunkSizeCalculationPolicy is set to Static.
+                                No ChunkSize with a ChunkSizeCalculationPolicy set to Static will result in a default chunk size of 128Ki.
+                                It can be between 64Ki and 1Gi due to the underlying limitations of lvm2.
+                              pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
+                              x-kubernetes-int-or-string: true
+                            chunkSizeCalculationPolicy:
+                              default: Static
+                              description: |-
+                                ChunkSizeCalculationPolicy specifies the policy to calculate the chunk size for the underlying volume.
+                                When set to Host, the chunk size is calculated based on the lvm2 host setting on the node.
+                                When set to Static, the chunk size is calculated based on the static size attribute provided within ChunkSize.
+                              enum:
+                              - Host
+                              - Static
+                              type: string
                             name:
                               description: Name specifies a name for the thin pool.
                               type: string

bundle/manifests/lvm.topolvm.io_lvmvolumegroups.yaml

@@ -150,6 +150,27 @@ spec:
               thinPoolConfig:
                 description: ThinPoolConfig contains configurations for the thin-pool
                 properties:
+                  chunkSize:
+                    anyOf:
+                    - type: integer
+                    - type: string
+                    description: |-
+                      ChunkSize specifies the statically calculated chunk size for the thin pool.
+                      Thus, It is only used when the ChunkSizeCalculationPolicy is set to Static.
+                      No ChunkSize with a ChunkSizeCalculationPolicy set to Static will result in a default chunk size of 128Ki.
+                      It can be between 64Ki and 1Gi due to the underlying limitations of lvm2.
+                    pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
+                    x-kubernetes-int-or-string: true
+                  chunkSizeCalculationPolicy:
+                    default: Static
+                    description: |-
+                      ChunkSizeCalculationPolicy specifies the policy to calculate the chunk size for the underlying volume.
+                      When set to Host, the chunk size is calculated based on the lvm2 host setting on the node.
+                      When set to Static, the chunk size is calculated based on the static size attribute provided within ChunkSize.
+                    enum:
+                    - Host
+                    - Static
+                    type: string
                   name:
                     description: Name specifies a name for the thin pool.
                     type: string