From 7d07b4ac40d6f8e0cf9cae30fa984daa799497a6 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 23 Feb 2023 17:39:09 +0100 Subject: [PATCH 1/5] rbd: cleanup passing `mkfs` arguments for NodeStageVolume Storing the default `mkfs` arguments in a map with key per filesystem type makes this a little more modular. It prepares th code for fetching the `mkfs` arguments from the VolumeContext. Signed-off-by: Niels de Vos --- internal/rbd/nodeserver.go | 40 ++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index 359b3fdf0c2..68b0f0a37bb 100644 --- a/internal/rbd/nodeserver.go +++ b/internal/rbd/nodeserver.go @@ -101,6 +101,15 @@ var ( // xfsHasReflink is set by xfsSupportsReflink(), use the function when // checking the support for reflink. xfsHasReflink = xfsReflinkUnset + + mkfsDefaultArgs = map[string][]string{ + "ext4": {"-m0", "-Enodiscard,lazy_itable_init=1,lazy_journal_init=1"}, + "xfs": {"-K"}, + } + + mountDefaultOpts = map[string][]string{ + "xfs": {"nouuid"}, + } ) // parseBoolOption checks if parameters contain option and parse it. If it is @@ -756,7 +765,8 @@ func (ns *NodeServer) mountVolumeToStagePath( return err } - opt := []string{"_netdev"} + opt := mountDefaultOpts[fsType] + opt = append(opt, "_netdev") opt = csicommon.ConstructMountOptions(opt, req.GetVolumeCapability()) isBlock := req.GetVolumeCapability().GetBlock() != nil rOnly := "ro" @@ -771,34 +781,34 @@ func (ns *NodeServer) mountVolumeToStagePath( readOnly = true } - if fsType == "xfs" { - opt = append(opt, "nouuid") - } - if existingFormat == "" && !staticVol && !readOnly { - args := []string{} + args := mkfsDefaultArgs[fsType] + + // add extra arguments depending on the filesystem + mkfs := "mkfs." + fsType switch fsType { case "ext4": - args = []string{"-m0", "-Enodiscard,lazy_itable_init=1,lazy_journal_init=1"} if fileEncryption { args = append(args, "-Oencrypt") } - args = append(args, devicePath) case "xfs": - args = []string{"-K", devicePath} // always disable reflink // TODO: make enabling an option, see ceph/ceph-csi#1256 if ns.xfsSupportsReflink() { args = append(args, "-m", "reflink=0") } + case "": + // no filesystem type specified, just use "mkfs" + mkfs = "mkfs" } - if len(args) > 0 { - cmdOut, cmdErr := diskMounter.Exec.Command("mkfs."+fsType, args...).CombinedOutput() - if cmdErr != nil { - log.ErrorLog(ctx, "failed to run mkfs error: %v, output: %v", cmdErr, string(cmdOut)) - return cmdErr - } + // add device as last argument + args = append(args, devicePath) + cmdOut, cmdErr := diskMounter.Exec.Command(mkfs, args...).CombinedOutput() + if cmdErr != nil { + log.ErrorLog(ctx, "failed to run mkfs.%s (%v) error: %v, output: %v", fsType, args, cmdErr, string(cmdOut)) + + return cmdErr } } From 8d2f13d4fad8a720b25bbbb7e964f1db1d0bed1d Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 23 Feb 2023 18:07:43 +0100 Subject: [PATCH 2/5] rbd: allow setting `mkfsOptions` in the StorageClass Add `mkfsOptions` to the StorageClass and pass them to the `mkfs` command while creating the filesystem on the RBD device. Fixes: #374 Signed-off-by: Niels de Vos --- docs/deploy-rbd.md | 1 + examples/rbd/storageclass.yaml | 11 +++++++++++ internal/rbd/nodeserver.go | 9 +++++++++ 3 files changed, 21 insertions(+) diff --git a/docs/deploy-rbd.md b/docs/deploy-rbd.md index 0f3838f72d6..270d734d15e 100644 --- a/docs/deploy-rbd.md +++ b/docs/deploy-rbd.md @@ -61,6 +61,7 @@ make image-cephcsi | `volumeNamePrefix` | no | Prefix to use for naming RBD images (defaults to `csi-vol-`). | | `snapshotNamePrefix` | no | Prefix to use for naming RBD snapshot images (defaults to `csi-snap-`). | | `imageFeatures` | no | RBD image features. CSI RBD currently supports `layering`, `journaling`, `exclusive-lock`, `object-map`, `fast-diff`, `deep-flatten` features. deep-flatten is added for cloned images. Refer for image feature dependencies. | +| `mkfsOptions` | no | Options to pass to the `mkfs` command while creating the filesystem on the RBD device. Check the man-page for the `mkfs` command for the filesystem for more details. When `mkfsOptions` is set here, the defaults will not be used, consider including them in this parameter. | | `tryOtherMounters` | no | Specifies whether to try other mounters in case if the current mounter fails to mount the rbd image for any reason | | `mapOptions` | no | Map options to use when mapping rbd image. See [krbd](https://docs.ceph.com/docs/master/man/8/rbd/#kernel-rbd-krbd-options) and [nbd](https://docs.ceph.com/docs/master/man/8/rbd-nbd/#options) options. | | `unmapOptions` | no | Unmap options to use when unmapping rbd image. See [krbd](https://docs.ceph.com/docs/master/man/8/rbd/#kernel-rbd-krbd-options) and [nbd](https://docs.ceph.com/docs/master/man/8/rbd-nbd/#options) options. | diff --git a/examples/rbd/storageclass.yaml b/examples/rbd/storageclass.yaml index 7270b642b58..0e476fa2b19 100644 --- a/examples/rbd/storageclass.yaml +++ b/examples/rbd/storageclass.yaml @@ -37,6 +37,17 @@ parameters: # imageFeatures: layering,journaling,exclusive-lock,object-map,fast-diff imageFeatures: "layering" + # (optional) Options to pass to the `mkfs` command while creating the + # filesystem on the RBD device. Check the man-page for the `mkfs` command + # for the filesystem for more details. When `mkfsOptions` is set here, the + # defaults will not be used, consider including them in this parameter. + # + # The default options depend on the csi.storage.k8s.io/fstype setting: + # - ext4: "-m0 -Enodiscard,lazy_itable_init=1,lazy_journal_init=1" + # - xfs: "-onouuid -K" + # + # mkfsOptions: "-m0 -Ediscard -i1024" + # (optional) Specifies whether to try other mounters in case if the current # mounter fails to mount the rbd image for any reason. True means fallback # to next mounter, default is set to false. diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index 68b0f0a37bb..338b5791de4 100644 --- a/internal/rbd/nodeserver.go +++ b/internal/rbd/nodeserver.go @@ -784,6 +784,15 @@ func (ns *NodeServer) mountVolumeToStagePath( if existingFormat == "" && !staticVol && !readOnly { args := mkfsDefaultArgs[fsType] + // if the VolumeContext contains "mkfsOptions", use those as args instead + volumeCtx := req.GetVolumeContext() + if volumeCtx != nil { + mkfsOptions := volumeCtx["mkfsOptions"] + if mkfsOptions != "" { + args = strings.Split(mkfsOptions, " ") + } + } + // add extra arguments depending on the filesystem mkfs := "mkfs." + fsType switch fsType { From f30f0863c700d4cdcc5cb2645c3b94ae3f9665f2 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 2 Mar 2023 10:30:47 +0100 Subject: [PATCH 3/5] e2e: replace CephFS in messages for RBD tests Signed-off-by: Niels de Vos --- e2e/rbd.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/rbd.go b/e2e/rbd.go index 6c180077834..7f5a28d89d2 100644 --- a/e2e/rbd.go +++ b/e2e/rbd.go @@ -419,7 +419,7 @@ var _ = Describe("RBD", func() { By("verify PVC and app binding on helm installation", func() { err := validatePVCAndAppBinding(pvcPath, appPath, f) if err != nil { - framework.Failf("failed to validate CephFS pvc and application binding: %v", err) + framework.Failf("failed to validate RBD pvc and application binding: %v", err) } // validate created backend rbd images validateRBDImageCount(f, 0, defaultRBDPool) @@ -1207,7 +1207,7 @@ var _ = Describe("RBD", func() { } err = validatePVCAndAppBinding(pvcPath, appPath, f) if err != nil { - framework.Failf("failed to validate CephFS pvc and application binding: %v", err) + framework.Failf("failed to validate RBD pvc and application binding: %v", err) } // validate created backend rbd images validateRBDImageCount(f, 0, defaultRBDPool) @@ -1372,7 +1372,7 @@ var _ = Describe("RBD", func() { } err = validatePVCAndAppBinding(pvcPath, appPath, f) if err != nil { - framework.Failf("failed to validate CephFS pvc and application binding: %v", err) + framework.Failf("failed to validate RBD pvc and application binding: %v", err) } // validate created backend rbd images validateRBDImageCount(f, 0, defaultRBDPool) From 6fc3dfccabaaec8c7b884ab5a2b65ced0213a59d Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 2 Mar 2023 09:18:35 +0100 Subject: [PATCH 4/5] e2e: add test for `mkfsOptions` in RBD StorageClass Create a 1G volume, with ext4 and only 1024 inodes. Use `df` to check if the number of inodes is correct. Signed-off-by: Niels de Vos --- e2e/rbd.go | 39 ++++++++++++++++++++++++++++++++++++++ e2e/utils.go | 53 +++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/e2e/rbd.go b/e2e/rbd.go index 7f5a28d89d2..1a26cb8194d 100644 --- a/e2e/rbd.go +++ b/e2e/rbd.go @@ -1089,6 +1089,45 @@ var _ = Describe("RBD", func() { } }) + By("create a PVC and bind it to an app with ext4 as the FS and 1024 inodes ", func() { + err := deleteResource(rbdExamplePath + "storageclass.yaml") + if err != nil { + framework.Failf("failed to delete storageclass: %v", err) + } + err = createRBDStorageClass( + f.ClientSet, + f, + defaultSCName, + nil, + map[string]string{ + "csi.storage.k8s.io/fstype": "ext4", + "mkfsOptions": "-N1024", // 1024 inodes + }, + deletePolicy) + if err != nil { + framework.Failf("failed to create storageclass: %v", err) + } + err = validatePVCAndAppBinding(pvcPath, appPath, f) + if err != nil { + framework.Failf("failed to validate pvc and application binding: %v", err) + } + err = validateInodeCount(pvcPath, f, 1024) + if err != nil { + framework.Failf("failed to validate pvc and application binding: %v", err) + } + // validate created backend rbd images + validateRBDImageCount(f, 0, defaultRBDPool) + validateOmapCount(f, 0, rbdType, defaultRBDPool, volumesType) + err = deleteResource(rbdExamplePath + "storageclass.yaml") + if err != nil { + framework.Failf("failed to delete storageclass: %v", err) + } + err = createRBDStorageClass(f.ClientSet, f, defaultSCName, nil, nil, deletePolicy) + if err != nil { + framework.Failf("failed to create storageclass: %v", err) + } + }) + By("create a PVC and bind it to an app using rbd-nbd mounter", func() { if !testNBD { framework.Logf("skipping NBD test") diff --git a/e2e/utils.go b/e2e/utils.go index 9273223a959..7eaea6d0ae8 100644 --- a/e2e/utils.go +++ b/e2e/utils.go @@ -486,6 +486,7 @@ func validatePVCAndAppBinding(pvcPath, appPath string, f *framework.Framework) e if err != nil { return err } + err = deletePVCAndApp("", f, pvc, app) return err @@ -508,6 +509,50 @@ func getMountType(selector, mountPath string, f *framework.Framework) (string, e } func validateNormalUserPVCAccess(pvcPath string, f *framework.Framework) error { + writeTest := func(ns string, opts *metav1.ListOptions) error { + _, stdErr, err := execCommandInPod(f, "echo testing > /target/testing", ns, opts) + if err != nil { + return fmt.Errorf("failed to exec command in pod: %w", err) + } + if stdErr != "" { + return fmt.Errorf("failed to touch a file as non-root user %v", stdErr) + } + + return nil + } + + return validateNormalUserPVCAccessFunc(pvcPath, f, writeTest) +} + +func validateInodeCount(pvcPath string, f *framework.Framework, inodes int) error { + countInodes := func(ns string, opts *metav1.ListOptions) error { + stdOut, stdErr, err := execCommandInPod(f, "df --output=itotal /target | tail -n1", ns, opts) + if err != nil { + return fmt.Errorf("failed to exec command in pod: %w", err) + } + if stdErr != "" { + return fmt.Errorf("failed to list inodes in pod: %v", stdErr) + } + + itotal, err := strconv.Atoi(strings.TrimSpace(stdOut)) + if err != nil { + return fmt.Errorf("failed to parse itotal %q to int: %w", strings.TrimSpace(stdOut), err) + } + if inodes != itotal { + return fmt.Errorf("expected inodes (%d) do not match itotal on volume (%d)", inodes, itotal) + } + + return nil + } + + return validateNormalUserPVCAccessFunc(pvcPath, f, countInodes) +} + +func validateNormalUserPVCAccessFunc( + pvcPath string, + f *framework.Framework, + validate func(ns string, opts *metav1.ListOptions) error, +) error { pvc, err := loadPVC(pvcPath) if err != nil { return err @@ -571,12 +616,10 @@ func validateNormalUserPVCAccess(pvcPath string, f *framework.Framework) error { opt := metav1.ListOptions{ LabelSelector: "app=pod-run-as-non-root", } - _, stdErr, err := execCommandInPod(f, "echo testing > /target/testing", app.Namespace, &opt) + + err = validate(app.Namespace, &opt) if err != nil { - return fmt.Errorf("failed to exec command in pod: %w", err) - } - if stdErr != "" { - return fmt.Errorf("failed to touch a file as non-root user %v", stdErr) + return fmt.Errorf("failed to run validation function: %w", err) } // metrics for BlockMode was added in Kubernetes 1.22 From 4f19b9fde25d56958fa2628d93a07d62e07b6c38 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Fri, 3 Mar 2023 12:15:38 +0100 Subject: [PATCH 5/5] rbd: do not run mkfs on a BlockMode volume Signed-off-by: Niels de Vos --- internal/rbd/nodeserver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index 338b5791de4..4f7e3d70d0b 100644 --- a/internal/rbd/nodeserver.go +++ b/internal/rbd/nodeserver.go @@ -781,7 +781,7 @@ func (ns *NodeServer) mountVolumeToStagePath( readOnly = true } - if existingFormat == "" && !staticVol && !readOnly { + if existingFormat == "" && !staticVol && !readOnly && !isBlock { args := mkfsDefaultArgs[fsType] // if the VolumeContext contains "mkfsOptions", use those as args instead