cephfs: upgrading mount syntax #5090

MageekChiu · 2025-01-17T18:36:58Z

The old syntax is almost deprecated, and there are reasons to upgrade it to the new one

old syntax is lack of fsid param, which is critical for debugging and observability
mds_namespace is deprecated, so it might be inappropriate to continue using it
kernel will try new syntax first and then the old one(check with mount -v), it is a waste to use the old one

From the ubuntu manpage，20.04 LTS supports the old syntax while 22.04 LTS supports the new one.

nixpanic · 2025-01-20T09:02:04Z

Could you update the PR description with a reference that explains the changes? It would be useful to know how recent these changes are, and if there could be a problem when users have an older version deployed.

It seems the commit message contains a few long lines. Please edit it and keep them under 80 characters.

@kotreshhr, maybe you have a reference to the new/changed mount options handy?

MageekChiu · 2025-01-21T11:26:38Z

@nixpanic Greate advices, thank you.
I‘ve edited the PR description and the commit message.

Cool homepage by the way, need to add one myself 😄.

nixpanic

Looks good to me, thanks!

internal/cephfs/mounter/kernel.go

Pull request has been modified.

kotreshhr · 2025-01-28T07:08:02Z

Many thanks @MageekChiu!

Could you update the PR description with a reference that explains the changes? It would be useful to know how recent these changes are, and if there could be a problem when users have an older version deployed.

It seems the commit message contains a few long lines. Please edit it and keep them under 80 characters.

@kotreshhr, maybe you have a reference to the new/changed mount options handy?

I see the old and new mount options are already linked in the PR. Yes, the kernel tries the new syntax first and falls back to old if not available. I think this should be fine but tagging the kernel developer @Markuze @vshankar to confirm if this breaks anything based on the version it got introduced.

nixpanic · 2025-01-28T12:00:06Z

internal/cephfs/store/volumeoptions.go

