[cinder-csi-plugin] Add mkfs-options StorageClass parameter by rybnico · Pull Request #3084 · kubernetes/cloud-provider-openstack

rybnico · 2026-03-17T12:59:10Z

What this PR does / why we need it:

Add support for a new mkfsOptions StorageClass parameter that allows cluster admins to pass custom mkfs flags when formatting volumes. This is particularly useful for Ceph-backed storage where passing -E nodiscard to mkfs.ext4 can dramatically speed up provisioning (e.g. from ~7 minutes to seconds for a 100 GB volume).

The parameter value is comma-separated, with each token further split on whitespace to produce individual mkfs command-line arguments. Example: mkfsOptions: "-E nodiscard,-O ^metadata_csum"

Implementation:

Extract mkfsOptions from StorageClass params in CreateVolume and pass via VolumeContext (both new-volume and idempotent early-return paths)
Parse mkfsOptions in NodeStageVolume using parseMkfsOptions() helper
Use FormatAndMountSensitiveWithFormatOptions() from k8s.io/mount-utils to pass format options separately from mount options
No behavior change when mkfsOptions is not set

Which issue this PR fixes(if applicable):
fixes #3068

Special notes for reviewers:
Inspired by the Ceph CSI implementation (ceph/ceph-csi#621).

Release note:

NONE

linux-foundation-easycla · 2026-03-17T12:59:19Z

The committers listed above are authorized under a signed CLA.

✅ login: rybnico / name: Nicolai Rybnikar (e6271a0)

k8s-ci-robot · 2026-03-17T12:59:21Z

Hi @rybnico. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2026-03-17T12:59:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign stephenfin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kayrus · 2026-04-10T13:44:14Z

/ok-to-test

kayrus · 2026-04-10T13:53:45Z

reading the https://github.com/kubernetes/utils/blob/28399d86e0b55e612b83ad94dc6bac8e5de2a5ac/mount/mount_linux.go#L396 source code it seems that this is not a mkfs option, but a mount option. Does it make sense to rename it?

rybnico · 2026-04-13T07:57:45Z

reading the https://github.com/kubernetes/utils/blob/28399d86e0b55e612b83ad94dc6bac8e5de2a5ac/mount/mount_linux.go#L396 source code it seems that this is not a mkfs option, but a mount option. Does it make sense to rename it?

No, it's an option for mkfs when formatting the device for the first time, not a mount option. We use the function FormatAndMountSensitiveWithFormatOptions from mount_linux.go and pass the mkfsOptions as the argument formatOptions. Since it's called mkfsOptions in Ceph CSI and other CSI controllers, I think it's the correct name for the field.

kayrus · 2026-04-13T08:24:21Z

No, it's an option for mkfs when formatting the device for the first time, not a mount option.

could you point me to the code that passes sensitiveOptions to mkfs? I've been looking but can't find it. from what I can see, this list is only used for mount options.

rybnico · 2026-04-13T08:53:47Z

No, it's an option for mkfs when formatting the device for the first time, not a mount option.

could you point me to the code that passes sensitiveOptions to mkfs? I've been looking but can't find it. from what I can see, this list is only used for mount options.

Of course - but we don't pass them as argument sensitiveOptions but rather formatOptions.

We create formatOptions from mkfsOptions here
We pass this as a new argument formatOptions to the formatAndMountRetry function here

In formatAndMountRetry, the formatOptions are passed to FormatAndMountSensitiveWithFormatOptions here and here.

In mount-utils, the options is finally passed to formatAndMountSensitive and added to the args in the call to mounter.format

kayrus · 2026-04-13T10:28:38Z

+// parseMkfsOptions splits a comma-separated mkfsOptions string into individual
+// command-line arguments. Each comma-separated token is further split on
+// whitespace. Empty tokens are filtered out.
+func parseMkfsOptions(mkfsOpts string) []string {


you can use

cloud-provider-openstack/pkg/util/util.go

Lines 175 to 182 in 9c80105

// SplitTrim splits a string of values separated by sep rune into a slice of

// strings with trimmed spaces.

func SplitTrim(s string, sep rune) []string {

f := func(c rune) bool {

return unicode.IsSpace(c) || c == sep

}

return strings.FieldsFunc(s, f)

}

Good catch! I removed the parseMkfsOptions function and use SplitTrim instead.

kayrus · 2026-04-13T10:28:41Z

Thanks, got it. I mixed up sensitiveOptions and formatOptions. They were hidden in github UI, and I missed the formatOptions:

Next time I'll try to use a proper code viewer :)

kayrus · 2026-04-13T11:22:01Z

 |-------------------------   |-----------------------|-----------------|-----------------|
 | StorageClass `parameters`  | `availability`          | `nova`          | String. Volume Availability Zone |
 | StorageClass `parameters`  | `type`                  | Empty String    | String. Name/ID of Volume type. Corresponding volume type should exist in cinder     |
+| StorageClass `parameters`  | `mkfsOptions`           | Empty String    | String. Comma-separated mkfs options passed to mkfs when formatting the volume. Useful for passing `-E nodiscard` on Ceph-backed storage to speed up provisioning. |


it makes sense to use kebab-case style here

Yes, you're right. It isn't consistent across CSI implementations. However, looking at the other parameters in Cinder CSI, kebab-style is a better fit.
I also removed the overly specific comment about Ceph/nodiscard.

kayrus · 2026-04-13T12:41:05Z

pkg/csi/cinder/nodeserver.go:29:1: File is not properly formatted (gofmt)
"google.golang.org/grpc/status"
^
1 issues:

gofmt: 1
exit status 1
make: *** [Makefile:84: check] Error 1

can you fix formatting? and rename the PR title

rybnico · 2026-04-13T12:43:44Z

pkg/csi/cinder/nodeserver.go:29:1: File is not properly formatted (gofmt)
"google.golang.org/grpc/status"
^
1 issues:

gofmt: 1
exit status 1
make: *** [Makefile:84: check] Error 1

can you fix formatting? and rename the PR title

I fixed it. I don't know why it didn't get caught locally. I also changed the PR title.

kayrus · 2026-04-13T12:44:01Z

+			},
+		},
+		Parameters: map[string]string{
+			"mkfs-options": "-E nodiscard",


I think it also makes sense to add tests for multiple format options, separated with comma.

This was a very good suggestion because it made me aware of a bug that occurred when using multiple arguments. Since it doesn't make sense to split the arguments by comma, I now use the strings.Fields function to split them by space.
I added tests for different/multiple arguments in mkfs-options and I made mocking commands in pkg/util/mount/mount_mock.go possible to check if the correct arguments are used in the mkfs call.

I appreciate your thorough review!

hm. it seems we need something smarter to avoid cases like this: https://go.dev/play/p/fp3YAphLD8m

Fields are: ["-L" "\"my" "data" "disk\"" "/dev/sdb1"]

see kubernetes-sigs/kustomize#5943

I noticed that, too. However, I was hesitant to add a dependency on shlex.
As far as I know, the label argument is the only one that takes a quoted string as a parameter. And the label argument doesn't make sense in our context anyway.
If you'd like, though, I can improve the parsing. Would you prefer to use shlex or copy kustomization's shlex implementation?
Another option is to keep it as it is and add a note to the documentation stating that quoted strings are not supported.
What do you prefer?

@kayrus Could you take another look and let me know how we should proceed? Thanks in advance!

@zetaab @stephenfin can you take a look at this PR?

@kayrus @zetaab @stephenfin Would you mind having another look and telling me if the argument parsing needs additional work for this PR to get merged? Thanks!

I was just thinking could it work in similar way than pod command in kubernetes ([]string)? By that way people could easily understand the syntax if its more complex than just this in test.

First, thanks for your review!
Using a string array for the options would be great, but the parameters field in the StorageClass is of type map[string]string.

Add support for a new `mkfs-options` StorageClass parameter that allows cluster admins to pass custom mkfs flags when formatting volumes. For example, this is useful for Ceph-backed storage where passing `-E nodiscard` to mkfs.ext4 can dramatically speed up provisioning. Implementation: - Extract mkfs-options from StorageClass params in CreateVolume and pass via VolumeContext (both new-volume and idempotent early-return paths) - Parse mkfs-options in NodeStageVolume - Use FormatAndMountSensitiveWithFormatOptions() from k8s.io/mount-utils to pass format options separately from mount options - No behavior change when mkfs-options is not set Inspired by the Ceph CSI implementation (ceph/ceph-csi#621). MountMock.Mounter() now returns a stable, memoized SafeFormatAndMount with a SetMounter escape hatch, enabling tests to script and capture the mkfs argv via FakeExec.

zetaab

/lgtm

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 17, 2026

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 17, 2026

k8s-ci-robot requested review from stephenfin and zetaab March 17, 2026 12:59

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 17, 2026

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 17, 2026

rybnico force-pushed the feature/add-mkfs-options branch from 5c03378 to d7fa2d8 Compare March 25, 2026 15:26

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 25, 2026

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 10, 2026

rybnico force-pushed the feature/add-mkfs-options branch from d7fa2d8 to 715349b Compare April 13, 2026 07:49

kayrus reviewed Apr 13, 2026

View reviewed changes

rybnico force-pushed the feature/add-mkfs-options branch from 715349b to cbc2205 Compare April 13, 2026 12:24

rybnico force-pushed the feature/add-mkfs-options branch from cbc2205 to 5b221ba Compare April 13, 2026 12:42

rybnico changed the title ~~[cinder-csi-plugin] Add mkfsOptions StorageClass parameter~~ [cinder-csi-plugin] Add mkfs-options StorageClass parameter Apr 13, 2026

kayrus reviewed Apr 13, 2026

View reviewed changes

rybnico force-pushed the feature/add-mkfs-options branch from 5b221ba to e6271a0 Compare April 16, 2026 08:20

zetaab approved these changes May 21, 2026

View reviewed changes

k8s-ci-robot assigned zetaab May 21, 2026

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2026

	// SplitTrim splits a string of values separated by sep rune into a slice of
	// strings with trimmed spaces.
	func SplitTrim(s string, sep rune) []string {
	f := func(c rune) bool {
	return unicode.IsSpace(c) \|\| c == sep
	}
	return strings.FieldsFunc(s, f)
	}

Conversation

rybnico commented Mar 17, 2026

Uh oh!

linux-foundation-easycla Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 17, 2026

Uh oh!

k8s-ci-robot commented Mar 17, 2026

Uh oh!

kayrus commented Apr 10, 2026

Uh oh!

kayrus commented Apr 10, 2026

Uh oh!

rybnico commented Apr 13, 2026

Uh oh!

kayrus commented Apr 13, 2026

Uh oh!

rybnico commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayrus commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayrus commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rybnico commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zetaab left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla Bot commented Mar 17, 2026 •

edited

Loading

kayrus commented Apr 13, 2026 •

edited

Loading