Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cephfs: upgrading mount syntax #5090

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

cephfs: upgrading mount syntax #5090

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21d1d64
cephfs: upgrading mount syntax
MageekChiu Jan 17, 2025
42b818b
cephfs: upgrading mount syntax
MageekChiu Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions internal/cephfs/mounter/kernel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,13 @@ func (m *kernelMounter) mountKernel(

args := []string{
"-t", "ceph",
fmt.Sprintf("%s:%s", volOptions.Monitors, volOptions.RootPath),
fmt.Sprintf("%s@%s.%s=%s", cr.ID, volOptions.ClusterID, volOptions.FsName, volOptions.RootPath),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()
}

Copy link
Author

@MageekChiu MageekChiu Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Author

@MageekChiu MageekChiu Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

mountPoint,
}

optionsStr := fmt.Sprintf("name=%s,secretfile=%s", cr.ID, cr.KeyFile)
mdsNamespace := ""
if volOptions.FsName != "" {
mdsNamespace = "mds_namespace=" + volOptions.FsName
}
optionsStr = util.MountOptionsAdd(optionsStr, mdsNamespace, volOptions.KernelMountOptions, netDev)
optionsStr := fmt.Sprintf("mon_addr=%s,secretfile=%s", strings.ReplaceAll(volOptions.Monitors, ",", "/"), cr.KeyFile)

optionsStr = util.MountOptionsAdd(optionsStr, volOptions.KernelMountOptions, netDev)

args = append(args, "-o", optionsStr)

Expand Down
Loading