Added forcecephkernelclient as startup parameter to force enabling ceph #664

shaas · 2019-10-01T14:51:21Z

Added "forcecephkernelclient" as parameter to force the use of the ceph kernel client for kernels < 4.17

This is need as SUSE Enterprise Storage 6 does not support fuse but ceph quotas got back-ported to kernels < 4.17

Signed-off-by: Stefan Haas shaas@suse.com

humblec · 2019-10-01T14:53:55Z

This is need as SUSE Enterprise Storage 6 does not support fuse but ceph quotas got back-ported to kernels < 4.17

@shaas are you saying SUSE kernel did a backport of quota support for kernels < 4.17 ?

humblec · 2019-10-01T14:55:56Z

We have some concerns on enabling kernel client without quota support as mentioned in #617. If SUSE kernel support it, I would rather go for suse specific kernel check and enablement than a general one.

denisok · 2019-10-01T15:02:50Z

any kernel could support needed cephfs client if it would be back ported there. It is probably falls down from CSI driver level to know such things.
As there is no ways to check such capabilities from cephfs client, the better approach is to get parameter that could be set by other tools that would know such information (distro specific).

Madhu-1 · 2019-10-01T15:06:06Z

@batrick @ajarr PTAL

BlaineEXE · 2019-10-01T15:22:05Z

#617 is a good instance of a user wanting this feature. I see a lot of good arguments here for why a hard limit on 4.17 is a poor choice, like here: #623 (comment)

batrick

Kernel version checks are good for weak enforcement but an override should exist too. I'm cool with this change.

cc @jtlayton @ajarr

Madhu-1

can you also update the doc for this new variable here https://github.com/ceph/ceph-csi/blob/master/docs/deploy-cephfs.md#configuration

shaas · 2019-10-02T13:10:01Z

can you also update the doc for this new variable here https://github.com/ceph/ceph-csi/blob/master/docs/deploy-cephfs.md#configuration

Done

Madhu-1 · 2019-10-03T06:30:37Z

