-
Notifications
You must be signed in to change notification settings - Fork 184
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.
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
Unset CephBlockPool mirroring if Mirroring spec is nil on SC #2937
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vbnrh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold |
What we could do is,
@umangachapagain @vbnrh WDYT? And with converged when we move to use the mirroring controller to manage mirroring/secret exchange, we could remove enabling mirroring from the storageCluster controller (cephblockpool.go) |
1cdaff0
to
e0e36f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other places in this file where we are enabling mirroring, please make sure that we follow the same in other places as well
deploymentType, ok := storageCluster.Annotations["ocs.openshift.io/deployment-mode"] | ||
if !ok { | ||
return fmt.Errorf("deployment mode annotation not found") | ||
} | ||
|
||
// Since provider mode handles mirroring, we only need to handle for converged mode | ||
if deploymentType != "provider" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have the annotation for other modes
deploymentType, ok := storageCluster.Annotations["ocs.openshift.io/deployment-mode"] | |
if !ok { | |
return fmt.Errorf("deployment mode annotation not found") | |
} | |
// Since provider mode handles mirroring, we only need to handle for converged mode | |
if deploymentType != "provider" { | |
// Since provider mode handles mirroring, we only need to handle for internal mode | |
if storageCluster.Annotations["ocs.openshift.io/deployment-mode"] != "provider" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annotation is guaranteed to be there in all cases ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will be present only in provider mode, but if we query an empty map with a key that is not present it will return an empty value
Signed-off-by: vbadrina <vbadrina@redhat.com>
Were you able to verify it on a cluster? |
Not tested it but should work. |
It would be good if you could verify it so we don't miss any edge cases. Maybe add the annotation to the internal Mode cluster and check that storageCluster doesn't reconcile the mirroring field. Anyway, I will un-hold the PR. |
@rewantsoni pls be aware that most if not all the traces of provider mode will be removed and this specific annotation will also go away starting in #2936 (4.19) |
https://issues.redhat.com/browse/DFBUGS-1185