-
Notifications
You must be signed in to change notification settings - Fork 17
Description
tl;dr:
Whenever the fsmReconciler fails to applyOutputs(), the object's Ready status is always unexpectedly True 🙀
Details
The "Ready" condition's status is always set to:
Trueif all other conditions areTrue; or,Falseif any other condition isFalse
However, when the fsmReconciler fails to applyOutputs(), no False condition is ever set within the returned conditions!
achilles-sdk/pkg/fsm/internal/reconciler.go
Lines 288 to 295 in 23adbe7
| if err := r.applyOutputs(ctx, log, obj, out); err != nil { | |
| return obj, conditions, types.ErrorResult(fmt.Errorf("applying outputs: %w", err)) | |
| } | |
| // accumulate status conditions, overwrites duplicate conditions with those of later states | |
| if !condition.IsEmpty() { | |
| conditions.SetConditions(condition) | |
| } |
As a result, the resource's Ready status will always show as True despite this failure.
Proposed Fix
All other code paths through the main loop seem to apply the current condition to conditions before returning:
-
When the result is not done:
achilles-sdk/pkg/fsm/internal/reconciler.go
Lines 272 to 280 in 23adbe7
// mark the state's condition as failed if not done or the state signals a requeue after FSM completion if !result.IsDone() { // falsify condition if provided, set message and reason if !condition.IsEmpty() { condition.Status = corev1.ConditionFalse condition.Message, condition.Reason = result.GetMessageAndReason() conditions.SetConditions(condition) } return obj, conditions, result.WrapError(fmt.Sprintf("transitioning state %q", currentState.Name)) -
When the result is done, and the outputs are applied successfully:
achilles-sdk/pkg/fsm/internal/reconciler.go
Lines 292 to 295 in 23adbe7
// accumulate status conditions, overwrites duplicate conditions with those of later states if !condition.IsEmpty() { conditions.SetConditions(condition) }
So perhaps failures applying outputs should take any non-empty condition and:
- Falsify the status
- Set the reason and message to something indicating the failure to apply resources
- Set that onto
conditionsbefore returning
That would align the condition handling with the other code paths and ensure that output failures don't cause the resource to report an "incorrect" status of being ready.