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.

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]>

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...). 😢

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

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

	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

MageekChiu commented Jan 17, 2025 •

edited

Loading

iPraveenParihar Feb 7, 2025 •

edited

Loading