Add watching on Secrets in drcluster_controller #1167
Conversation
ELENAGER
requested review from
ShyamsundarR,
raghavendra-talur and
BenamarMk
ELENAGER
force-pushed
the
bug_2248666_watch_secrets
branch
from
5ce49d6
to
7aff605
Compare
| Watches(&source.Kind{Type: &corev1.Secret{}}, | ||
| handler.EnqueueRequestsFromMapFunc(r.drClusterSecretMapFunc), | ||
| builder.WithPredicates(util.CreateOrDeleteOrResourceVersionUpdatePredicate{}), | ||
| ). |
There was a problem hiding this comment.
This is little bit inconsistent with other code - in other watches we have a function (e.g. mcvMapFun) and predicate (e.g. mcvPred). Here we have harder to follow structure.
There was a problem hiding this comment.
Not exactly, here is the same structure
I moved the other watch with the same structure, hope it looks more clear now
| @@ -141,6 +145,24 @@ func (r *DRClusterReconciler) drClusterConfigMapMapFunc(configMap client.Object) | |||
| return requests | |||
| } | |||
|
|
|||
| func (r *DRClusterReconciler) drClusterSecretMapFunc(secret client.Object) []reconcile.Request { | |||
| if secret.GetNamespace() != NamespaceName() { | |||
There was a problem hiding this comment.
Not clear what is this namespace. Not an issue in your change but maybe it is time to make this more clear.
| @@ -141,6 +145,24 @@ func (r *DRClusterReconciler) drClusterConfigMapMapFunc(configMap client.Object) | |||
| return requests | |||
| } | |||
|
|
|||
| func (r *DRClusterReconciler) drClusterSecretMapFunc(secret client.Object) []reconcile.Request { | |||
| if secret.GetNamespace() != NamespaceName() { | |||
| return []reconcile.Request{} | |||
There was a problem hiding this comment.
Why not return nil? (empty slices is nil)
There was a problem hiding this comment.
I am returning it for the consistency with the other code. The same return can be found in 5 more places in this file.
| return []reconcile.Request{} | ||
| } | ||
|
|
||
| drcusters := &ramen.DRClusterList{} |
There was a problem hiding this comment.
We don't need to create the DRClusterList on the heap - we can do:
drclusters := ramen.DRClusterList{}
r.Client.List(context.TODO(), &drcusters)
There was a problem hiding this comment.
You are probably right, but it's also for consistency. We are always creating lists on heap (I've looked through all our code).
controllers/drcluster_controller.go
Outdated
|
|
||
| requests := make([]reconcile.Request, len(drcusters.Items)) | ||
| for i, drcluster := range drcusters.Items { | ||
| requests[i].Name = drcluster.GetName() |
There was a problem hiding this comment.
Doesn't this loop copy all drclusters from drcusters.Items? We can avoid the copy if we do:
for i := range drcusters.Items {
requests[i].Name = drclusters.Items[i].GetName()
}
There was a problem hiding this comment.
I was thinking of a envtest and looked for parallels in DRPolicy, which also does not have a test for a secret event (though it is covered in other ways as per the coverage report).
Otherwise this looks good. In case we still want to address the optimizations that nir suggested we can amend it accordingly.
| return []reconcile.Request{} | ||
| } | ||
|
|
||
| requests := make([]reconcile.Request, len(drcusters.Items)) |
There was a problem hiding this comment.
Instead of reconciling all DRClusters we may want to filter out only those that reference the secret via their S3Profile name. The same non-optimized version exists here as well, so let's add a TODO/issue at least to keep this tracked.
There was a problem hiding this comment.
I added an optimization issue
ELENAGER
force-pushed
the
bug_2248666_watch_secrets
branch
from
21457cd
to
4d99c40
Compare
Signed-off-by: Elena Gershkovich <[email protected]>
ELENAGER
force-pushed
the
bug_2248666_watch_secrets
branch
from
4d99c40
to
445d8a1
Compare
Upon Hub recovery, it takes time till secrets are copied. While secrets are still not copied, drcluster_controller fails to find them several times and goes to exponential backoff. When secrets are finally copied, drcluster_controller is not notified, because it was not registered for it.
Watching on Secrets added
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2248666