-
Notifications
You must be signed in to change notification settings - Fork 273
fix: propagate Accepted=False to Ready condition when GitRepo job creation fails #5318
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,6 +145,12 @@ func (r *StatusReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr | |
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // If the Accepted condition is False (e.g. missing secret, cabundle failure, | ||
| // restriction violation), propagate that failure to the Ready condition. | ||
| // Without this, a GitRepo with no bundles (because the job never ran) would | ||
| // report Ready=True because the empty bundle summary satisfies 0/0 == ready. | ||
| propagateAcceptedFailureToReady(gitrepo) | ||
|
|
||
| if err := r.updateStatus(ctx, orig, gitrepo); err != nil { | ||
| logger.Error(err, "Reconcile failed update to git repo status", "status", gitrepo.Status) | ||
| return ctrl.Result{RequeueAfter: durations.GitRepoStatusDelay}, nil | ||
|
|
@@ -247,6 +253,49 @@ bundles: | |
| return nil | ||
| } | ||
|
|
||
| // propagateAcceptedFailureToReady ensures the Ready condition reflects an | ||
| // Accepted=False error. When a pre-job error (missing secret, cabundle failure, | ||
| // restriction violation) prevents any bundle from being created, the bundle | ||
| // summary is 0/0, which would normally resolve to Ready=True. By copying the | ||
| // Accepted condition's message onto Ready we surface the real error. | ||
| func propagateAcceptedFailureToReady(gitrepo *fleet.GitRepo) { | ||
| var acceptedMsg string | ||
| var acceptedReason string | ||
| acceptedFalse := false | ||
| for _, c := range gitrepo.Status.Conditions { | ||
| if c.Type == fleet.GitRepoAcceptedCondition && c.Status == v1.ConditionFalse { | ||
| acceptedFalse = true | ||
| acceptedMsg = c.Message | ||
| acceptedReason = c.Reason | ||
| break | ||
| } | ||
| } | ||
| if !acceptedFalse { | ||
| return | ||
| } | ||
|
|
||
| found := false | ||
| newConditions := make([]genericcondition.GenericCondition, 0, len(gitrepo.Status.Conditions)) | ||
| for _, c := range gitrepo.Status.Conditions { | ||
| if c.Type == string(fleet.Ready) { | ||
| c.Status = v1.ConditionFalse | ||
| c.Message = acceptedMsg | ||
| c.Reason = acceptedReason | ||
| found = true | ||
| } | ||
| newConditions = append(newConditions, c) | ||
| } | ||
| if !found { | ||
| newConditions = append(newConditions, genericcondition.GenericCondition{ | ||
| Type: string(fleet.Ready), | ||
| Status: v1.ConditionFalse, | ||
| Message: acceptedMsg, | ||
| Reason: acceptedReason, | ||
| }) | ||
| } | ||
| gitrepo.Status.Conditions = newConditions | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this invalidate/remove all other conditions that may be existing on this object? Not that familiar with the API to judge, but I feel like it would be a more clean approach. |
||
| } | ||
|
|
||
| type forcedDelayingSource[R comparable] struct { | ||
| source.TypedSource[R] | ||
| delay time.Duration | ||
|
|
||
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.
I'm not sure this could introduce an undesired
LastUpdateTimeupdate.Let's say the first iteration of the status reconciler computes the desired state and, because there are no
Bundlesit considersReadyshould betrue.Later this new function propagates the message and reason from the
Acceptedcondition and status is nowFalse.That's fine.
The problem I see (I would need to confirm testing it myself) is that for the next reconcile it re-checks again, there are no Bundles and the
Readycondition is found asFalse.When that happens, wrangler updates the
LastUpdateTimewhen it sets the condition back toTrue.Later the new function only preserves the message and reason, so the
LastUpdateTimehas been updated (when it shouldn't).I think the tests are not exercising this particular scenario. I think it should be also covered
Also, what happens if the
Readycondition was alreadyFalse?We would be overwritting unconditionally the message and reason to the
Acceptedvalues, right?