Merge pull request #644 from jakobmoellerdev/OCPBUGS-35404-fixup-back…

…port

[openshift-4.16] OCPBUGS-35404: fix: actually pass through and validate chunk size

internal/controllers/vgmanager/controller.go

@@ -30,7 +30,6 @@ import (
 	"github.com/openshift/lvm-operator/internal/controllers/vgmanager/lvm"
 	"github.com/openshift/lvm-operator/internal/controllers/vgmanager/lvmd"
 	"github.com/openshift/lvm-operator/internal/controllers/vgmanager/wipefs"
-	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/utils/ptr"
 
 	corev1 "k8s.io/api/core/v1"
@@ -577,6 +576,10 @@ func (r *Reconciler) validateLVs(ctx context.Context, volumeGroup *lvmv1alpha1.L
 					"you should manually extend the metadata_partition or you will risk data loss: metadata_percent: %v", metadataPercentage, lv.MetadataPercent)
 			}
 
+			if err := verifyChunkSizeForPolicy(volumeGroup.Spec.ThinPoolConfig, lv); err != nil {
+				return err
+			}
+
 			logger.V(1).Info("confirmed created logical volume has correct attributes", "lv_attr", lvAttr.String())
 		}
 		if !thinPoolExists {
@@ -618,19 +621,24 @@ func (r *Reconciler) addThinPoolToVG(ctx context.Context, vgName string, config
 	}
 
 	logger.Info("creating lvm thinpool")
-	if err := r.LVM.CreateLV(ctx, config.Name, vgName, config.SizePercent, convertChunkSize(config.ChunkSize)); err != nil {
+	if err := r.LVM.CreateLV(ctx, config.Name, vgName, config.SizePercent, convertChunkSize(config)); err != nil {
 		return fmt.Errorf("failed to create thinpool: %w", err)
 	}
 	logger.Info("successfully created thinpool")
 
 	return nil
 }
 
-func convertChunkSize(size *resource.Quantity) int64 {
-	if size == nil {
+// convertChunkSize converts the chunk size from the ThinPoolConfig to the correct value for the LVM API
+// if the ChunkSizeCalculationPolicy is set to Host, it will return -1, signaling the LVM API to use the Host value.
+func convertChunkSize(config *lvmv1alpha1.ThinPoolConfig) int64 {
+	if config.ChunkSizeCalculationPolicy == lvmv1alpha1.ChunkSizeCalculationPolicyHost {
+		return -1
+	}
+	if config.ChunkSize == nil {
 		return lvmv1alpha1.ChunkSizeDefault.Value()
 	}
-	return size.Value()
+	return config.ChunkSize.Value()
 }
 
 func (r *Reconciler) extendThinPool(ctx context.Context, vgName string, lvSize string, config *lvmv1alpha1.ThinPoolConfig) error {
@@ -748,3 +756,20 @@ func (r *Reconciler) NormalEvent(ctx context.Context, obj *lvmv1alpha1.LVMVolume
 	r.Event(obj, corev1.EventTypeNormal, string(reason),
 		fmt.Sprintf("update on node %s: %s", client.ObjectKeyFromObject(nodeStatus), message))
 }
+
+func verifyChunkSizeForPolicy(config *lvmv1alpha1.ThinPoolConfig, lv lvm.LogicalVolume) error {
+	if config.ChunkSizeCalculationPolicy == lvmv1alpha1.ChunkSizeCalculationPolicyHost {
+		// Host policy means that the chunk size is not set by the user, but by the host and cannot be validated
+		// against the spec
+		return nil
+	}
+	if chunkSizeBytes, err := strconv.ParseInt(lv.ChunkSize, 10, 64); err == nil {
+		if chunkSizeBytes != convertChunkSize(config) {
+			return fmt.Errorf("chunk size of logical volume %s does not match the static policy: %w", lv.Name, err)
+		}
+	} else {
+		return fmt.Errorf("could not parse chunk size from logical volume %s to verify against static policy: %w", lv.Name, err)
+	}
+
+	return nil
+}

internal/controllers/vgmanager/controller_test.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -306,6 +307,7 @@ func testVGWithLocalDevice(ctx context.Context, vgTemplate lvmv1alpha1.LVMVolume
 				LvAttr:          "twi---tz--",
 				LvSize:          "1.0G",
 				MetadataPercent: "10.0",
+				ChunkSize:       strconv.FormatInt(ptr.To(resource.MustParse("128Ki")).Value(), 10),
 			}
 			createdVG = lvm.VolumeGroup{
 				Name:   vg.GetName(),

internal/controllers/vgmanager/lvm/lvm.go

@@ -51,6 +51,18 @@ const (
 	lvmsTag = "@lvms"
 )
 
+var (
+	DefaultListLVColumns = []string{
+		"lv_name",
+		"vg_name",
+		"pool_lv",
+		"lv_attr",
+		"lv_size",
+		"metadata_percent",
+		"chunk_size",
+	}
+)
+
 // VGReport represents the output of the `vgs --reportformat json` command
 type VGReport struct {
 	Report []struct {
@@ -85,6 +97,7 @@ type LogicalVolume struct {
 	LvAttr          string `json:"lv_attr"`
 	LvSize          string `json:"lv_size"`
 	MetadataPercent string `json:"metadata_percent"`
+	ChunkSize       string `json:"chunk_size"`
 }
 
 type LVM interface {
@@ -402,7 +415,15 @@ func (hlvm *HostLVM) ListLVsByName(ctx context.Context, vgName string) ([]string
 func (hlvm *HostLVM) ListLVs(ctx context.Context, vgName string) (*LVReport, error) {
 	res := new(LVReport)
 	args := []string{
-		"-S", fmt.Sprintf("vgname=%s", vgName), "--units", "g", "--reportformat", "json",
+		"-S",
+		fmt.Sprintf("vgname=%s", vgName),
+		"--units",
+		"b",
+		"--nosuffix",
+		"--reportformat",
+		"json",
+		"-o",
+		strings.Join(DefaultListLVColumns, ","),
 	}
 	if err := hlvm.RunCommandAsHostInto(ctx, res, lvsCmd, args...); err != nil {
 		return nil, err
@@ -462,8 +483,13 @@ func (hlvm *HostLVM) CreateLV(ctx context.Context, lvName, vgName string, sizePe
 		return fmt.Errorf("failed to create logical volume in volume group: size percent should be greater than 0")
 	}
 
-	args := []string{"-l", fmt.Sprintf("%d%%FREE", sizePercent),
-		"-c", fmt.Sprintf("%vb", chunkSizeBytes), "-Z", "y", "-T", fmt.Sprintf("%s/%s", vgName, lvName)}
+	args := []string{"-l", fmt.Sprintf("%d%%FREE", sizePercent), "-Z", "y", "-T"}
+
+	if chunkSizeBytes > 0 {
+		args = append(args, "-c", fmt.Sprintf("%vb", chunkSizeBytes))
+	}
+
+	args = append(args, fmt.Sprintf("%s/%s", vgName, lvName))
 
 	if err := hlvm.RunCommandAsHost(ctx, lvCreateCmd, args...); err != nil {
 		return fmt.Errorf("failed to create logical volume %q in the volume group %q using command '%s': %w",

internal/controllers/vgmanager/validatelvs_test.go

@@ -13,7 +13,9 @@ import (
 	mockExec "github.com/openshift/lvm-operator/internal/controllers/vgmanager/exec/test"
 	"github.com/openshift/lvm-operator/internal/controllers/vgmanager/lvm"
 	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/ptr"
 	"k8s.io/utils/strings/slices"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
@@ -51,7 +53,7 @@ var mockLvsOutputThinPoolValid = `
       "report": [
           {
               "lv": [
-                  {"lv_name":"thin-pool-1", "vg_name":"vg1", "lv_attr":"twi-a-tz--", "lv_size":"26.96g", "pool_lv":"", "origin":"", "data_percent":"0.00", "metadata_percent":"10.52", "move_pv":"", "mirror_log":"", "copy_percent":"", "convert_lv":""}
+                  {"lv_name":"thin-pool-1", "vg_name":"vg1", "lv_attr":"twi-a-tz--", "lv_size":"26.96g", "pool_lv":"", "origin":"", "data_percent":"0.00", "metadata_percent":"10.52", "move_pv":"", "mirror_log":"", "copy_percent":"", "convert_lv":"", "chunk_size": "524288"}
               ]
           }
       ]
@@ -105,9 +107,12 @@ func TestVGReconciler_validateLVs(t *testing.T) {
 		"-S",
 		"vgname=vg1",
 		"--units",
-		"g",
+		"b",
+		"--nosuffix",
 		"--reportformat",
 		"json",
+		"-o",
+		strings.Join(lvm.DefaultListLVColumns, ","),
 	}
 
 	mockExecutorForLVSOutput := func(output string) lvmexec.Executor {
@@ -135,11 +140,26 @@ func TestVGReconciler_validateLVs(t *testing.T) {
 			args: args{volumeGroup: &lvmv1alpha1.LVMVolumeGroup{
 				ObjectMeta: metav1.ObjectMeta{Name: "vg1", Namespace: "default"},
 				Spec: lvmv1alpha1.LVMVolumeGroupSpec{ThinPoolConfig: &lvmv1alpha1.ThinPoolConfig{
-					Name: "thin-pool-1",
+					Name:      "thin-pool-1",
+					ChunkSize: ptr.To(resource.MustParse("512Ki")),
 				}},
 			}},
 			wantErr: assert.NoError,
 		},
+		{
+			name: "Invalid LV due to Chunk Size",
+			fields: fields{
+				executor: mockExecutorForLVSOutput(mockLvsOutputRAID),
+			},
+			args: args{volumeGroup: &lvmv1alpha1.LVMVolumeGroup{
+				ObjectMeta: metav1.ObjectMeta{Name: "vg1", Namespace: "default"},
+				Spec: lvmv1alpha1.LVMVolumeGroupSpec{ThinPoolConfig: &lvmv1alpha1.ThinPoolConfig{
+					Name:      "thin-pool-1",
+					ChunkSize: ptr.To(resource.MustParse("1Ki")),
+				}},
+			}},
+			wantErr: assert.Error,
+		},
 		{
 			name: "Invalid LV due to Type not being Thin Pool",
 			fields: fields{