cephfs: do not run modprobe if support is compiled into the kernel
#4378
Conversation
nixpanic
added
bug
| if err := execCommandErr(ctx, "modprobe", kernelModule); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
i think we can optimize it by moving this modeprobe to NewKernelMounter so that we run it only once when driver starts not always WDYT?
There was a problem hiding this comment.
There is a check to see if the driver is loaded when NewKernelMounter() is called. That sets m.needsModprobe. It is done so that NewKernelMounter() does not need to return an error, keeping other changes minimal.
There was a problem hiding this comment.
yeah i agree but this works byt when where a mountKernel is called and if needsModprobe not set we keep on running modeprobe, but running that can be avoided if we set needsModprobe to true on successful run of modprobe,
There was a problem hiding this comment.
we can take it as an enhancement as well.
There was a problem hiding this comment.
Ah, yes, if modprobe succeeded, it should be set to true
There was a problem hiding this comment.
this is done now, please review again.
| if err := execCommandErr(ctx, "modprobe", kernelModule); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
yeah i agree but this works byt when where a mountKernel is called and if needsModprobe not set we keep on running modeprobe, but running that can be avoided if we set needsModprobe to true on successful run of modprobe,
| ) | ||
|
|
||
| type KernelMounter struct{} | ||
| // testErrorf can be set by unit test for enhanced error reporting. | ||
| var testErrorf = func(fmt string, args ...any) { /* do nothing */ } |
There was a problem hiding this comment.
testErrorf might not be the right name for non-test file and also fmt can be replaced to something else as its a core package name.
There was a problem hiding this comment.
not sure I understand, fmt is just a string?
There was a problem hiding this comment.
what i mean is that fmt is also a package name, we might get into a static check problem if we use the same name for some variable name as well.
| if err := execCommandErr(ctx, "modprobe", kernelModule); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
we can take it as an enhancement as well.
| return err | ||
| } | ||
|
|
||
| m.needsModprobe = false |
There was a problem hiding this comment.
i think this needs to be set to true? also this is not of much use with the current code because mounter.New() is called for every NodeStage RPC call. we need to move this to Driver initialization time as it needs to run only one time.
There was a problem hiding this comment.
once the module is loaded, it will be listed in /proc/filesystems, so eventually modprobe is only run once
|
@Mergifyio rebase |
By reading the contents of /proc/filesystems, and checking if "ceph" is included there, running "modprobe ceph" can be skipped. Fixes: ceph#4376 Signed-off-by: Niels de Vos <[email protected]>
✅ Branch has been successfully rebased |
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at ab87045 |
|
/test ci/centos/k8s-e2e-external-storage/1.26 |
|
/test ci/centos/mini-e2e-helm/k8s-1.26 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/mini-e2e/k8s-1.26 |
|
/test ci/centos/k8s-e2e-external-storage/1.28 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/mini-e2e-helm/k8s-1.28 |
|
/test ci/centos/k8s-e2e-external-storage/1.27 |
|
/test ci/centos/mini-e2e/k8s-1.28 |
|
/test ci/centos/k8s-e2e-external-storage/1.29 |
|
/test ci/centos/mini-e2e-helm/k8s-1.27 |
|
/test ci/centos/mini-e2e-helm/k8s-1.29 |
|
/test ci/centos/mini-e2e/k8s-1.27 |
|
/test ci/centos/mini-e2e/k8s-1.29 |
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 [email protected]