7.401µs, max_per_file_from_linter: 3.717µs, diff: 3.568µs, exclude-rules: 3.031µs, skip_files: 1.471µs
pkg/cephfs/volumemounter.go:73:6: S1002: should omit comparison to bool constant, can be simplified to conf.ForceKernelClient (gosimple)
if conf.ForceKernelClient == true || majorVers >= 4 && minorVers >= 17 {
^
@shaas we have CI failure can you please fix it, Thanks

shaas · 2019-10-04T13:49:57Z

7.401µs, max_per_file_from_linter: 3.717µs, diff: 3.568µs, exclude-rules: 3.031µs, skip_files: 1.471µs
pkg/cephfs/volumemounter.go:73:6: S1002: should omit comparison to bool constant, can be simplified to conf.ForceKernelClient (gosimple)
if conf.ForceKernelClient == true || majorVers >= 4 && minorVers >= 17 {
^
@shaas we have CI failure can you please fix it, Thanks

Done

humblec · 2019-10-07T05:14:42Z

cmd/cephcsi.go

@@ -63,6 +63,7 @@ func init() {

 	// cephfs related flags
 	flag.StringVar(&conf.MountCacheDir, "mountcachedir", "", "mount info cache save dir")
+	flag.BoolVar(&conf.ForceKernelClient, "forcecephkernelclient", false, "enable Ceph Kernel clients on kernel < 4.17 which support quotas")


which does not support quotas?

No, the flag is to enable the kernel client on kernels with version <4.17 that do support quotas (via backported patches). All kernels >=4.17 support quotas and don't need the flag

humblec · 2019-10-07T05:15:12Z

pkg/util/util.go

 	// rbd related flag
 	Containerized bool // whether run as containerized

+	// cephfs related flags
+	ForceKernelClient bool // force to use the ceph kernel client even if the kernel is < 4.17


Isnt it better to put the fied as "forcekcephfs" ?

Madhu-1 · 2019-10-09T03:31:29Z

@humblec anything else is pending on this one?

Madhu-1 · 2019-10-10T05:34:15Z

Fixes: #664

Madhu-1 · 2019-10-10T05:36:27Z

@nixpanic @humblec PTAL

Madhu-1

LGTM

humblec · 2019-10-10T05:42:36Z

@Madhu-1 there were some unanswerd review comments

Madhu-1 · 2019-10-10T05:49:13Z

added DNM till we get answers for @humblec question, @shaas can you please take a look, would like to pull this for 1.2.2 release

Madhu-1

please update the target branch to master, instead of release-v1.2.0

humblec · 2019-10-10T06:30:32Z

All, this was possible in 1.0.0 does not justify much and if we allow kcephfs without quota support we are actually against kubernetes norms where it says the user should get "atleast requested space". however considering this is a concern from many users, the best bet would be "defaulting to false" which this PR does and adding "extra" documentation in "README" about this functionality with disclaimers in place which says, this is not recommended/supported if the kernel does not support quota.
@shaas we have to document this in README before getting this PR in

Pull request has been modified.

Madhu-1 · 2019-10-10T10:18:33Z

README.md

@@ -77,6 +77,9 @@ NOTE:
 compatible changes in the future, and is thus not recommended
 for production use.

+`NOTE` : The parameter `-forcecephkernelclient` enables the Kernel CephFS mounter on kernels < 4.17. 


I believe this Note should go to https://github.com/ceph/ceph-csi/blob/master/docs/deploy-cephfs.md which talks about cephfs deployment

Good point!
Moved this line

Madhu-1 · 2019-10-10T11:57:37Z

@shaas static check is failing

mdl --style scripts/mdl-style.rb ./docs/deploy-rbd.md 
mdl --style scripts/mdl-style.rb ./docs/deploy-cephfs.md 
./docs/deploy-cephfs.md:65: MD012 Multiple consecutive blank lines
A detailed description of the rules is available at https://github.com/markdownlint/markdownlint/blob/master/docs/RULES.md
mdl --style scripts/mdl-style.rb ./docs/coding.md 
mdl --style scripts/mdl-style.rb ./e2e/README.md 
mdl --style scripts/mdl-style.rb ./README.md 
mdl --style scripts/mdl-style.rb ./charts/ceph-csi-cephfs/README.md 
mdl --style scripts/mdl-style.rb ./charts/ceph-csi-rbd/README.md 
Makefile:45: recipe for target 'static-check' failed
make: *** [static-check] Error 123
The command "make static-check" exited with 2.

Signed-off-by: Stefan Haas <shaas@suse.com>

shaas · 2019-10-10T12:52:10Z

please update the target branch to master, instead of release-v1.2.0

Done

Madhu-1 · 2019-10-10T12:59:11Z

@humblec PTAL

shaas · 2019-10-14T09:01:14Z

Is there anything still needed/missing from my side?

Madhu-1 · 2019-10-14T10:00:03Z

@humblec @ShyamsundarR @nixpanic PTAL

humblec · 2019-10-16T05:43:55Z

@shaas we have to rebase this PR .

This reverts commit 2453156.

Support ceph#664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch>

Support #664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch>

Support #664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch> (cherry picked from commit 9d7b50d)

batrick approved these changes Oct 2, 2019

View reviewed changes

Madhu-1 reviewed Oct 2, 2019

View reviewed changes

humblec reviewed Oct 7, 2019

View reviewed changes

ajarr previously approved these changes Oct 7, 2019

View reviewed changes

ajarr mentioned this pull request Oct 7, 2019

cephfs: Choose kernel mounter when mounter specified is kernel #623

Closed

Madhu-1 added the backport-to-release-v1.2.0 label Oct 9, 2019

Madhu-1 approved these changes Oct 10, 2019

View reviewed changes

Madhu-1 added the DNM DO NOT MERGE label Oct 10, 2019

Madhu-1 self-requested a review October 10, 2019 05:50

Madhu-1 requested changes Oct 10, 2019

View reviewed changes

shaas changed the base branch from release-v1.2.0 to master October 10, 2019 09:32

Madhu-1 reviewed Oct 10, 2019

View reviewed changes

Madhu-1 removed the DNM DO NOT MERGE label Oct 10, 2019

Added forcecephkernelclient as startup parameter to force enabling ceph

e159371

Signed-off-by: Stefan Haas <shaas@suse.com>

shaas requested a review from Madhu-1 October 10, 2019 12:52

Madhu-1 approved these changes Oct 10, 2019

View reviewed changes

ShyamsundarR approved these changes Oct 15, 2019

View reviewed changes

Merge branch 'master' into ceph_kernel_parameter

05f6427

mergify bot merged commit 6a2717c into ceph:master Oct 16, 2019

mergify bot mentioned this pull request Oct 16, 2019

Added forcecephkernelclient as startup parameter to force enabling ceph (bp #664) #687

Merged

shaas deleted the ceph_kernel_parameter branch October 16, 2019 13:52

hswong3i added a commit to alvistack/ansible-role-kube_csi_cephfs that referenced this pull request Oct 24, 2019

https://github.com/ceph/ceph-csi/pull/664

8272c42

hswong3i added a commit to alvistack/ansible-role-kube_csi_rbd that referenced this pull request Oct 24, 2019

https://github.com/ceph/ceph-csi/pull/664

2453156

hswong3i added a commit to alvistack/ansible-collection-kubernetes that referenced this pull request Oct 24, 2019

https://github.com/ceph/ceph-csi/pull/664

f4e5c58

hswong3i added a commit to alvistack/ansible-role-kube_csi_cephfs that referenced this pull request Oct 24, 2019

rollback ceph/ceph-csi#664

cac6c80

hswong3i added a commit to alvistack/ansible-role-kube_csi_rbd that referenced this pull request Oct 24, 2019

Revert "ceph/ceph-csi#664"

60f4a5b

This reverts commit 2453156.

hswong3i added a commit to alvistack/ansible-collection-kubernetes that referenced this pull request Oct 24, 2019

rollback ceph/ceph-csi#664

f4e0f2f

Madhu-1 mentioned this pull request Dec 16, 2019

CSI: Add ENV variable to use ceph kernel client rook/rook#4390

Merged

9 tasks

jonasrutishauser added a commit to jonasrutishauser/ceph-csi that referenced this pull request Feb 18, 2020

Added forcecephkernelclient as startup parameter

bad4c00

Support ceph#664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch>

jonasrutishauser mentioned this pull request Feb 18, 2020

Added forcecephkernelclient as startup parameter #835

Merged

jonasrutishauser added a commit to jonasrutishauser/ceph-csi that referenced this pull request Feb 20, 2020

Added forcecephkernelclient as startup parameter

268dc30

Support ceph#664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch>

jonasrutishauser added a commit to jonasrutishauser/ceph-csi that referenced this pull request Feb 25, 2020

Added forcecephkernelclient as startup parameter

ce1e822

Support ceph#664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch>

Madhu-1 pushed a commit that referenced this pull request Mar 3, 2020

Added forcecephkernelclient as startup parameter

9d7b50d

Support #664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch>

mergify bot pushed a commit that referenced this pull request Mar 3, 2020

Added forcecephkernelclient as startup parameter

16b228e

Support #664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch> (cherry picked from commit 9d7b50d)

mergify bot pushed a commit that referenced this pull request Mar 3, 2020

Added forcecephkernelclient as startup parameter

8e870fd

Support #664 in the helm chart. Signed-off-by: Jonas Rutishauser <jonas.rutishauser@alumni.ethz.ch> (cherry picked from commit 9d7b50d)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added forcecephkernelclient as startup parameter to force enabling ceph #664

Added forcecephkernelclient as startup parameter to force enabling ceph #664

shaas commented Oct 1, 2019

humblec commented Oct 1, 2019

humblec commented Oct 1, 2019

denisok commented Oct 1, 2019

Madhu-1 commented Oct 1, 2019

BlaineEXE commented Oct 1, 2019

batrick left a comment

Madhu-1 left a comment

shaas commented Oct 2, 2019

Madhu-1 commented Oct 3, 2019

shaas commented Oct 4, 2019

humblec Oct 7, 2019

krig Oct 7, 2019

humblec Oct 7, 2019

shaas Oct 10, 2019

Madhu-1 commented Oct 9, 2019

Madhu-1 commented Oct 10, 2019

Madhu-1 commented Oct 10, 2019

Madhu-1 left a comment

humblec commented Oct 10, 2019

Madhu-1 commented Oct 10, 2019

Madhu-1 left a comment

humblec commented Oct 10, 2019

Madhu-1 Oct 10, 2019

shaas Oct 10, 2019

Madhu-1 commented Oct 10, 2019

shaas commented Oct 10, 2019

Madhu-1 commented Oct 10, 2019

shaas commented Oct 14, 2019

Madhu-1 commented Oct 14, 2019 •

edited

Loading

humblec commented Oct 16, 2019

Added forcecephkernelclient as startup parameter to force enabling ceph #664

Added forcecephkernelclient as startup parameter to force enabling ceph #664

Conversation

shaas commented Oct 1, 2019

humblec commented Oct 1, 2019

humblec commented Oct 1, 2019

denisok commented Oct 1, 2019

Madhu-1 commented Oct 1, 2019

BlaineEXE commented Oct 1, 2019

batrick left a comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

shaas commented Oct 2, 2019

Madhu-1 commented Oct 3, 2019

shaas commented Oct 4, 2019

humblec Oct 7, 2019

Choose a reason for hiding this comment

krig Oct 7, 2019

Choose a reason for hiding this comment

humblec Oct 7, 2019

Choose a reason for hiding this comment

shaas Oct 10, 2019

Choose a reason for hiding this comment

Madhu-1 commented Oct 9, 2019

Madhu-1 commented Oct 10, 2019

Madhu-1 commented Oct 10, 2019

Madhu-1 left a comment

Choose a reason for hiding this comment

humblec commented Oct 10, 2019

Madhu-1 commented Oct 10, 2019

Madhu-1 left a comment

Choose a reason for hiding this comment

humblec commented Oct 10, 2019

Madhu-1 Oct 10, 2019

Choose a reason for hiding this comment

shaas Oct 10, 2019

Choose a reason for hiding this comment

Madhu-1 commented Oct 10, 2019

shaas commented Oct 10, 2019

Madhu-1 commented Oct 10, 2019

shaas commented Oct 14, 2019

Madhu-1 commented Oct 14, 2019 • edited Loading

humblec commented Oct 16, 2019

Madhu-1 commented Oct 14, 2019 •

edited

Loading