CephFS: kernel and fuse mount options per cluster #4245

iPraveenParihar · 2023-11-08T05:36:00Z

Describe what this PR does

Implemented the capability to include kernel and fuse mount options
for individual clusters within the ceph-csi-config ConfigMap.
This allows users to configure the kernel and fuse mount options
for each cluster separately. The mount options specified in the ConfigMap
will supersede those provided via command line arguments.

Is there anything that requires special attention

Is the change backward compatible?
Yes

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

Rakshith-R · 2023-11-08T09:47:08Z

internal/cephfs/nodeserver.go

+
+	switch mnt.(type) {
+	case *mounter.FuseMounter:
+		getMountOptionsFunc = util.GetCephFSFuseMountOptions
+		CLIMountOptions = ns.fuseMountOptions
+	case *mounter.KernelMounter:
+		getMountOptionsFunc = util.GetCephFSKernelMountOptions
+		CLIMountOptions = ns.kernelMountOptions
+	}
+
+	mountOptions, err := getMountOptionsFunc(util.CsiConfigFile, volOptions.ClusterID)
+	if err != nil {
+		return "", err
+	}
+
+	if mountOptions != "" {
+		return mountOptions, nil
+	}


use a single function to fetch both kernel and fuse mount options and later decide which one to pass.

Suggested change

switch mnt.(type) {

case *mounter.FuseMounter:

getMountOptionsFunc = util.GetCephFSFuseMountOptions

CLIMountOptions = ns.fuseMountOptions

case *mounter.KernelMounter:

getMountOptionsFunc = util.GetCephFSKernelMountOptions

CLIMountOptions = ns.kernelMountOptions

}

mountOptions, err := getMountOptionsFunc(util.CsiConfigFile, volOptions.ClusterID)

if err != nil {

return "", err

}

if mountOptions != "" {

return mountOptions, nil

}

kernelMountOptions, fuseMountOptions, err := getMountOptions(util.CsiConfigFile, volOptions.ClusterID)

if err != nil {

return "", err

}

case *mounter.FuseMounter:

mountOptions = fuseMountOptions

CLIMountOptions = ns.fuseMountOptions

case *mounter.KernelMounter:

mountOptions = kernelMountOptions

CLIMountOptions = ns.kernelMountOptions

}

if mountOptions != "" {

return mountOptions, nil

}

Rakshith-R · 2023-11-08T09:48:01Z

internal/cephfs/nodeserver.go

@@ -316,12 +316,19 @@ func (ns *NodeServer) mount(
 		mountOptions = m.GetMountFlags()
 	}

+	mntOptions, err := ns.getMountOptions(mnt, volOptions)


maybe name it configuredMountOptions or something and add a label where these options come from ?

Rakshith-R · 2023-11-08T09:49:58Z

internal/cephfs/nodeserver.go

@@ -316,12 +316,19 @@ func (ns *NodeServer) mount(
 		mountOptions = m.GetMountFlags()
 	}


I think its better to move the entire flow to obtain mountOptions from line 314 above to a helper function + unit tests.

mergify · 2023-11-08T21:18:28Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

Rakshith-R · 2023-11-14T06:52:25Z

internal/util/csiconfig.go

