csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile #14746

parth-gr · 2024-09-20T10:04:39Z

ROOK_CSI_DISABLE_DRIVER was not working accurate
from #14489
The csi driver was installed even if its not
expected

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

subhamkrai

we don't need return here, we need to add condition https://github.com/rook/rook/blob/master/pkg/operator/ceph/csi/controller.go#L335 and include && !disableCSI otherwise CSI operator will not start

subhamkrai · 2024-09-20T12:06:24Z

pkg/operator/ceph/csi/controller.go

@@ -332,7 +332,7 @@ func (r *ReconcileCSI) reconcile(request reconcile.Request) (reconcile.Result, e
 		}
 	}

-	if !EnableCSIOperator() {
+	if !EnableCSIOperator() && !disableCSI {



Suggested change

nit, not related to your change but if you could remove this empty line

travisn

Please add a more descriptive commit message and PR title. You can mention that the driver was being installed even though it was intended to be disabled.

ROOK_CSI_DISABLE_DRIVER was not working accurate from rook#14489 The csi driver was installed even if its not expected Signed-off-by: parth-gr <partharora1010@gmail.com>

parth-gr · 2024-09-20T15:36:54Z

Testing looks good

storageclient-737342087af10580-status-reporter-28780773-jrphc     0/1     Completed   0               49s
ux-backend-server-57b55857db-8hvls                                2/2     Running     0               23h
[rider@fedora Downloads]$ oc get pods | grep csi
ceph-csi-controller-manager-564fdb9f68-z6j7m                      2/2     Running           0               23h
csi-addons-controller-manager-945b5fb8d-4z6ns                     2/2     Running           0               23h
openshift-storage.cephfs.csi.ceph.com-ctrlplugin-76db9dcf55vxd7   7/7     Running           0               23h
openshift-storage.cephfs.csi.ceph.com-ctrlplugin-76db9dcf57f8f8   7/7     Running           0               23h
openshift-storage.cephfs.csi.ceph.com-nodeplugin-h7qt4            3/3     Running           6               43h
openshift-storage.cephfs.csi.ceph.com-nodeplugin-ldp8t            3/3     Running           6               43h
openshift-storage.cephfs.csi.ceph.com-nodeplugin-qk2nd            3/3     Running           0               43h
openshift-storage.rbd.csi.ceph.com-ctrlplugin-7f75d86c95-kst96    7/7     Running           0               23h
openshift-storage.rbd.csi.ceph.com-ctrlplugin-7f75d86c95-r7wpm    7/7     Running           0               23h
openshift-storage.rbd.csi.ceph.com-nodeplugin-6662j               4/4     Running           0               43h
openshift-storage.rbd.csi.ceph.com-nodeplugin-jsvmd               4/4     Running           8               43h
openshift-storage.rbd.csi.ceph.com-nodeplugin-l7ztl               4/4     Running           8               43h
[rider@fedora Downloads]$

travisn · 2024-09-20T16:47:20Z

pkg/operator/ceph/csi/controller.go

@@ -332,8 +332,7 @@ func (r *ReconcileCSI) reconcile(request reconcile.Request) (reconcile.Result, e
 		}
 	}

-	if !EnableCSIOperator() {
-
+	if !disableCSI && !EnableCSIOperator() {


Is the goal of the setting ROOK_CSI_DISABLE_DRIVER: true to completely skip all reconcile of the csi driver and csi operator? If so, seems like we should add a return statement above on line 205, so that we skip all reconciling in this method. If that is not the intention, then I'm not clear what is the intention of this flag since it's still reconciling some csi resources between lines 210-333.

I had the same question today to @subhamkrai as I was under the same impression,

He mentioned ROOK_CSI_DISABLE_DRIVER should only responsible for not creating CSI driver from rook

And enable_csi_operator should be responsible for creating CSI operator.

So if ROOK_CSI_DISABLE_DRIVER:true and enable_csi_operator:true it should create the CSI operator resources and not CSI driver
But we can discuss this if needed more discussion and changes...

@subhamkrai please add your thoughts ^^

IMO, ROOK_CSI_DISABLE_DRIVER is the config that we should prioritize working correctly. This config has existed for a while, and I think we expect it to continue to be used after the CSI operator is GA'ed as well. This config has had the meaning "Rook should not manage CSI driver resources," and I think keeping that meaning will help others continue to use Rook as they have been without unexpected results.

IIUC, ENABLE_CSI_OPERATOR is something that I think is temporary while we are working to test and migrate to the new CSI operator setup. I think it's fine if ENABLE_CSI_OPERATOR doesn't do anything if ROOK_CSI_DISABLE_DRIVER == true since we don't expect to need the option for a long time.

I would suggest that ROOK_CSI_DISABLE_DRIVER behaves to disable all Rook management of CSI drivers, including the operator. I think that is also what Travis is expecting, based on his comment.

I guess the important thing might be to ensure Rook will remove CSI drivers when ROOK_CSI_DISABLE_DRIVER == true, assuming that is the behavior in Rook today.

Yes above should be the correct expectation 👍🏻

We need to decide which flag we want to give precedence as let's say

User set ROOK_CSI_DISABLE_DRIVER==true and ENABLE_CSI_OPERATOR == true, if we place return higher it will skip deploying CSI Operator.

While working on this, I treated ROOK_CSI_DISABLE_DRIVER to enable/disable driver code and ENABLE_CSI_OPERATOR to enable/disable operator code.

We can also log an error when user sets ROOK_CSI_DISABLE_DRIVER==trueandENABLE_CSI_OPERATOR == true. But just to mention, we have to csi way of deployment driver and operator` so to have it clear it clear for user I treated like this currently in the code.

As per the huddle discussion,
We are keeping the current implementations,

Later when we remove the csi operator flag check, then we will bring back the disable the controller reconcile at very start of the reconcile

Per discussion, approving to go ahead with the current changes

travisn

marking as changes requested until the discussion is resolved

travisn · 2024-09-23T16:19:54Z

pkg/operator/ceph/csi/controller.go

@@ -332,8 +332,7 @@ func (r *ReconcileCSI) reconcile(request reconcile.Request) (reconcile.Result, e
 		}
 	}

-	if !EnableCSIOperator() {
-
+	if !disableCSI && !EnableCSIOperator() {


Per discussion, approving to go ahead with the current changes

csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile (backport #14746)

parth-gr marked this pull request as draft September 20, 2024 10:04

parth-gr marked this pull request as ready for review September 20, 2024 10:06

Madhu-1 approved these changes Sep 20, 2024

View reviewed changes

Madhu-1 requested a review from subhamkrai September 20, 2024 10:22

subhamkrai requested changes Sep 20, 2024

View reviewed changes

parth-gr force-pushed the fix-csi-driver branch from c93287d to 54e828a Compare September 20, 2024 12:00

subhamkrai requested changes Sep 20, 2024

View reviewed changes

parth-gr force-pushed the fix-csi-driver branch from 54e828a to b1b6ec1 Compare September 20, 2024 12:06

parth-gr requested a review from subhamkrai September 20, 2024 14:14

travisn reviewed Sep 20, 2024

View reviewed changes

parth-gr force-pushed the fix-csi-driver branch 2 times, most recently from 5c695d0 to 4072dbd Compare September 20, 2024 14:54

parth-gr changed the title ~~csi: fix the csi driver reconcile~~ csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile Sep 20, 2024

csi: fix the disable driver flag in the csi driver reconcile

166e4f3

ROOK_CSI_DISABLE_DRIVER was not working accurate from rook#14489 The csi driver was installed even if its not expected Signed-off-by: parth-gr <partharora1010@gmail.com>

parth-gr force-pushed the fix-csi-driver branch from 4072dbd to 166e4f3 Compare September 20, 2024 15:00

parth-gr added do-not-merge DO NOT MERGE :) backport-release-1.15 and removed do-not-merge DO NOT MERGE :) labels Sep 20, 2024

parth-gr requested a review from travisn September 20, 2024 15:36

subhamkrai approved these changes Sep 20, 2024

View reviewed changes

travisn reviewed Sep 20, 2024

View reviewed changes

parth-gr requested a review from travisn September 20, 2024 17:46

travisn requested changes Sep 20, 2024

View reviewed changes

parth-gr requested review from BlaineEXE and travisn September 23, 2024 15:59

travisn approved these changes Sep 23, 2024

View reviewed changes

travisn merged commit aba78ea into rook:master Sep 23, 2024
54 checks passed

mergify bot mentioned this pull request Sep 23, 2024

csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile (backport #14746) #14759

Merged

6 tasks

mergify bot added a commit that referenced this pull request Sep 24, 2024

Merge pull request #14759 from rook/mergify/bp/release-1.15/pr-14746

0ea35c9

csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile (backport #14746)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile #14746

csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile #14746

parth-gr commented Sep 20, 2024 •

edited

Loading

subhamkrai left a comment

subhamkrai Sep 20, 2024

parth-gr Sep 20, 2024

travisn left a comment

parth-gr commented Sep 20, 2024

travisn Sep 20, 2024

parth-gr Sep 20, 2024 •

edited

Loading

parth-gr Sep 20, 2024

BlaineEXE Sep 20, 2024 •

edited

Loading

Madhu-1 Sep 23, 2024

subhamkrai Sep 23, 2024

parth-gr Sep 23, 2024 •

edited

Loading

travisn Sep 23, 2024

travisn left a comment

travisn Sep 23, 2024

csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile #14746

csi: fix the ROOK_CSI_DISABLE_DRIVER flag in the csi reconcile #14746

Conversation

parth-gr commented Sep 20, 2024 • edited Loading

subhamkrai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

parth-gr commented Sep 20, 2024

Choose a reason for hiding this comment

parth-gr Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr commented Sep 20, 2024 •

edited

Loading

parth-gr Sep 20, 2024 •

edited

Loading

BlaineEXE Sep 20, 2024 •

edited

Loading

parth-gr Sep 23, 2024 •

edited

Loading