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

Avoid reconciling rook-ceph-operator-config via the external controller #2970

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Avoid reconciling rook-ceph-operator-config via the external controller #2970

merged 2 commits into from
Jan 21, 2025

Conversation

iamniting
Copy link
Member

@iamniting iamniting commented Jan 20, 2025 •
edited
Loading

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2025
@iamniting iamniting force-pushed the 4.18 branch 3 times, most recently from b8711c1 to 1d32743 Compare January 20, 2025 13:39
@iamniting
Avoid reconciling rook-ceph-operator-config via the external controller
a4bec06 
The rook-ceph-operator-config ConfigMap is managed by the
OCSInitialization controller and should only be reconciled
by it. This ensures consistency and prevents duplicate handling.

It should be reconciled from OCSInitialization only.

Signed-off-by: Nitin Goyal <[email protected]>
@iamniting
Remove ROOK_CSI_ENABLE_CEPHFS key from rook-ceph-operator-config CM
8f8afcb 
The ROOK_CSI_ENABLE_CEPHFS key was set in the external mode within the
rook-ceph-operator-config ConfigMap. However, it should have been
removed when transitioning from rook-ceph-operator-config to
ocs-operator-config. The rook-ceph-operator-config ConfigMap is now
retained only for customer modifications.

This code can be safely removed in the next release, as it only require
to run once to delete the key.

Signed-off-by: Nitin Goyal <[email protected]>
@iamniting iamniting changed the title Do not set ROOK_CSI_ENABLE_CEPHFS key in the rook-ceph-operator-config configmap Avoid reconciling rook-ceph-operator-config via the external controller Jan 20, 2025
parth-gr
parth-gr reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@parth-gr parth-gr left a comment

Choose a reason for hiding this comment

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

looks good,

@travisn @Madhu-1 by this PR external mode will always have cephfs csi-provisioners.
plus more details on commit message

@iamniting
Copy link
Member Author

/cherry-pick release-4.18

@openshift-cherrypick-robot
Copy link

@iamniting: once the present PR merges, I will cherry-pick it on top of release-4.18 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.18

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

malayparida2000
malayparida2000 approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@malayparida2000 malayparida2000 left a comment

Choose a reason for hiding this comment

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

/lgtm,
/hold in case we need more eyes on this before merge.

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jan 20, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, malayparida2000

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Madhu-1
Copy link
Member

Madhu-1 commented Jan 20, 2025

looks good,

@travisn @Madhu-1 by this PR external mode will always have cephfs csi-provisioners. plus more details on commit message

wont this be a change in the behavior on the upgraded/new cluster where only RBD is in use not cephfs? Are we fine with it and also do we need any documentation change for it?

@parth-gr
Copy link
Member

wont this be a change in the behavior on the upgraded/new cluster where only RBD is in use not cephfs? Are we fine with it and also do we need any documentation change for it?

We are thinking to update the ocs-operator config map with this config value as a next step but need a discussion on that.
We would be deleting it from rook config with this PR and adding it to the ocs operator config in the same release, wdyt?

@Madhu-1
Copy link
Member

Madhu-1 commented Jan 20, 2025

wont this be a change in the behavior on the upgraded/new cluster where only RBD is in use not cephfs? Are we fine with it and also do we need any documentation change for it?

We are thinking to update the ocs-operator config map with this config value as a next step but need a discussion on that. We would be deleting it from rook config with this PR and adding it to the ocs operator config in the same release, wdyt?

IMO as long as the behavior remains the same we are good.

@iamniting
Copy link
Member Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 84fdc5a into red-hat-storage:main Jan 21, 2025
11 checks passed
@openshift-cherrypick-robot
Copy link

@iamniting: new pull request created: #2971

In response to this:

/cherry-pick release-4.18

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jan 21, 2025
Merged
@parth-gr parth-gr mentioned this pull request Jan 21, 2025
Merged
6 tasks
@travisn
Copy link
Contributor

travisn commented Jan 21, 2025

wont this be a change in the behavior on the upgraded/new cluster where only RBD is in use not cephfs? Are we fine with it and also do we need any documentation change for it?

We are thinking to update the ocs-operator config map with this config value as a next step but need a discussion on that. We would be deleting it from rook config with this PR and adding it to the ocs operator config in the same release, wdyt?

IMO as long as the behavior remains the same we are good.

If the user desires to disable the CephFS CSI driver, now how do they do it? It seems this PR is forcing the cephfs csi driver to be enabled and the user is not allowed to override it? The configmap settings should be controlled by the user, but maybe I'm missing something.

@parth-gr
Copy link
Member

parth-gr commented Jan 22, 2025 •
edited
Loading

If the user desires to disable the CephFS CSI driver, now how do they do it? It seems this PR is forcing the cephfs csi driver to be enabled and the user is not allowed to override it? The configmap settings should be controlled by the user, but maybe I'm missing something.

It's correct with this PR we have force enabled the cephfs driver,
But now we are thinking of enabling disabling the cephfs driver from user input using the ocs operator config map and rook operator env variable...

@iamniting iamniting mentioned this pull request Jan 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parth-gr parth-gr parth-gr left review comments

@malayparida2000 malayparida2000 malayparida2000 approved these changes

Assignees

@malayparida2000 malayparida2000

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@iamniting @openshift-cherrypick-robot @Madhu-1 @parth-gr @travisn @malayparida2000