Add custom informer for ManagedClusterAddOns #73
Conversation
Signed-off-by: fxiang1 <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
- Coverage 65.86% 59.84% -6.02%
==========================================
Files 2 3 +1
Lines 706 924 +218
==========================================
+ Hits 465 553 +88
- Misses 218 340 +122
- Partials 23 31 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: fxiang1 <[email protected]>
| // Start the custom informer in a goroutine | ||
| go func() { | ||
| if err := customInformer.Start(); err != nil { | ||
| log.Log.Error(err, "Failed to start custom ManagedClusterAddOn informer") |
There was a problem hiding this comment.
We might need to exit/panic here or mca won't be watched and it will be hard to detect afterwards.
Does it make sense to retry a few times then exit out?
There was a problem hiding this comment.
Thanks Mike! Yes, the AI suggested to retry.
Yes, it makes sense to add retry logic here! The informer's Start() method can fail if the cache doesn't sync, which could happen due to:
1. Temporary API server unavailability during startup
2. Network issues
3. The ManagedClusterAddOn CRD not being installed yet
| // Call the reconcile function directly | ||
| _, err := r.Reconcile(ctx, req) | ||
| if err != nil { | ||
| log.Error(err, "Failed to reconcile ClusterPermission", "name", cp.Name, "namespace", cp.Namespace) |
There was a problem hiding this comment.
When reconcile error the general pattern should be requeue. I don't see it here or the caller side.
There was a problem hiding this comment.
AI said since we are calling Reconcile manually from the custom informer event handler (not through the normal controller queue), we can only retry here. So AI added retries for this as well 😅
Signed-off-by: fxiang1 <[email protected]>
Signed-off-by: fxiang1 <[email protected]>
Signed-off-by: fxiang1 <[email protected]>
…r-management-io#73) * Red Hat Konflux update cluster-permission-acm-214 Signed-off-by: red-hat-konflux <[email protected]> * Bump Go to 1.23 Signed-off-by: fxiang1 <[email protected]> * Add microdnf update -y Signed-off-by: fxiang1 <[email protected]> --------- Signed-off-by: fxiang1 <[email protected]> Co-authored-by: red-hat-konflux <[email protected]> Co-authored-by: fxiang1 <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1, mikeshng 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 |
|
Merging manually, as codecov is not taking account of the e2e test coverage. |
fxiang1
merged commit 49671db
into
open-cluster-management-io:main
2 participants
https://issues.redhat.com/browse/ACM-24726