From ab87045afb0c15ca4d30ae01003ed0f331843181 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Tue, 16 Jan 2024 15:58:22 +0100 Subject: [PATCH] cephfs: do not run `modprobe` if support is compiled into the kernel By reading the contents of /proc/filesystems, and checking if "ceph" is included there, running "modprobe ceph" can be skipped. Fixes: #4376 Signed-off-by: Niels de Vos --- internal/cephfs/mounter/kernel.go | 69 +++++++++++++++++++++--- internal/cephfs/mounter/kernel_test.go | 38 +++++++++++++ internal/cephfs/mounter/volumemounter.go | 2 +- internal/cephfs/nodeserver.go | 4 +- internal/cephfs/nodeserver_test.go | 8 +-- 5 files changed, 107 insertions(+), 14 deletions(-) create mode 100644 internal/cephfs/mounter/kernel_test.go diff --git a/internal/cephfs/mounter/kernel.go b/internal/cephfs/mounter/kernel.go index 8d6d833b3..924b9565a 100644 --- a/internal/cephfs/mounter/kernel.go +++ b/internal/cephfs/mounter/kernel.go @@ -19,6 +19,8 @@ package mounter import ( "context" "fmt" + "os" + "strings" "github.com/ceph/ceph-csi/internal/cephfs/store" "github.com/ceph/ceph-csi/internal/util" @@ -27,13 +29,47 @@ import ( const ( volumeMounterKernel = "kernel" netDev = "_netdev" + kernelModule = "ceph" ) -type KernelMounter struct{} +// testErrorf can be set by unit test for enhanced error reporting. +var testErrorf = func(fmt string, args ...any) { /* do nothing */ } -func mountKernel(ctx context.Context, mountPoint string, cr *util.Credentials, volOptions *store.VolumeOptions) error { - if err := execCommandErr(ctx, "modprobe", "ceph"); err != nil { - return err +type KernelMounter interface { + Mount( + ctx context.Context, + mountPoint string, + cr *util.Credentials, + volOptions *store.VolumeOptions, + ) error + + Name() string +} + +type kernelMounter struct { + // needsModprobe indicates that the ceph kernel module is not loaded in + // the kernel yet (or compiled into it) + needsModprobe bool +} + +func NewKernelMounter() KernelMounter { + return &kernelMounter{ + needsModprobe: !filesystemSupported(kernelModule), + } +} + +func (m *kernelMounter) mountKernel( + ctx context.Context, + mountPoint string, + cr *util.Credentials, + volOptions *store.VolumeOptions, +) error { + if m.needsModprobe { + if err := execCommandErr(ctx, "modprobe", kernelModule); err != nil { + return err + } + + m.needsModprobe = false } args := []string{ @@ -68,7 +104,7 @@ func mountKernel(ctx context.Context, mountPoint string, cr *util.Credentials, v return err } -func (m *KernelMounter) Mount( +func (m *kernelMounter) Mount( ctx context.Context, mountPoint string, cr *util.Credentials, @@ -78,7 +114,26 @@ func (m *KernelMounter) Mount( return err } - return mountKernel(ctx, mountPoint, cr, volOptions) + return m.mountKernel(ctx, mountPoint, cr, volOptions) } -func (m *KernelMounter) Name() string { return "Ceph kernel client" } +func (m *kernelMounter) Name() string { return "Ceph kernel client" } + +// filesystemSupported checks if the passed name of the filesystem is included +// in /proc/filesystems. +func filesystemSupported(fs string) bool { + // /proc/filesystems contains a list of all supported filesystems, + // either compiled into the kernel, or as loadable module. + data, err := os.ReadFile("/proc/filesystems") + if err != nil { + testErrorf("failed to read /proc/filesystems: %v", err) + + return false + } + + // The format of /proc/filesystems is one filesystem per line, an + // optional keyword ("nodev") followed by a tab and the name of the + // filesystem. Matching ceph for the ceph kernel module that + // supports CephFS. + return strings.Contains(string(data), "\t"+fs+"\n") +} diff --git a/internal/cephfs/mounter/kernel_test.go b/internal/cephfs/mounter/kernel_test.go new file mode 100644 index 000000000..f370d4768 --- /dev/null +++ b/internal/cephfs/mounter/kernel_test.go @@ -0,0 +1,38 @@ +/* +Copyright 2024 The Ceph-CSI Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package mounter + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFilesystemSupported(t *testing.T) { + t.Parallel() + + testErrorf = func(fmt string, args ...any) { + t.Errorf(fmt, args...) + } + + // "proc" is always a supported filesystem, we detect supported + // filesystems by reading from it + assert.True(t, filesystemSupported("proc")) + + // "nonefs" is a made-up name, and does not exist + assert.False(t, filesystemSupported("nonefs")) +} diff --git a/internal/cephfs/mounter/volumemounter.go b/internal/cephfs/mounter/volumemounter.go index dcfccb177..986ac6ee6 100644 --- a/internal/cephfs/mounter/volumemounter.go +++ b/internal/cephfs/mounter/volumemounter.go @@ -131,7 +131,7 @@ func New(volOptions *store.VolumeOptions) (VolumeMounter, error) { case volumeMounterFuse: return &FuseMounter{}, nil case volumeMounterKernel: - return &KernelMounter{}, nil + return NewKernelMounter(), nil } return nil, fmt.Errorf("unknown mounter '%s'", chosenMounter) diff --git a/internal/cephfs/nodeserver.go b/internal/cephfs/nodeserver.go index 58b2e3deb..2af514c36 100644 --- a/internal/cephfs/nodeserver.go +++ b/internal/cephfs/nodeserver.go @@ -801,7 +801,7 @@ func (ns *NodeServer) setMountOptions( } volOptions.FuseMountOptions = util.MountOptionsAdd(volOptions.FuseMountOptions, configuredMountOptions) volOptions.FuseMountOptions = util.MountOptionsAdd(volOptions.FuseMountOptions, mountOptions...) - case *mounter.KernelMounter: + case mounter.KernelMounter: configuredMountOptions = ns.kernelMountOptions // override of kernelMountOptions are set if kernelMountOptions != "" { @@ -821,7 +821,7 @@ func (ns *NodeServer) setMountOptions( if !csicommon.MountOptionContains(strings.Split(volOptions.FuseMountOptions, ","), readOnly) { volOptions.FuseMountOptions = util.MountOptionsAdd(volOptions.FuseMountOptions, readOnly) } - case *mounter.KernelMounter: + case mounter.KernelMounter: if !csicommon.MountOptionContains(strings.Split(volOptions.KernelMountOptions, ","), readOnly) { volOptions.KernelMountOptions = util.MountOptionsAdd(volOptions.KernelMountOptions, readOnly) } diff --git a/internal/cephfs/nodeserver_test.go b/internal/cephfs/nodeserver_test.go index d35c1e2cf..e4f714675 100644 --- a/internal/cephfs/nodeserver_test.go +++ b/internal/cephfs/nodeserver_test.go @@ -77,7 +77,7 @@ func Test_setMountOptions(t *testing.T) { { name: "KernelMountOptions set in cluster-1 config and not set in CLI", ns: &NodeServer{}, - mnt: mounter.VolumeMounter(&mounter.KernelMounter{}), + mnt: mounter.VolumeMounter(mounter.NewKernelMounter()), volOptions: &store.VolumeOptions{ ClusterID: "cluster-1", }, @@ -97,7 +97,7 @@ func Test_setMountOptions(t *testing.T) { ns: &NodeServer{ kernelMountOptions: cliKernelMountOptions, }, - mnt: mounter.VolumeMounter(&mounter.KernelMounter{}), + mnt: mounter.VolumeMounter(mounter.NewKernelMounter()), volOptions: &store.VolumeOptions{ ClusterID: "cluster-1", }, @@ -119,7 +119,7 @@ func Test_setMountOptions(t *testing.T) { ns: &NodeServer{ kernelMountOptions: cliKernelMountOptions, }, - mnt: mounter.VolumeMounter(&mounter.KernelMounter{}), + mnt: mounter.VolumeMounter(mounter.NewKernelMounter()), volOptions: &store.VolumeOptions{ ClusterID: "cluster-2", }, @@ -162,7 +162,7 @@ func Test_setMountOptions(t *testing.T) { if !strings.Contains(tc.volOptions.FuseMountOptions, tc.want) { t.Errorf("Set FuseMountOptions = %v Required FuseMountOptions = %v", tc.volOptions.FuseMountOptions, tc.want) } - case *mounter.KernelMounter: + case mounter.KernelMounter: if !strings.Contains(tc.volOptions.KernelMountOptions, tc.want) { t.Errorf("Set KernelMountOptions = %v Required KernelMountOptions = %v", tc.volOptions.KernelMountOptions, tc.want) }