Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rbd: add mkfsOptions to the StorageClass and pass them to mkfs #3692

Merged
merged 5 commits into from
Mar 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/deploy-rbd.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://docs.ceph.com/en/latest/rbd/rbd-config-ref/#image-features> 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. |
Expand Down
45 changes: 42 additions & 3 deletions e2e/rbd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nixpanic can we have a test case for XFS FS as well ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think there is much value in that. XFS is used in other tests already, so this PR does not break it.

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")
Expand Down Expand Up @@ -1207,7 +1246,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)
Expand Down Expand Up @@ -1372,7 +1411,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)
Expand Down
53 changes: 48 additions & 5 deletions e2e/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions examples/rbd/storageclass.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
49 changes: 34 additions & 15 deletions internal/rbd/nodeserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having some references to these options and why we enable them as default options could help

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, but I don't know where they come from. This PR just moves them around...

"xfs": {"-K"},
}

mountDefaultOpts = map[string][]string{
"xfs": {"nouuid"},
}
)

// parseBoolOption checks if parameters contain option and parse it. If it is
Expand Down Expand Up @@ -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"
Expand All @@ -771,34 +781,43 @@ func (ns *NodeServer) mountVolumeToStagePath(
readOnly = true
}

if fsType == "xfs" {
opt = append(opt, "nouuid")
}
if existingFormat == "" && !staticVol && !readOnly && !isBlock {
args := mkfsDefaultArgs[fsType]

if existingFormat == "" && !staticVol && !readOnly {
args := []string{}
// 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 {
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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"-K" also needs to move to newly added mkfsDefultArgs ?

mkfsDefaultArgs = map[string][]string{
		"ext4": {"-m0", "-Enodiscard,lazy_itable_init=1,lazy_journal_init=1"},
		"xfs": {"-K"},
	}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good catch!

// 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
}
}

Expand Down