@@ -48,6 +48,10 @@ type ClusterInfo struct {
 		NetNamespaceFilePath string `json:"netNamespaceFilePath"`
 		// SubvolumeGroup contains the name of the SubvolumeGroup for CSI volumes
 		SubvolumeGroup string `json:"subvolumeGroup"`
+		// KernelMountOptions contains the kernel mount options for CephFS volumes


Extract the entire CephFS struct into a independent type.
That should help writing unit test cases instead of defining the type each time.

Rakshith-R · 2023-11-14T06:53:08Z

internal/util/csiconfig_test.go

+				KernelMountOptions   string `json:"kernelMountOptions"`
+				FuseMountOptions     string `json:"fuseMountOptions"`
+			}{
+				KernelMountOptions: "crc",


Mention the input along the output in the same struct or create a sub struct for it.

We require the separate definition of 'csiConfig' input since it will be written to a file. It will be complex to combine both input and output together.

Rakshith-R · 2023-11-14T06:54:35Z

internal/cephfs/nodeserver.go

-	if m := volCap.GetMount(); m != nil {
-		mountOptions = m.GetMountFlags()
-	}
+	err = ns.getMountOptions(mnt, volOptions, volCap)


I think setMountOptions is better suited.
Please include the below "ro" detection in the function too.

internal/cephfs/nodeserver.go

internal/util/csiconfig_test.go

iPraveenParihar · 2023-11-14T16:26:33Z

/test ci/centos/mini-e2e/k8s-1.28

PendingReleaseNotes.md

deploy/csi-config-map-sample.yaml

Pull request has been modified.

Madhu-1

LGTM, can you please check that we don't have any problem for the static PVC where clusterID might not be set but monitor IP's are passed?

deploy/csi-config-map-sample.yaml

iPraveenParihar · 2023-11-15T09:00:16Z

LGTM, can you please check that we don't have any problem for the static PVC where clusterID might not be set but monitor IP's are passed?

Added a check for empty ClusterID.

Madhu-1

LGTM

Rakshith-R · 2023-11-15T09:12:05Z

@Mergifyio rebase

This commit adds GetCephFSMountOptions util method which returns KernelMountOptions and fuseMountOptions for cluster `clusterID`. Signed-off-by: Praveen M <m.praveen@ibm.com>

Implemented the capability to include kernel mount options and fuse mount options for individual clusters within the ceph-csi-config ConfigMap.This allows users to configure the kernel/fuse mount options for each cluster separately. The mount options specified in the ConfigMap will supersede those provided via command line arguments. Signed-off-by: Praveen M <m.praveen@ibm.com>

Signed-off-by: Praveen M <m.praveen@ibm.com>

mergify · 2023-11-15T09:12:16Z

rebase

✅ Branch has been successfully rebased

Rakshith-R · 2023-11-15T09:52:10Z

@Mergifyio queue

mergify · 2023-11-15T09:52:14Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 4ee466b

ceph-csi-bot · 2023-11-15T09:52:35Z

/test ci/centos/k8s-e2e-external-storage/1.27

ceph-csi-bot · 2023-11-15T09:52:36Z

/test ci/centos/k8s-e2e-external-storage/1.28

ceph-csi-bot · 2023-11-15T09:52:36Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2023-11-15T09:52:36Z

/test ci/centos/k8s-e2e-external-storage/1.26

ceph-csi-bot · 2023-11-15T09:52:36Z

/test ci/centos/mini-e2e-helm/k8s-1.28

ceph-csi-bot · 2023-11-15T09:52:37Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2023-11-15T09:52:37Z

/test ci/centos/mini-e2e-helm/k8s-1.26

ceph-csi-bot · 2023-11-15T09:52:37Z

/test ci/centos/mini-e2e/k8s-1.28

ceph-csi-bot · 2023-11-15T09:52:37Z

/test ci/centos/mini-e2e-helm/k8s-1.27

ceph-csi-bot · 2023-11-15T09:52:38Z

/test ci/centos/mini-e2e/k8s-1.26

ceph-csi-bot · 2023-11-15T09:52:38Z

/test ci/centos/mini-e2e/k8s-1.27

iPraveenParihar changed the title ~~Feature/kernel fuse mount options per cluster~~ CephFS: kernel and fuse mount options per cluster Nov 8, 2023

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch 2 times, most recently from 12538e0 to 9a66115 Compare November 8, 2023 06:15

iPraveenParihar marked this pull request as ready for review November 8, 2023 06:16

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch from 9a66115 to a43e630 Compare November 8, 2023 06:27

Rakshith-R reviewed Nov 8, 2023

View reviewed changes

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch 2 times, most recently from 7457046 to e22b23f Compare November 8, 2023 14:50

iPraveenParihar requested a review from Rakshith-R November 9, 2023 07:19

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch from e22b23f to 3fafd2e Compare November 14, 2023 06:50

Rakshith-R requested changes Nov 14, 2023

View reviewed changes

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch from 3fafd2e to 5367aaa Compare November 14, 2023 08:26

iPraveenParihar requested a review from Rakshith-R November 14, 2023 08:33

Madhu-1 reviewed Nov 14, 2023

View reviewed changes

internal/cephfs/nodeserver.go Outdated Show resolved Hide resolved

internal/cephfs/nodeserver.go Outdated Show resolved Hide resolved

internal/cephfs/nodeserver.go Show resolved Hide resolved

internal/util/csiconfig_test.go Outdated Show resolved Hide resolved

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch 3 times, most recently from fbda7a8 to 59da859 Compare November 14, 2023 15:09

iPraveenParihar requested a review from Madhu-1 November 15, 2023 04:01

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch from 59da859 to a56a8b7 Compare November 15, 2023 05:18

Rakshith-R previously approved these changes Nov 15, 2023

View reviewed changes

Madhu-1 requested changes Nov 15, 2023

View reviewed changes

PendingReleaseNotes.md Outdated Show resolved Hide resolved

deploy/csi-config-map-sample.yaml Outdated Show resolved Hide resolved

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch from a56a8b7 to 4c6063f Compare November 15, 2023 08:22

iPraveenParihar requested a review from Rakshith-R November 15, 2023 08:22

iPraveenParihar requested a review from Madhu-1 November 15, 2023 08:22

Madhu-1 reviewed Nov 15, 2023

View reviewed changes

deploy/csi-config-map-sample.yaml Outdated Show resolved Hide resolved

iPraveenParihar force-pushed the feature/kernel-fuse-mount-options-per-cluster branch from 4c6063f to 35ca751 Compare November 15, 2023 08:54

iPraveenParihar requested a review from Madhu-1 November 15, 2023 09:00

Madhu-1 approved these changes Nov 15, 2023

View reviewed changes

Rakshith-R approved these changes Nov 15, 2023

View reviewed changes

iPraveenParihar added 3 commits November 15, 2023 09:12

util: added GetCephFSMountOptions method

8b27f01

This commit adds GetCephFSMountOptions util method which returns KernelMountOptions and fuseMountOptions for cluster `clusterID`. Signed-off-by: Praveen M <m.praveen@ibm.com>

doc: updated doc for kernel/fuse mount options

3e60a29

Signed-off-by: Praveen M <m.praveen@ibm.com>

Rakshith-R force-pushed the feature/kernel-fuse-mount-options-per-cluster branch from 35ca751 to 3e60a29 Compare November 15, 2023 09:12

mergify bot added the ok-to-test Label to trigger E2E tests label Nov 15, 2023

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Nov 15, 2023

mergify bot merged commit 4ee466b into ceph:devel Nov 15, 2023
34 checks passed

iPraveenParihar mentioned this pull request Nov 15, 2023

provide way to apply configuration to specific ceph clusters. #3649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CephFS: kernel and fuse mount options per cluster #4245

CephFS: kernel and fuse mount options per cluster #4245

iPraveenParihar commented Nov 8, 2023 •

edited

Loading

Rakshith-R Nov 8, 2023

Rakshith-R Nov 8, 2023

Rakshith-R Nov 8, 2023

mergify bot commented Nov 8, 2023

Rakshith-R Nov 14, 2023

iPraveenParihar Nov 14, 2023

Rakshith-R Nov 14, 2023

iPraveenParihar Nov 14, 2023

Rakshith-R Nov 14, 2023

iPraveenParihar Nov 14, 2023

iPraveenParihar commented Nov 14, 2023

Madhu-1 left a comment

iPraveenParihar commented Nov 15, 2023

Madhu-1 left a comment

Rakshith-R commented Nov 15, 2023

mergify bot commented Nov 15, 2023

Rakshith-R commented Nov 15, 2023

mergify bot commented Nov 15, 2023 •

edited

Loading

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

		@@ -316,12 +316,19 @@ func (ns *NodeServer) mount(
		mountOptions = m.GetMountFlags()
		}

CephFS: kernel and fuse mount options per cluster #4245

CephFS: kernel and fuse mount options per cluster #4245

Conversation

iPraveenParihar commented Nov 8, 2023 • edited Loading

Describe what this PR does

Is there anything that requires special attention

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iPraveenParihar commented Nov 14, 2023

Madhu-1 left a comment

Choose a reason for hiding this comment

iPraveenParihar commented Nov 15, 2023

Madhu-1 left a comment

Choose a reason for hiding this comment

Rakshith-R commented Nov 15, 2023

mergify bot commented Nov 15, 2023

✅ Branch has been successfully rebased

Rakshith-R commented Nov 15, 2023

mergify bot commented Nov 15, 2023 • edited Loading

✅ The pull request has been merged automatically

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

ceph-csi-bot commented Nov 15, 2023

iPraveenParihar commented Nov 8, 2023 •

edited

Loading

mergify bot commented Nov 15, 2023 •

edited

Loading