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

Jump to bottom

cleanup: create k8s.io/mount-utils Mounter only once #3247

Merged
merged 1 commit into from
Jul 21, 2022
Merged

cleanup: create k8s.io/mount-utils Mounter only once #3247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions internal/cephfs/fuserecovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ func (ms mountState) String() string {
}[int(ms)]
}

func getMountState(path string) (mountState, error) {
isMnt, err := util.IsMountPoint(path)
func (ns *NodeServer) getMountState(path string) (mountState, error) {
isMnt, err := util.IsMountPoint(ns.Mounter, path)
if err != nil {
if util.IsCorruptedMountError(err) {
return msCorrupted, nil
Expand Down Expand Up @@ -117,12 +117,12 @@ func (ns *NodeServer) tryRestoreFuseMountsInNodePublish(
) error {
// Check if there is anything to restore.

stagingTargetMs, err := getMountState(stagingTargetPath)
stagingTargetMs, err := ns.getMountState(stagingTargetPath)
if err != nil {
return err
}

targetMs, err := getMountState(targetPath)
targetMs, err := ns.getMountState(targetPath)
if err != nil {
return err
}
Expand Down Expand Up @@ -230,7 +230,7 @@ func (ns *NodeServer) tryRestoreFuseMountInNodeStage(
) error {
// Check if there is anything to restore.

stagingTargetMs, err := getMountState(stagingTargetPath)
stagingTargetMs, err := ns.getMountState(stagingTargetPath)
if err != nil {
return err
}
Expand Down
12 changes: 6 additions & 6 deletions internal/cephfs/nodeserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func (ns *NodeServer) NodeStageVolume(
return nil, status.Errorf(codes.Internal, "failed to try to restore FUSE mounts: %v", err)
}

isMnt, err := util.IsMountPoint(stagingTargetPath)
isMnt, err := util.IsMountPoint(ns.Mounter, stagingTargetPath)
if err != nil {
log.ErrorLog(ctx, "stat failed: %v", err)

Expand Down Expand Up @@ -426,7 +426,7 @@ func (ns *NodeServer) NodePublishVolume(

// Ensure staging target path is a mountpoint.

if isMnt, err := util.IsMountPoint(stagingTargetPath); err != nil {
if isMnt, err := util.IsMountPoint(ns.Mounter, stagingTargetPath); err != nil {
log.ErrorLog(ctx, "stat failed: %v", err)

return nil, status.Error(codes.Internal, err.Error())
Expand All @@ -438,7 +438,7 @@ func (ns *NodeServer) NodePublishVolume(

// Check if the volume is already mounted

isMnt, err := util.IsMountPoint(targetPath)
isMnt, err := util.IsMountPoint(ns.Mounter, targetPath)
if err != nil {
log.ErrorLog(ctx, "stat failed: %v", err)

Expand Down Expand Up @@ -482,7 +482,7 @@ func (ns *NodeServer) NodeUnpublishVolume(
// considering kubelet make sure node operations like unpublish/unstage...etc can not be called
// at same time, an explicit locking at time of nodeunpublish is not required.
targetPath := req.GetTargetPath()
isMnt, err := util.IsMountPoint(targetPath)
isMnt, err := util.IsMountPoint(ns.Mounter, targetPath)
if err != nil {
log.ErrorLog(ctx, "stat failed: %v", err)

Expand Down Expand Up @@ -551,7 +551,7 @@ func (ns *NodeServer) NodeUnstageVolume(
return nil, status.Error(codes.Internal, err.Error())
}

isMnt, err := util.IsMountPoint(stagingTargetPath)
isMnt, err := util.IsMountPoint(ns.Mounter, stagingTargetPath)
if err != nil {
log.ErrorLog(ctx, "stat failed: %v", err)

Expand Down Expand Up @@ -637,7 +637,7 @@ func (ns *NodeServer) NodeGetVolumeStats(
}

if stat.Mode().IsDir() {
return csicommon.FilesystemNodeGetVolumeStats(ctx, targetPath)
return csicommon.FilesystemNodeGetVolumeStats(ctx, ns.Mounter, targetPath)
}

return nil, status.Errorf(codes.InvalidArgument, "targetpath %q is not a directory or device", targetPath)
Expand Down
6 changes: 4 additions & 2 deletions internal/csi-common/nodeserver-default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ import (
"github.com/container-storage-interface/spec/lib/go/csi"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
mount "k8s.io/mount-utils"
)

// DefaultNodeServer stores driver object.
type DefaultNodeServer struct {
Driver *CSIDriver
Type string
Driver *CSIDriver
Type string
Mounter mount.Interface
}

// NodeExpandVolume returns unimplemented response.
Expand Down
14 changes: 10 additions & 4 deletions internal/csi-common/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"google.golang.org/grpc/status"
"k8s.io/klog/v2"
"k8s.io/kubernetes/pkg/volume"
mount "k8s.io/mount-utils"
)

func parseEndpoint(ep string) (string, string, error) {
Expand All @@ -61,8 +62,9 @@ func NewDefaultNodeServer(d *CSIDriver, t string, topology map[string]string) *D
d.topology = topology

return &DefaultNodeServer{
Driver: d,
Type: t,
Driver: d,
Type: t,
Mounter: mount.New(""),
}
}

Expand Down Expand Up @@ -229,8 +231,12 @@ func panicHandler(
// requested by the NodeGetVolumeStats CSI procedure.
// It is shared for FileMode volumes, both the CephFS and RBD NodeServers call
// this.
func FilesystemNodeGetVolumeStats(ctx context.Context, targetPath string) (*csi.NodeGetVolumeStatsResponse, error) {
isMnt, err := util.IsMountPoint(targetPath)
func FilesystemNodeGetVolumeStats(
ctx context.Context,
mounter mount.Interface,
targetPath string,
) (*csi.NodeGetVolumeStatsResponse, error) {
isMnt, err := util.IsMountPoint(mounter, targetPath)
if err != nil {
if os.IsNotExist(err) {
return nil, status.Errorf(codes.InvalidArgument, "targetpath %s does not exist", targetPath)
Expand Down
3 changes: 2 additions & 1 deletion internal/csi-common/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/container-storage-interface/spec/lib/go/csi"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
mount "k8s.io/mount-utils"
)

var fakeID = "fake-id"
Expand Down Expand Up @@ -87,7 +88,7 @@ func TestFilesystemNodeGetVolumeStats(t *testing.T) {

// retry until a mountpoint is found
for {
stats, err := FilesystemNodeGetVolumeStats(context.TODO(), cwd)
stats, err := FilesystemNodeGetVolumeStats(context.TODO(), mount.New(""), cwd)
if err != nil && cwd != "/" && strings.HasSuffix(err.Error(), "is not mounted") {
// try again with the parent directory
cwd = filepath.Dir(cwd)
Expand Down
4 changes: 0 additions & 4 deletions internal/rbd/driver/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/ceph/ceph-csi/internal/util/log"

"github.com/container-storage-interface/spec/lib/go/csi"
mount "k8s.io/mount-utils"
)

// Driver contains the default identity,node and controller struct.
Expand Down Expand Up @@ -73,11 +72,8 @@ func NewReplicationServer(c *rbd.ControllerServer) *rbd.ReplicationServer {

// NewNodeServer initialize a node server for rbd CSI driver.
func NewNodeServer(d *csicommon.CSIDriver, t string, topology map[string]string) (*rbd.NodeServer, error) {
mounter := mount.New("")

return &rbd.NodeServer{
DefaultNodeServer: csicommon.NewDefaultNodeServer(d, t, topology),
Mounter: mounter,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need this change for rbd right? or else it will panic when we try to access mounter

Copy link
Member Author

Choose a reason for hiding this comment

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

no, that should be set by the DefaultNodeServer... not sure why this isn't working as I expected 🤔

Having a look now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping the overloading Mounter attribute from rbd.NodeServer should work 🤞

VolumeLocks: util.NewVolumeLocks(),
}, nil
}
Expand Down
5 changes: 2 additions & 3 deletions internal/rbd/nodeserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import (
// node server spec.
type NodeServer struct {
*csicommon.DefaultNodeServer
Mounter mount.Interface
// A map storing all volumes with ongoing operations so that additional operations
// for that same volume (as defined by VolumeID) return an Aborted error
VolumeLocks *util.VolumeLocks
Expand Down Expand Up @@ -806,7 +805,7 @@ func (ns *NodeServer) mountVolume(ctx context.Context, stagingPath string, req *
if readOnly {
mountOptions = append(mountOptions, "ro")
}
if err := util.Mount(stagingPath, targetPath, fsType, mountOptions); err != nil {
if err := util.Mount(ns.Mounter, stagingPath, targetPath, fsType, mountOptions); err != nil {
return status.Error(codes.Internal, err.Error())
}

Expand Down Expand Up @@ -1241,7 +1240,7 @@ func (ns *NodeServer) NodeGetVolumeStats(
}

if stat.Mode().IsDir() {
return csicommon.FilesystemNodeGetVolumeStats(ctx, targetPath)
return csicommon.FilesystemNodeGetVolumeStats(ctx, ns.Mounter, targetPath)
} else if (stat.Mode() & os.ModeDevice) == os.ModeDevice {
return blockNodeGetVolumeStats(ctx, targetPath)
}
Expand Down
11 changes: 4 additions & 7 deletions internal/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,8 @@ func checkDirExists(p string) bool {
}

// IsMountPoint checks if the given path is mountpoint or not.
func IsMountPoint(p string) (bool, error) {
dummyMount := mount.New("")
notMnt, err := dummyMount.IsLikelyNotMountPoint(p)
func IsMountPoint(mounter mount.Interface, p string) (bool, error) {
notMnt, err := mounter.IsLikelyNotMountPoint(p)
if err != nil {
return false, err
}
Expand All @@ -348,10 +347,8 @@ func ReadMountInfoForProc(proc string) ([]mount.MountInfo, error) {
}

// Mount mounts the source to target path.
func Mount(source, target, fstype string, options []string) error {
dummyMount := mount.New("")

return dummyMount.MountSensitiveWithoutSystemd(source, target, fstype, options, nil)
func Mount(mounter mount.Interface, source, target, fstype string, options []string) error {
return mounter.MountSensitiveWithoutSystemd(source, target, fstype, options, nil)
}

// MountOptionsAdd adds the `add` mount options to the `options` and returns a
Expand Down