diff --git a/docs/cinder-csi-plugin/using-cinder-csi-plugin.md b/docs/cinder-csi-plugin/using-cinder-csi-plugin.md index 643a95126e..86ff46d071 100644 --- a/docs/cinder-csi-plugin/using-cinder-csi-plugin.md +++ b/docs/cinder-csi-plugin/using-cinder-csi-plugin.md @@ -283,6 +283,7 @@ helm install --namespace kube-system --name cinder-csi ./charts/cinder-csi-plugi |------------------------- |-----------------------|-----------------|-----------------| | 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` | `mkfs-options` | Empty String | String. Whitespace-separated arguments passed to mkfs when formatting the volume. | | VolumeSnapshotClass `parameters` | `force-create` | `false` | Enable to support creating snapshot for a volume in in-use status | | VolumeSnapshotClass `parameters` | `type` | Empty String | `snapshot` creates a VolumeSnapshot object linked to a Cinder volume snapshot. `backup` creates a VolumeSnapshot object linked to a cinder volume backup. Defaults to `snapshot` if not defined | | VolumeSnapshotClass `parameters` | `backup-max-duration-seconds-per-gb` | `20` | Defines the amount of time to wait for a backup to complete in seconds per GB of volume size | diff --git a/pkg/csi/cinder/controllerserver.go b/pkg/csi/cinder/controllerserver.go index 1a1b9e2ff6..abfc42c00a 100644 --- a/pkg/csi/cinder/controllerserver.go +++ b/pkg/csi/cinder/controllerserver.go @@ -119,7 +119,11 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol } klog.V(4).Infof("Volume %s already exists in Availability Zone: %s of size %d GiB", vols[0].ID, vols[0].AvailabilityZone, vols[0].Size) accessibleTopology := getTopology(&vols[0], accessibleTopologyReq, cs.Driver.withTopology, ignoreVolumeAZ) - return getCreateVolumeResponse(&vols[0], nil, accessibleTopology), nil + existingVolCtx := map[string]string{} + if mkfsOptions := volParams["mkfs-options"]; mkfsOptions != "" { + existingVolCtx["mkfs-options"] = mkfsOptions + } + return getCreateVolumeResponse(&vols[0], existingVolCtx, accessibleTopology), nil } if len(vols) > 1 { @@ -233,6 +237,11 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol klog.V(4).Infof("CreateVolume: Resolved scheduler hints: affinity=%s, anti-affinity=%s", affinity, antiAffinity) } + // Pass mkfsOptions to node via volume context + if mkfsOptions := volParams["mkfs-options"]; mkfsOptions != "" { + volCtx["mkfs-options"] = mkfsOptions + } + vol, err := cloud.CreateVolume(ctx, opts, schedulerHints) if err != nil { klog.Errorf("Failed to CreateVolume: %v", err) diff --git a/pkg/csi/cinder/controllerserver_test.go b/pkg/csi/cinder/controllerserver_test.go index df608d1b2c..42c1b54d5a 100644 --- a/pkg/csi/cinder/controllerserver_test.go +++ b/pkg/csi/cinder/controllerserver_test.go @@ -200,6 +200,94 @@ func TestCreateVolumeWithParam(t *testing.T) { assert.Equal(FakeAvailability, actualRes.Volume.AccessibleTopology[0].GetSegments()[topologyKey]) } +// Test CreateVolume passes mkfs-options to VolumeContext +func TestCreateVolumeWithMkfsOptions(t *testing.T) { + fakeCs, osmock := fakeControllerServer() + + // mock OpenStack + properties := map[string]string{cinderCSIClusterIDKey: FakeCluster} + osmock.On("CreateVolume", FakeVolName, mock.AnythingOfType("int"), FakeVolType, FakeAvailability, "", "", "", properties).Return(&FakeVol, nil) + osmock.On("GetVolumesByName", FakeVolName).Return(FakeVolListEmpty, nil) + osmock.On("GetBlockStorageOpts").Return(openstack.BlockStorageOpts{}) + + assert := assert.New(t) + + // Fake request with mkfs-options + fakeReq := &csi.CreateVolumeRequest{ + Name: FakeVolName, + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + }, + Parameters: map[string]string{ + "mkfs-options": "-E nodiscard", + }, + AccessibilityRequirements: &csi.TopologyRequirement{ + Requisite: []*csi.Topology{ + { + Segments: map[string]string{topologyKey: FakeAvailability}, + }, + }, + }, + } + + // Invoke CreateVolume + actualRes, err := fakeCs.CreateVolume(FakeCtx, fakeReq) + if err != nil { + t.Errorf("failed to CreateVolume: %v", err) + } + + // Assert mkfs-options is in VolumeContext + assert.NotNil(actualRes.Volume) + assert.Equal("-E nodiscard", actualRes.Volume.VolumeContext["mkfs-options"]) +} + +// Test CreateVolume passes mkfs-options on idempotent path (volume already exists) +func TestCreateVolumeExistingWithMkfsOptions(t *testing.T) { + fakeCs, osmock := fakeControllerServer() + + // mock OpenStack - volume already exists + osmock.On("GetVolumesByName", FakeVolName).Return(FakeVolList, nil) + osmock.On("GetBlockStorageOpts").Return(openstack.BlockStorageOpts{}) + + assert := assert.New(t) + + // Fake request with mkfs-options + fakeReq := &csi.CreateVolumeRequest{ + Name: FakeVolName, + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + }, + Parameters: map[string]string{ + "mkfs-options": "-E nodiscard", + }, + AccessibilityRequirements: &csi.TopologyRequirement{ + Requisite: []*csi.Topology{ + { + Segments: map[string]string{topologyKey: FakeAvailability}, + }, + }, + }, + } + + // Invoke CreateVolume + actualRes, err := fakeCs.CreateVolume(FakeCtx, fakeReq) + if err != nil { + t.Errorf("failed to CreateVolume: %v", err) + } + + // Assert mkfs-options is in VolumeContext even on idempotent path + assert.NotNil(actualRes.Volume) + assert.Equal("-E nodiscard", actualRes.Volume.VolumeContext["mkfs-options"]) +} + // Test CreateVolume with ignore-volume-az option func TestCreateVolumeWithIgnoreVolumeAZ(t *testing.T) { fakeCs, osmock := fakeControllerServer() diff --git a/pkg/csi/cinder/nodeserver.go b/pkg/csi/cinder/nodeserver.go index ee91ae0c15..70280752f9 100644 --- a/pkg/csi/cinder/nodeserver.go +++ b/pkg/csi/cinder/nodeserver.go @@ -219,8 +219,16 @@ func (ns *nodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol mountFlags := mnt.GetMountFlags() options = append(options, collectMountOptions(fsType, mountFlags)...) } + + // Parse mkfs options from volume context + var formatOptions []string + if mkfsOpts, ok := volumeContext["mkfs-options"]; ok && mkfsOpts != "" { + formatOptions = strings.Fields(mkfsOpts) + klog.V(4).Infof("NodeStageVolume: mkfs-options for volume %s: %v", volumeID, formatOptions) + } + // Mount - err = ns.formatAndMountRetry(devicePath, stagingTarget, fsType, options) + err = ns.formatAndMountRetry(devicePath, stagingTarget, fsType, options, formatOptions) if err != nil { return nil, status.Error(codes.Internal, err.Error()) } @@ -248,9 +256,9 @@ func (ns *nodeServer) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol // formatAndMountRetry attempts to format and mount a device at the given path. // If the initial mount fails, it rescans the device and retries the mount operation. -func (ns *nodeServer) formatAndMountRetry(devicePath, stagingTarget, fsType string, options []string) error { +func (ns *nodeServer) formatAndMountRetry(devicePath, stagingTarget, fsType string, options []string, formatOptions []string) error { m := ns.Mount - err := m.Mounter().FormatAndMount(devicePath, stagingTarget, fsType, options) + err := m.Mounter().FormatAndMountSensitiveWithFormatOptions(devicePath, stagingTarget, fsType, options, nil, formatOptions) if err != nil { klog.Infof("Initial format and mount failed: %v. Attempting rescan.", err) // Attempting rescan if the initial mount fails @@ -260,7 +268,7 @@ func (ns *nodeServer) formatAndMountRetry(devicePath, stagingTarget, fsType stri return err } klog.Infof("Rescan succeeded, retrying format and mount") - err = m.Mounter().FormatAndMount(devicePath, stagingTarget, fsType, options) + err = m.Mounter().FormatAndMountSensitiveWithFormatOptions(devicePath, stagingTarget, fsType, options, nil, formatOptions) } return err } diff --git a/pkg/csi/cinder/nodeserver_test.go b/pkg/csi/cinder/nodeserver_test.go index 8e8713e05d..750fca45b4 100644 --- a/pkg/csi/cinder/nodeserver_test.go +++ b/pkg/csi/cinder/nodeserver_test.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "testing" "github.com/container-storage-interface/spec/lib/go/csi" @@ -30,6 +31,9 @@ import ( "k8s.io/cloud-provider-openstack/pkg/csi/cinder/openstack" "k8s.io/cloud-provider-openstack/pkg/util/metadata" "k8s.io/cloud-provider-openstack/pkg/util/mount" + mountutil "k8s.io/mount-utils" + utilsexec "k8s.io/utils/exec" + testingexec "k8s.io/utils/exec/testing" ) func fakeNodeServer() (*nodeServer, *openstack.OpenStackMock, *mount.MountMock, *metadata.MetadataMock) { @@ -236,6 +240,134 @@ func TestNodeStageVolumeBlock(t *testing.T) { assert.Equal(expectedRes, actualRes) } +// TestNodeStageVolume_MkfsOptions_ArgvCapture drives NodeStageVolume with a +// range of mkfs-options values and asserts that the argv handed to mkfs +// starts with the user-supplied arguments. +func TestNodeStageVolume_MkfsOptions_ArgvCapture(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skipf("mkfs argv capture only runs on linux (GOOS=%s)", runtime.GOOS) + } + + stdMountVolCap := &csi.VolumeCapability{ + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + } + + cases := []struct { + name string + mkfsOptions string + expectedArguments []string + }{ + { + name: "no options", + mkfsOptions: "", + expectedArguments: []string{}, + }, + { + name: "single flag", + mkfsOptions: "-j", + expectedArguments: []string{"-j"}, + }, + { + name: "flag and value", + mkfsOptions: "-E nodiscard", + expectedArguments: []string{"-E", "nodiscard"}, + }, + { + name: "multiple options", + mkfsOptions: "-E nodiscard -j", + expectedArguments: []string{"-E", "nodiscard", "-j"}, + }, + { + // Demonstrates the bug in util.SplitTrim: it also splits on + // whitespace, so the comma-containing value was shredded into + // two separate mkfs args and treated by mkfs as a block count. + name: "commas inside option value", + mkfsOptions: "-E lazy_itable_init=0,lazy_journal_init=0", + expectedArguments: []string{"-E", "lazy_itable_init=0,lazy_journal_init=0"}, + }, + { + name: "extra whitespace", + mkfsOptions: " -j -F ", + expectedArguments: []string{"-j", "-F"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fakeNs, omock, mmock, _ := fakeNodeServer() + + mmock.On("GetDevicePath", FakeVolID).Return(FakeDevicePath, nil) + mmock.On("IsLikelyNotMountPointAttach", FakeStagingTargetPath).Return(true, nil) + omock.On("GetVolume", FakeVolID).Return(FakeVol, nil) + + // Script exec: + // 1. blkid → exit 2 (device is unformatted) to trigger the mkfs path. + // 2. mkfs.ext4 → succeed, and capture the argv it was called with. + var capturedMkfsArgs []string + + fakeExec := &testingexec.FakeExec{} + + blkidCmd := &testingexec.FakeCmd{ + CombinedOutputScript: []testingexec.FakeAction{ + func() ([]byte, []byte, error) { + return nil, nil, &testingexec.FakeExitError{Status: 2} + }, + }, + } + fakeExec.CommandScript = append(fakeExec.CommandScript, + func(cmd string, args ...string) utilsexec.Cmd { + return testingexec.InitFakeCmd(blkidCmd, cmd, args...) + }, + ) + + mkfsCmd := &testingexec.FakeCmd{ + CombinedOutputScript: []testingexec.FakeAction{ + func() ([]byte, []byte, error) { return nil, nil, nil }, + }, + } + fakeExec.CommandScript = append(fakeExec.CommandScript, + func(cmd string, args ...string) utilsexec.Cmd { + capturedMkfsArgs = append([]string(nil), args...) + return testingexec.InitFakeCmd(mkfsCmd, cmd, args...) + }, + ) + + mmock.SetMounter(&mountutil.SafeFormatAndMount{ + Interface: mount.NewFakeMounter(), + Exec: fakeExec, + }) + + volCtx := map[string]string{} + if tc.mkfsOptions != "" { + volCtx["mkfs-options"] = tc.mkfsOptions + } + + fakeReq := &csi.NodeStageVolumeRequest{ + VolumeId: FakeVolID, + PublishContext: map[string]string{"DevicePath": FakeDevicePath}, + StagingTargetPath: FakeStagingTargetPath, + VolumeCapability: stdMountVolCap, + VolumeContext: volCtx, + } + + _, err := fakeNs.NodeStageVolume(FakeCtx, fakeReq) + assert.NoError(t, err) + + // mount-utils constructs mkfs argv as: formatOptions ++ ["-F", "-m0", devicePath]. + // Our user-supplied options therefore occupy the argv prefix. + assert.GreaterOrEqual(t, len(capturedMkfsArgs), len(tc.expectedArguments), + "captured argv = %v", capturedMkfsArgs) + assert.Equal(t, tc.expectedArguments, capturedMkfsArgs[:len(tc.expectedArguments)], + "mkfs argv prefix mismatch; full argv = %v", capturedMkfsArgs) + }) + } +} + // Test NodeUnpublishVolume func TestNodeUnpublishVolume(t *testing.T) { fakeNs, omock, mmock, _ := fakeNodeServer() @@ -394,5 +526,4 @@ func TestNodeGetVolumeStatsFs(t *testing.T) { assert.NoError(err) assert.Equal(expectedFsRes, fsRes) - } diff --git a/pkg/util/mount/mount_mock.go b/pkg/util/mount/mount_mock.go index 81514a962a..8a1442de4e 100644 --- a/pkg/util/mount/mount_mock.go +++ b/pkg/util/mount/mount_mock.go @@ -28,10 +28,22 @@ import ( // ORIGINALLY GENERATED BY mockery with hand edits type MountMock struct { mock.Mock + + // safeMounter is lazily constructed on first Mounter() call and returned + // unchanged on subsequent calls, so tests can inspect or pre-configure + // the associated FakeExec. Tests that need a custom exec script should + // call SetMounter before NodeStageVolume runs. + safeMounter *mount.SafeFormatAndMount } // revive:enable:exported +// SetMounter injects a caller-built SafeFormatAndMount. Useful in tests that +// want to script or capture exec invocations via a custom FakeExec. +func (_m *MountMock) SetMounter(sfm *mount.SafeFormatAndMount) { + _m.safeMounter = sfm +} + func (_m *MountMock) GetDeviceStats(path string) (*DeviceStats, error) { ret := _m.Called(path) @@ -146,6 +158,10 @@ func (_m *MountMock) UnmountPath(mountPath string) error { // GetBaseMounter provides a mock function func (_m *MountMock) Mounter() *mount.SafeFormatAndMount { + if _m.safeMounter != nil { + return _m.safeMounter + } + scripts := []struct { cmd string output []byte @@ -177,10 +193,11 @@ func (_m *MountMock) Mounter() *mount.SafeFormatAndMount { fakeexec.CommandScript = append(fakeexec.CommandScript, cmdAction) } - return &mount.SafeFormatAndMount{ + _m.safeMounter = &mount.SafeFormatAndMount{ Interface: NewFakeMounter(), Exec: fakeexec, } + return _m.safeMounter } func (_m *MountMock) MakeDir(pathname string) error {