controllers: deploy omap sidecar only when required #286
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rewantsoni 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 |
Signed-off-by: Rewant Soni <[email protected]>
rewantsoni
force-pushed
the
csi-omap
branch
2 times, most recently
from
5ddf9d4 to
c5254ea
Compare
move from always deploying the omap side car to deploy it only when required Signed-off-by: Rewant Soni <[email protected]>
| mirrorEnabledChangedPredicate := predicate.Funcs{ | ||
| UpdateFunc: func(e event.UpdateEvent) bool { | ||
| if e.ObjectOld == nil || e.ObjectNew == nil { | ||
| return false | ||
| } | ||
| oldObj := e.ObjectOld.(*v1alpha1.StorageClient) | ||
| newObj := e.ObjectNew.(*v1alpha1.StorageClient) | ||
| return oldObj.Status.MirrorEnabled != newObj.Status.MirrorEnabled | ||
| }, | ||
| } |
There was a problem hiding this comment.
We usually do not predicate on a single field in the spec but on the entire spec using a generation change predicate (as we consider this an overoptimization), especially when considering CRs that are almost singletons.
There was a problem hiding this comment.
Here, we are storing the MirroringEnabled flag in the status of the storageClient CR, I want the operatorConfigmap controller to trigger a reconciliation only when the field changes. I could watch for the entire status but I think it's better to be specific here
There was a problem hiding this comment.
I am not sure if a status update affects the generation of a CR or not. But either way, this seems like the wrong thing to watch
| for idx := range storageClients.Items { | ||
| if storageClients.Items[idx].Status.MirrorEnabled { | ||
| return true, nil | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
If we are to generate omap data in cases where a single client requests it, how would that affect the other clients that don't need it?
There was a problem hiding this comment.
If any client is requesting for the omap info, we are going to enable it, as this is a operator level operation we cannot restrict it to per client
There was a problem hiding this comment.
I understand, but I was asking about the impact/the side effects. Not the procedure
There was a problem hiding this comment.
The impact would be that we enable it for all other clients as well. I don't think we have any side effects if it is always enabled
| @@ -207,6 +207,7 @@ func (r *StorageClientReconciler) reconcilePhases() (ctrl.Result, error) { | |||
|
|
|||
| if storageClientResponse.SystemAttributes != nil { | |||
| r.storageClient.Status.InMaintenanceMode = storageClientResponse.SystemAttributes.SystemInMaintenanceMode | |||
| r.storageClient.Status.MirrorEnabled = storageClientResponse.SystemAttributes.MirrorEnabled | |||
There was a problem hiding this comment.
Because we are talking about the status section (where we reflect the actual state and not the desired state) I am not sure it is correct to reflect that the client is mirror-enabled before validating that the omap generator is running. In addition shouldn't we reflect similar things for other clients? shouldn't this be more granular?
There was a problem hiding this comment.
Here, I want to convey that the client is enabled for mirroring on the provider side. Hence storing the actual state in the Status, the desired state would be in the storage-client-mapping in the Provider side
There was a problem hiding this comment.
We have an issue here, status should not be treated as desired state and should be considered as something that can be deleted at any point in time.
Instead of always deploying the Omap sidecar, only deploy it when required.
For each client, fetch if Mirroring is enabled on the provider and then deploy the Omap sidecar if any client has mirroring enabled
Depends On: red-hat-storage/ocs-operator#2914