@@ -46,6 +46,7 @@ type VolumeOptions struct {
 	RequestName  string
 	NamePrefix   string
 	ClusterID    string
+	FsID         string


Instead of always fetching the FsID, can you add a func (vo *VolumeOptions) GetFSID() (string, error) function?

This can then be used where needed, currently only in NewKernelMounter().

By extending the NewVolumeOptions() function, it grows (too) large, and the golang-ci linter fails due to that.

@nixpanic,I believe you may have missed this—we already have GetFSID() defined in the ClusterConnection struct. Since ClusterConnection is a member of the VolumeOptions struct, it can be accessed via vo.conn.GetFSID().

nixpanic · 2025-02-05T17:05:33Z

Hi @MageekChiu, the PR looks good to me. Can you squash all commits into a single one and force-push your branch? That makes it possible for the @mergify bot can do it's work and get this in soon.

Thanks!

iPraveenParihar · 2025-02-06T09:05:04Z

internal/cephfs/mounter/kernel.go

@@ -72,24 +72,25 @@ func (m *kernelMounter) mountKernel(
 		m.needsModprobe = false
 	}

+	fsID, err := volOptions.GetFSID()


why can't we use existing GetFSID() instead?

Suggested change

fsID, err := volOptions.GetFSID()

fsID, err := volOptions.GetConnection().GetFSID()

volOptions.GetConnection() can return nil, so it is not really safe. A new GetFSID() would prevent any incorrect usage, now, and possibly in the future.

Yeah, I think the checking is needed, Connect and Destroy do the checking too.
And I've squashed the commits.

volOptions.GetConnection() can return nil, so it is not really safe. A new GetFSID() would prevent any incorrect usage, now, and possibly in the future.

when volOptions struct is created, we make sure we have the connection set. There is no way GetConnection() could return nil.

IMO, GetConnection() should be handling the nil check, or the GetConnection() caller should check for nil and proceed.

as we have similar usage at some places. for example -

ceph-csi/internal/cephfs/nodeserver.go

Lines 145 to 153 in 72b9d5a

ioctx, err := volOptions.GetConnection().GetIoctx(volOptions.MetadataPool)

if err != nil {

log.ErrorLog(ctx, "Failed to create ioctx: %s", err)

return err

}

defer ioctx.Destroy()

Sure, improving GetConnection() would be nice too. I don't think that needs to be done in this PR though.

Sure, improving GetConnection() would be nice too. I don't think that needs to be done in this PR though.

That can be done through issue #5129.

iPraveenParihar · 2025-02-07T09:43:22Z

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

nixpanic · 2025-02-07T10:17:47Z

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

There has been a new Ceph release yesterday, building Ceph-CSI fails until the new container-image can install required RPMs (librados-devel etc...). 😢

nixpanic · 2025-02-13T09:13:24Z

@Mergifyio rebase

@iPraveenParihar , is there anything blocking this PR from your point of view?

The old syntax is almost deprecated,and there are reasons to upgrade it - old syntax is lack of fsid(critical for debugging and observability) - mds_namespace is deprecated, it might be inappropriate to continue using it - kernel will try new syntax first and then the old one, it's a waste Signed-off-by: mageekchiu <[email protected]>

mergify · 2025-02-13T09:13:43Z

rebase

✅ Branch has been successfully rebased

nixpanic · 2025-02-13T09:21:59Z

@Mergifyio queue

mergify · 2025-02-13T09:22:06Z

queue

🛑 The pull request has been removed from the queue `default`

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

ceph-csi-bot · 2025-02-13T09:22:21Z

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

ceph-csi-bot · 2025-02-13T09:22:21Z

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

ceph-csi-bot · 2025-02-13T09:22:21Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-02-13T09:22:21Z

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

ceph-csi-bot · 2025-02-13T09:22:22Z

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

ceph-csi-bot · 2025-02-13T09:22:22Z

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

ceph-csi-bot · 2025-02-13T09:22:22Z

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

ceph-csi-bot · 2025-02-13T09:22:22Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-02-13T09:22:23Z

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

ceph-csi-bot · 2025-02-13T09:22:24Z

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

ceph-csi-bot · 2025-02-13T09:22:25Z

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

mergify · 2025-02-13T09:56:17Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ ci/centos/mini-e2e-helm/k8s-1.30

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

nixpanic · 2025-02-13T10:06:54Z

It looks like the ci/centos/mini-e2e-helm/k8s-1.30 and ci/centos/mini-e2e/k8s-1.32 CI jobs failed during check static PVC with FsName, with

  I0213 09:41:19.618304   22892 utils.go:266] ID: 100 Req-ID: pv-name GRPC call: /csi.v1.Node/NodeStageVolume
  I0213 09:41:19.618350   22892 utils.go:267] ID: 100 Req-ID: pv-name GRPC request: {"secrets":"***stripped***","staging_target_path":"/var/lib/kubelet/plugins/kubernetes.io/csi/cephfs.csi.ceph.com/a0dd44413b10ee8f81629ecf349e45d15232fdfb63323715b60e6bbdb7f50dae/globalmount","volume_capability":{"access_mode":{"mode":"SINGLE_NODE_MULTI_WRITER"},"mount":{}},"volume_context":{"clusterID":"d1738887-c6ec-4570-bd3c-85bd6e09a8ab","fsName":"myfs","rootPath":"/volumes/testGroup/testSubVol/13f7794a-ff38-40fd-a599-91863996ac33","staticVolume":"true"},"volume_id":"pv-name"}
  I0213 09:41:19.618516   22892 volumemounter.go:126] requested mounter: , chosen mounter: kernel
  I0213 09:41:19.618594   22892 nodeserver.go:358] ID: 100 Req-ID: pv-name cephfs: mounting volume pv-name with Ceph kernel client
  I0213 09:41:19.618641   22892 crushlocation.go:41] CRUSH location labels passed for processing: [topology.kubernetes.io/region topology.kubernetes.io/zone]
  I0213 09:41:19.618656   22892 crushlocation.go:73] list of CRUSH location processed: map[region:east zone:east-zone1]
  E0213 09:41:19.618671   22892 nodeserver.go:368] ID: 100 Req-ID: pv-name failed to mount volume pv-name: failed to get fsID, stop mounting: cluster not connected yet Check dmesg logs if required.
  E0213 09:41:19.618694   22892 utils.go:271] ID: 100 Req-ID: pv-name GRPC error: rpc error: code = Internal desc = failed to get fsID, stop mounting: cluster not connected yet

mergify bot added the component/cephfs Issues related to CephFS label Jan 17, 2025

MageekChiu force-pushed the devel branch 3 times, most recently from 19eeb9a to 21d1d64 Compare January 21, 2025 10:46

nixpanic previously approved these changes Jan 21, 2025

View reviewed changes

nixpanic requested a review from a team January 21, 2025 12:35

iPraveenParihar reviewed Jan 24, 2025

