From 2725f964fbc4b2180ddd20bafafaad03b1c3ce36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Tue, 25 Jun 2024 15:16:09 +0200 Subject: [PATCH] fix: actually pass through and validate chunk size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jakob Möller --- internal/controllers/vgmanager/controller.go | 35 ++++++++++++++++--- .../controllers/vgmanager/controller_test.go | 2 ++ internal/controllers/vgmanager/lvm/lvm.go | 32 +++++++++++++++-- .../controllers/vgmanager/validatelvs_test.go | 26 ++++++++++++-- 4 files changed, 84 insertions(+), 11 deletions(-) diff --git a/internal/controllers/vgmanager/controller.go b/internal/controllers/vgmanager/controller.go index 028c183b8..0f2311f87 100644 --- a/internal/controllers/vgmanager/controller.go +++ b/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" @@ -570,6 +569,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 { @@ -611,7 +614,7 @@ 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") @@ -619,11 +622,16 @@ func (r *Reconciler) addThinPoolToVG(ctx context.Context, vgName string, config 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 { @@ -741,3 +749,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 +} diff --git a/internal/controllers/vgmanager/controller_test.go b/internal/controllers/vgmanager/controller_test.go index 463a5839b..6e6b1a65c 100644 --- a/internal/controllers/vgmanager/controller_test.go +++ b/internal/controllers/vgmanager/controller_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strconv" "strings" "testing" "time" @@ -313,6 +314,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(), diff --git a/internal/controllers/vgmanager/lvm/lvm.go b/internal/controllers/vgmanager/lvm/lvm.go index e1b09c7b1..10e14aba8 100644 --- a/internal/controllers/vgmanager/lvm/lvm.go +++ b/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", diff --git a/internal/controllers/vgmanager/validatelvs_test.go b/internal/controllers/vgmanager/validatelvs_test.go index 19fb9b806..9fc63c99a 100644 --- a/internal/controllers/vgmanager/validatelvs_test.go +++ b/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{