diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index b8dc5904..88415b78 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -234,7 +234,8 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( clusterRule.Spec.GitTargetRef.Namespace, clusterRule.Spec.GitTargetRef.Name, ) - return commitRule(ctx, st, ruleReadiness(clusterRule.Status.Conditions, "ClusterWatchRule", msg)) + return commitRule(ctx, st, + ruleReadiness(clusterRule.Status.Conditions, gitTargetStalled(target), "ClusterWatchRule", msg)) } // gateClusterWatchRule is the ClusterWatchRule gate and the ONE place this controller compiles a diff --git a/internal/controller/gittarget_dependency_status.go b/internal/controller/gittarget_dependency_status.go index c6779ef0..5c46197a 100644 --- a/internal/controller/gittarget_dependency_status.go +++ b/internal/controller/gittarget_dependency_status.go @@ -12,6 +12,19 @@ import ( configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) +// gitTargetStalled is the GitTarget's own verdict that it will not converge without a human: Ready +// is published and not True, and Stalled is True. A rule stalls on this verdict rather than on a +// list of the reasons behind it, so a reason the GitTarget learns to stall on reaches its rules +// without a second edit here. +func gitTargetStalled(target configbutleraiv1alpha3.GitTarget) bool { + ready := findCondition(target.Status.Conditions, GitTargetConditionReady) + if ready == nil || ready.Status == metav1.ConditionTrue { + return false + } + stalled := findCondition(target.Status.Conditions, GitTargetConditionStalled) + return stalled != nil && stalled.Status == metav1.ConditionTrue +} + func gitTargetReadyCondition(target configbutleraiv1alpha3.GitTarget) conditionValue { if ready := findCondition(target.Status.Conditions, GitTargetConditionReady); ready != nil { if ready.Status == metav1.ConditionTrue { @@ -21,8 +34,8 @@ func gitTargetReadyCondition(target configbutleraiv1alpha3.GitTarget) conditionV Message: fmt.Sprintf("GitTarget %s/%s is ready", target.Namespace, target.Name), } } - if stalled := findCondition(target.Status.Conditions, GitTargetConditionStalled); stalled != nil && - stalled.Status == metav1.ConditionTrue { + if gitTargetStalled(target) { + stalled := findCondition(target.Status.Conditions, GitTargetConditionStalled) reason, message := conditionReasonMessage(stalled, ReasonFailed, "GitTarget is stalled") return conditionValue{Status: metav1.ConditionFalse, Reason: reason, Message: message} } @@ -52,10 +65,14 @@ func gitTargetReadyCondition(target configbutleraiv1alpha3.GitTarget) conditionV // object that contradicts itself for minutes, with StreamsRunning already back to True because the // stream-state channel told it so directly. That made the mirrored condition unusable for alerting. // -// Only Status and Reason are compared. Both are low-cardinality and are what a printer column or an -// alert keys on, whereas the mirrored Message is free-form and moves with transient stream detail; -// including it would re-enqueue every rule of a target on churn that changes no verdict. A -// message-only drift self-corrects on the next Status or Reason move. +// Only Status, Reason and the Stalled verdict are compared. All three are low-cardinality and are +// what a printer column or an alert keys on, whereas the mirrored Message is free-form and moves +// with transient stream detail; including it would re-enqueue every rule of a target on churn that +// changes no verdict. A message-only drift self-corrects on the next move of any of the three. +// +// The verdict is compared on its own because one reason can be either: the apply-trigger gate +// reports the same reason as a stall or as progress, so a target can flip Stalled while its mirrored +// Status and Reason stay put, and the rule judges its stall from that verdict (see ruleReadiness). func gitTargetReadyProjectionChanged() predicate.Predicate { return predicate.Funcs{ CreateFunc: func(event.CreateEvent) bool { return true }, @@ -72,7 +89,8 @@ func gitTargetReadyProjectionChanged() predicate.Predicate { } before := gitTargetReadyCondition(*oldTarget) after := gitTargetReadyCondition(*newTarget) - return before.Status != after.Status || before.Reason != after.Reason + return before.Status != after.Status || before.Reason != after.Reason || + gitTargetStalled(*oldTarget) != gitTargetStalled(*newTarget) }, } } diff --git a/internal/controller/gittarget_dependency_status_test.go b/internal/controller/gittarget_dependency_status_test.go index fb3551df..f0fc8f08 100644 --- a/internal/controller/gittarget_dependency_status_test.go +++ b/internal/controller/gittarget_dependency_status_test.go @@ -70,6 +70,20 @@ func TestGitTargetReadyProjectionChanged(t *testing.T) { ObjectNew: gitTargetWithReady(1, metav1.ConditionTrue, GitTargetReasonOK, "ready"), }), "the first Ready publish must fire") + // One reason can be a stall or progress (the apply-trigger gate reports it either way), so a + // Stalled flip under an unchanged Status and Reason is a move the rule has to see: it stalls on + // that verdict, not on the reason. + progressing := gitTargetWithReady(1, metav1.ConditionFalse, "SourceMismatch", "applier reads another source") + stalled := gitTargetWithReady(1, metav1.ConditionFalse, "SourceMismatch", "applier reads another source") + stalled.Status.Conditions = append(stalled.Status.Conditions, metav1.Condition{ + Type: GitTargetConditionStalled, Status: metav1.ConditionTrue, Reason: "SourceMismatch", + Message: "applier reads another source", + }) + assert.True(t, p.Update(event.UpdateEvent{ObjectOld: progressing, ObjectNew: stalled}), + "a Stalled flip under the same Status and Reason must fire") + assert.True(t, p.Update(event.UpdateEvent{ObjectOld: stalled, ObjectNew: progressing}), + "a stall that clears under the same Status and Reason must fire") + assert.True(t, p.Create(event.CreateEvent{})) assert.True(t, p.Delete(event.DeleteEvent{})) assert.False(t, p.Generic(event.GenericEvent{})) diff --git a/internal/controller/stream_status.go b/internal/controller/stream_status.go index 310e18a6..80a6c675 100644 --- a/internal/controller/stream_status.go +++ b/internal/controller/stream_status.go @@ -52,8 +52,13 @@ func newRuleReadiness(kind, readyMessage string) *readiness { // source namespace is authorized. ClusterWatchRule simply carries no SourceNamespaceAuthorized // condition, so that gate is absent rather than special-cased. // +// gitTargetStalled is the GitTarget's own Stalled verdict (see gitTargetStalled). The mirrored +// GitTargetReady condition carries only the reason, and judging a stall from a list of reasons kept +// here fell behind every time the GitTarget learned a new one: a rule on a target stalled with +// AmbiguousLayout or ParentBranchNotFound read as progressing for as long as it stayed broken. +// // The order of the contributions below IS the precedence; see readiness. -func ruleReadiness(conditions []metav1.Condition, kind, readyMessage string) *readiness { +func ruleReadiness(conditions []metav1.Condition, gitTargetStalled bool, kind, readyMessage string) *readiness { rd := newRuleReadiness(kind, readyMessage) resources := findCondition(conditions, ConditionTypeResourcesResolved) @@ -68,7 +73,7 @@ func ruleReadiness(conditions []metav1.Condition, kind, readyMessage string) *re // deliberately non-terminal "cannot say yet", handled among the progressing gates below. contributeStalled(rd, sourceNS) contributeStalled(rd, resources) - if gitTarget != nil && gitTargetReadyReasonIsStalled(gitTarget.Reason) { + if gitTarget != nil && gitTargetStalled { contributeStalled(rd, gitTarget) } if streams != nil && streamReasonIsStalled(streams.Reason) { @@ -112,31 +117,6 @@ func streamReasonIsStalled(reason string) bool { return reason == watch.StreamReasonWatchError || reason == watch.StreamReasonWatchNotPermitted } -func gitTargetReadyReasonIsStalled(reason string) bool { - switch reason { - case GitTargetReasonProviderNotFound, - GitTargetReasonBranchNotAllowed, - GitTargetReasonTargetConflict, - GitTargetReasonUnsupportedContent, - GitTargetReasonIgnoreShadowsManagedPath, - GitTargetReasonWriteBoundaryRefused, - GitTargetReasonMultipleSourceNamespaces, - GitTargetReasonUnrenderedPlacement, - GitTargetReasonRenderDoesNotMatchLive, - GitTargetReadyReasonValidationFailed, - GitTargetReadyReasonEncryptionNotConfigured, - GitTargetReadyReasonWorkerUnavailable, - WatchRuleReasonGitTargetNotFound, - WatchRuleReasonGitProviderNotFound, - WatchRuleReasonGitDestinationInvalid, - watch.StreamReasonWatchError, - watch.StreamReasonWatchNotPermitted: - return true - default: - return false - } -} - // commitRule writes the trio, persists the status, and picks the requeue cadence from the same // verdict, so the cadence can never disagree with what status says. // diff --git a/internal/controller/stream_status_test.go b/internal/controller/stream_status_test.go index ad181e31..07af241b 100644 --- a/internal/controller/stream_status_test.go +++ b/internal/controller/stream_status_test.go @@ -19,6 +19,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/applier" "github.com/ConfigButler/gitops-reverser/internal/watch" ) @@ -43,7 +44,7 @@ func TestRuleReadiness_GitTargetReadyStalledBlocksRule(t *testing.T) { Message: "1/1 streams running", }, } - trio := ruleReadiness(conditions, "WatchRule", "rule ready").trio() + trio := ruleReadiness(conditions, true, "WatchRule", "rule ready").trio() assert.Equal(t, metav1.ConditionFalse, trio.Ready.Status) assert.Equal(t, metav1.ConditionFalse, trio.Reconciling.Status) @@ -51,6 +52,65 @@ func TestRuleReadiness_GitTargetReadyStalledBlocksRule(t *testing.T) { assert.Equal(t, GitTargetReasonUnsupportedContent, trio.Stalled.Reason) } +// TestRuleReadiness_StallsOnTheGitTargetsVerdictWhateverItsReason runs the path a rule reconcile +// takes, from the GitTarget object to the rule's trio. A rule used to stall only on reasons it kept a +// list of, and the list fell behind: AmbiguousLayout, ParentBranchNotFound and SourceMismatch all +// stall a GitTarget, and a rule on such a target read as progressing for as long as it stayed broken. +func TestRuleReadiness_StallsOnTheGitTargetsVerdictWhateverItsReason(t *testing.T) { + stalledTarget := func(reason string) configbutleraiv1alpha3.GitTarget { + return configbutleraiv1alpha3.GitTarget{Status: configbutleraiv1alpha3.GitTargetStatus{ + Conditions: []metav1.Condition{ + {Type: GitTargetConditionReady, Status: metav1.ConditionFalse, Reason: reason, Message: "broken"}, + {Type: GitTargetConditionReconciling, Status: metav1.ConditionFalse, Reason: reason}, + {Type: GitTargetConditionStalled, Status: metav1.ConditionTrue, Reason: reason, Message: "broken"}, + }, + }} + } + rechecking := GitTargetReasonRenderRechecking + progressingTarget := configbutleraiv1alpha3.GitTarget{Status: configbutleraiv1alpha3.GitTargetStatus{ + Conditions: []metav1.Condition{ + {Type: GitTargetConditionReady, Status: metav1.ConditionFalse, Reason: rechecking}, + {Type: GitTargetConditionReconciling, Status: metav1.ConditionTrue, Reason: rechecking}, + {Type: GitTargetConditionStalled, Status: metav1.ConditionFalse, Reason: ReasonProgressing}, + }, + }} + + tests := []struct { + name string + target configbutleraiv1alpha3.GitTarget + wantStalled metav1.ConditionStatus + wantReason string + }{ + {"refused folder", stalledTarget(GitTargetReasonAmbiguousLayout), + metav1.ConditionTrue, GitTargetReasonAmbiguousLayout}, + {"no parent branch", stalledTarget(GitTargetReasonParentBranchNotFound), + metav1.ConditionTrue, GitTargetReasonParentBranchNotFound}, + {"apply trigger on another source", stalledTarget(applier.ReasonSourceMismatch), + metav1.ConditionTrue, applier.ReasonSourceMismatch}, + {"a reason this package has never heard of", stalledTarget("SomeFutureReason"), + metav1.ConditionTrue, "SomeFutureReason"}, + {"a target still converging", progressingTarget, + metav1.ConditionFalse, GitTargetReasonRenderRechecking}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ready := gitTargetReadyCondition(tt.target) + streaming := watch.StreamReasonAllStreamsReady + conditions := []metav1.Condition{ + {Type: ConditionTypeResourcesResolved, Status: metav1.ConditionTrue, Reason: ReasonSucceeded}, + {Type: ConditionTypeGitTargetReady, Status: ready.Status, Reason: ready.Reason, Message: ready.Message}, + {Type: ConditionTypeStreamsRunning, Status: metav1.ConditionTrue, Reason: streaming}, + } + + trio := ruleReadiness(conditions, gitTargetStalled(tt.target), "WatchRule", "rule ready").trio() + + assert.Equal(t, metav1.ConditionFalse, trio.Ready.Status, "Ready") + assert.Equal(t, tt.wantStalled, trio.Stalled.Status, "Stalled") + assert.Equal(t, tt.wantReason, trio.Ready.Reason, "Ready reason") + }) + } +} + // TestCommitRule_LostWriteBeatsTheConvergingLoop covers the path a rule takes while it is still // coming up, which is where a lost write hurts most: a CONVERGING rule already polls on the fast // stream-settle loop, so returning that cadence directly looks close enough to correct and is not. diff --git a/internal/controller/watchrule_controller.go b/internal/controller/watchrule_controller.go index 415c80da..c7a5adcc 100644 --- a/internal/controller/watchrule_controller.go +++ b/internal/controller/watchrule_controller.go @@ -220,7 +220,7 @@ func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( targetNS, watchRule.Spec.GitTargetRef.Name, ) - return commitRule(ctx, st, ruleReadiness(watchRule.Status.Conditions, "WatchRule", msg)) + return commitRule(ctx, st, ruleReadiness(watchRule.Status.Conditions, gitTargetStalled(target), "WatchRule", msg)) } // stallRule publishes a terminal WatchRule outcome and ends the reconcile. diff --git a/internal/controller/watchrule_kstatus_test.go b/internal/controller/watchrule_kstatus_test.go index b8876157..e7d1009d 100644 --- a/internal/controller/watchrule_kstatus_test.go +++ b/internal/controller/watchrule_kstatus_test.go @@ -145,7 +145,7 @@ func TestRuleReadiness_SourceAuthorizationIsAPrerequisite(t *testing.T) { conditions = append(conditions, *tt.sourceNS) } - trio := ruleReadiness(conditions, "WatchRule", "ready").trio() + trio := ruleReadiness(conditions, false, "WatchRule", "ready").trio() assert.Equal(t, tt.wantReady, trio.Ready.Status, "Ready") assert.Equal(t, tt.wantStalled, trio.Stalled.Status, "Stalled") diff --git a/internal/manifestanalyzer/acceptance_refusal.go b/internal/manifestanalyzer/acceptance_refusal.go index bad37b72..f7bca60e 100644 --- a/internal/manifestanalyzer/acceptance_refusal.go +++ b/internal/manifestanalyzer/acceptance_refusal.go @@ -148,8 +148,8 @@ func (e *AcceptanceRefusedError) AllIssuesOfKinds(kinds ...IssueKind) bool { // with three comments elsewhere asking callers to keep their strings in sync with it by hand. // // The strings mirror the controller's GitTargetReason* constants — neither package can import -// controller without a cycle — and every one of them is a member of the controller's -// stalled-reason set, so a refusal surfaces as Stalled=True / kstatus Failed whichever it is. +// controller without a cycle. Whichever one a refusal gets, the GitTarget publishes it as +// Stalled=True / kstatus Failed, and its rules stall on that verdict, not on the reason. func GitPathRefusalReason(refused *AcceptanceRefusedError) string { switch { case refused.AllIssuesOfKinds(IssueIgnoreShadowsManaged):