View reviewed changes

internal/cephfs/mounter/kernel.go Outdated Show resolved Hide resolved

MageekChiu force-pushed the devel branch 2 times, most recently from 63c4692 to 42b818b Compare January 25, 2025 15:52

MageekChiu requested review from iPraveenParihar and nixpanic January 25, 2025 15:54

nixpanic reviewed Jan 28, 2025

View reviewed changes

MageekChiu requested a review from nixpanic February 2, 2025 11:42

iPraveenParihar requested changes Feb 6, 2025

View reviewed changes

MageekChiu force-pushed the devel branch from 82be04b to 4a721fe Compare February 6, 2025 12:49

nixpanic approved these changes Feb 6, 2025

View reviewed changes

nixpanic requested a review from iPraveenParihar February 7, 2025 09:34

nixpanic mentioned this pull request Feb 7, 2025

cephfs: make volOptions.GetConnection() safer to use #5129

Open

nixpanic force-pushed the devel branch from 4a721fe to 83cea81 Compare February 13, 2025 09:13

iPraveenParihar approved these changes Feb 13, 2025

View reviewed changes

mergify bot added the ok-to-test Label to trigger E2E tests label Feb 13, 2025

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Feb 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cephfs: upgrading mount syntax #5090

cephfs: upgrading mount syntax #5090

MageekChiu commented Jan 17, 2025 •

edited

Loading

nixpanic commented Jan 20, 2025

MageekChiu commented Jan 21, 2025

nixpanic left a comment

kotreshhr commented Jan 28, 2025

nixpanic Jan 28, 2025

iPraveenParihar Feb 3, 2025

nixpanic commented Feb 5, 2025

iPraveenParihar Feb 6, 2025

nixpanic Feb 6, 2025

MageekChiu Feb 6, 2025

iPraveenParihar Feb 7, 2025 •

edited

Loading

nixpanic Feb 7, 2025

nixpanic Feb 7, 2025

iPraveenParihar commented Feb 7, 2025

nixpanic commented Feb 7, 2025

nixpanic commented Feb 13, 2025

mergify bot commented Feb 13, 2025

nixpanic commented Feb 13, 2025

mergify bot commented Feb 13, 2025 •

edited

Loading

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

mergify bot commented Feb 13, 2025

nixpanic commented Feb 13, 2025

	fsID, err := volOptions.GetFSID()
	fsID, err := volOptions.GetConnection().GetFSID()


	ioctx, err := volOptions.GetConnection().GetIoctx(volOptions.MetadataPool)
	if err != nil {
	log.ErrorLog(ctx, "Failed to create ioctx: %s", err)

	return err
	}
	defer ioctx.Destroy()

cephfs: upgrading mount syntax #5090

Are you sure you want to change the base?

cephfs: upgrading mount syntax #5090

Conversation

MageekChiu commented Jan 17, 2025 • edited Loading

nixpanic commented Jan 20, 2025

MageekChiu commented Jan 21, 2025

nixpanic left a comment

Choose a reason for hiding this comment

kotreshhr commented Jan 28, 2025

nixpanic Jan 28, 2025

Choose a reason for hiding this comment

iPraveenParihar Feb 3, 2025

Choose a reason for hiding this comment

nixpanic commented Feb 5, 2025

iPraveenParihar Feb 6, 2025

Choose a reason for hiding this comment

nixpanic Feb 6, 2025

Choose a reason for hiding this comment

MageekChiu Feb 6, 2025

Choose a reason for hiding this comment

iPraveenParihar Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

nixpanic Feb 7, 2025

Choose a reason for hiding this comment

nixpanic Feb 7, 2025

Choose a reason for hiding this comment

iPraveenParihar commented Feb 7, 2025

nixpanic commented Feb 7, 2025

nixpanic commented Feb 13, 2025

mergify bot commented Feb 13, 2025

✅ Branch has been successfully rebased

nixpanic commented Feb 13, 2025

mergify bot commented Feb 13, 2025 • edited Loading

🛑 The pull request has been removed from the queue default

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

ceph-csi-bot commented Feb 13, 2025

mergify bot commented Feb 13, 2025

nixpanic commented Feb 13, 2025

MageekChiu commented Jan 17, 2025 •

edited

Loading

iPraveenParihar Feb 7, 2025 •

edited

Loading

mergify bot commented Feb 13, 2025 •

edited

Loading

🛑 The pull request has been removed from the queue `default`