cephfs: upgrading mount syntax

[MageekChiu]

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]

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?

[MageekChiu]

@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!

[iPraveenParihar]

@MageekChiu, The volOptions.ClusterID does not represent the actual Ceph cluster ID. Instead, Ceph-CSI uses it as a unique identifier to map the cluster configuration between the clusterID parameter specified in the StorageClass and the clusterID defined in the ceph-csi-config ConfigMap. This field does not necessarily have to be the cluster FSID required by the mount option.

[nixpanic]

Good catch! internal/util.GetMonsAndClusterID() should be used for obtaining the real cluster-id.

[iPraveenParihar]

@nixpanic 🤔, even that just returns the clusterID from the csi configFile. The right method is internal/util.GetFSID()

ceph-csi/internal/util/connection.go

Lines 118 to 124 in 4e02fae

	func (cc *ClusterConnection) GetFSID() (string, error) {
	if cc.conn == nil {
	return "", errors.New("cluster is not connected yet")
	}
	return cc.conn.GetFSID()
	}

[MageekChiu]

@iPraveenParihar @nixpanic thanks for your opinions, very helpful.

Ceph support multiple fs nowadays, but I don't see any parameter indicates our csi supports that.
So we only support getting fsid like bellow?

conn := &util.ClusterConnection{}
err = conn.Connect(monitors, cr)
if err != nil {
    return nil, status.Errorf(codes.Internal, "failed to connect to MONs %q: %s", monitors, err)
}
defer conn.Destroy()

fsID, err := conn.GetFSID()
if err != nil {
    return nil, status.Errorf(codes.Internal, "failed to get cephfs id: %s", err)
}

By the way, the fsid of the default and only one fs is indeed the clusterID itself.

If we don't intend to support multiple fs, then internal/util.GetMonsAndClusterID() should be enough.

[nixpanic]

@nixpanic 🤔, even that just returns the clusterID from the csi configFile.

Isn't that the ceph-csi-config ConfigMap?

[MageekChiu]

Ceph support multiple fs nowadays, but I don't see any parameter indicates our csi supports that.

My mistake, the fsName in storageclass does that, I'll do the refactoring.

And for clarity, FsID is UUID for a ceph cluster rather than an id for a CephFS instance(file system)

Pull request has been modified.

[kotreshhr]

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]

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.