Properly handle CARM errors and requeues #140
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this patch we introduced better mechanisms to handle Role ARN retrievals from the CARM cache. Which improved ACK runtime scaling capabilities and addressed possible race condition scenarios. However in the process we missed two things: - Setting an ACK.ResourceSynced condition stating that the resource isn't synced, yet. - Returning the right runtime error that will cause the reconciller to requeue every 15seconds. Both of the points stemmed from the fact that we're not "yet" in the reconcile function that is wrapped by a proper error handler (that triggers requeues, and resets/sets resource conditions). In this special case, we need to manually inject the condition and return a controller-runtime error that will trigger a requeue after 15seconds. While this is a good fix, we're planning on refactoring a lot of the runtime logic to make easier to read, maintain and more importantly expose reusable component that will help avoid falling into such traps. Signed-off-by: Amine Hilaly <[email protected]>
a-hilaly
commented
Feb 29, 2024
| @@ -124,6 +124,7 @@ func (r *adoptionReconciler) reconcile(ctx context.Context, req ctrlrt.Request) | |||
| // is not available. | |||
| roleARN, err = r.getRoleARN(acctID) | |||
| if err != nil { | |||
| ackrtlog.InfoAdoptedResource(r.log, res, fmt.Sprintf("Unable to start adoption reconcilliation %s: %v", acctID, err)) | |||
| // r.getRoleARN errors are not terminal, we should requeue. | |||
| return requeue.NeededAfter(err, roleARNNotAvailableRequeueDelay) | |||
There was a problem hiding this comment.
This requeue works fine, because r.reconcile is wrapped by handleReconcileErrors
|
/test all |
|
/retest |
1 similar comment
|
/retest |
|
/test all |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, yuxiang-zhang 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 |
ack-prow bot
pushed a commit
to aws-controllers-k8s/code-generator
that referenced
this pull request
Mar 5, 2024
Propagating two main patches to all the controllers: - Properly handle CARM errors and requeues by @a-hilaly in aws-controllers-k8s/runtime#140 - Introduce flags for fine-tuning maximum concurrent reconciles per resource by @a-hilaly in aws-controllers-k8s/runtime#141 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
ndbhat
pushed a commit
to ndbhat/ack-runtime
that referenced
this pull request
Apr 16, 2024
Prior to this patch we introduced new mechanisms to handle Role ARN retrievals from the CARM cache. Which improved ACK runtime scaling capabilities and addressed possible race condition scenarios. However in the process we missed two things: - Setting an `ACK.ResourceSynced` condition stating that the resource isn't synced, yet. - Returning the **correct** runtime error that will cause the reconciller to requeue every 15seconds. Both of the problems stemmed from the fact that we're not "yet" in the reconcile function (`rm.Sync`) that is wrapped by a proper error handler (that triggers requeues, and resets/sets resource conditions). In this special case, we need to manually inject the condition and return a controller-runtime error that will trigger a requeue after 15seconds. While this is a "fair" fix, we're planning on refactoring a lot of the runtime logic to make easier to read, maintain and more importantly expose reusable component that will help avoid falling into such traps. Signed-off-by: Amine Hilaly <[email protected]> By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this patch we introduced new mechanisms to handle Role ARN
retrievals from the CARM cache. Which improved ACK runtime scaling
capabilities and addressed possible race condition scenarios. However in
the process we missed two things:
ACK.ResourceSyncedcondition stating that the resourceisn't synced, yet.
requeue every 15seconds.
Both of the problems stemmed from the fact that we're not "yet" in the
reconcile function (
rm.Sync) that is wrapped by a proper error handler(that triggers requeues, and resets/sets resource conditions). In this
special case, we need to manually inject the condition and return a
controller-runtime error that will trigger a requeue after 15seconds.
While this is a "fair" fix, we're planning on refactoring a lot of the
runtime logic to make easier to read, maintain and more importantly
expose reusable component that will help avoid falling into such traps.
Signed-off-by: Amine Hilaly [email protected]
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.