fix: don't treat in-progress binding states as terminal failures (#670)

ytimocin · claude · web-flow · commit 447629b09f73 · 2026-04-30T16:24:34.000-07:00
HasBindingFailed previously returned true for any condition with Status=False, regardless of the Reason. Several in-progress reasons on the per-cluster binding conditions (Overridden, WorkSynchronized, Applied, Available) are expressed as Status=False rather than Status=Unknown — most notably WorkNotSynchronizedYetReason and NotAvailableYetReason. The over-broad classification caused upstream controllers (rollout, updaterun) to bail on bindings that were still making forward progress. Switch HasBindingFailed to consult an explicit allowlist of non-terminal reasons (nonTerminalBindingFailureReasons) drawn from the condition package: OverriddenPendingReason, WorkNotSynchronizedYetReason, ApplyPendingReason, NotAvailableYetReason. Anything outside that set is treated as terminal — the safer default, since wrongly classifying an unknown reason as "transient" would stall rollouts forever. Per code review feedback (the original draft used a `strings.Contains` on the literal "failed", which is fragile against typos and convention drift), the allowlist uses typed constants. New "Pending" / "NotYet" reasons added to pkg/utils/condition must also be added to the allowlist; this is documented inline. Tests: - TestHasBindingFailed_NonTerminalReasons covers each known transient reason returning false, each known terminal reason returning true, an unknown-reason fall-through to terminal, and a mixed-condition case where a transient reason on one condition does not mask a terminal failure on another. - Existing TestHasBindingFailed (which uses camelCase reason strings not in the allowlist) still passes — those legacy strings are correctly classified as terminal. - TestCheckClusterUpdateResult in the updaterun package was encoding the previous bug: it asserted that WorkNotSynchronizedYet should produce an error from checkClusterUpdateResult. Updated the case to match corrected semantics (in-progress, no error) and added a new case for the terminal SyncWorkFailedReason path. This aligns with the existing TODO at execution.go:626 that called for distinguishing recoverable from non-recoverable failures. Fixes #648 Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com> Co-authored-by: Claude Code <noreply@anthropic.com>
diff --git a/pkg/controllers/updaterun/execution_test.go b/pkg/controllers/updaterun/execution_test.go
@@ -242,7 +242,9 @@ func TestCheckClusterUpdateResult(t *testing.T) {
 			wantErr:       true,
 		},
 		{
-			name: "checkClusterUpdateResult should return false and error if the binding has false workSynchronized condition",
+			// WorkNotSynchronizedYet is an in-progress reason, not a terminal failure (issue #648).
+			// checkClusterUpdateResult now treats it as "still updating" rather than an error.
+			name: "checkClusterUpdateResult should return false but no error if workSynchronized is in-progress (WorkNotSynchronizedYet)",
 			binding: &placementv1beta1.ClusterResourceBinding{
 				ObjectMeta: metav1.ObjectMeta{Generation: 1},
 				Status: placementv1beta1.ResourceBindingStatus{
@@ -258,6 +260,25 @@ func TestCheckClusterUpdateResult(t *testing.T) {
 			},
 			clusterStatus: &placementv1beta1.ClusterUpdatingStatus{ClusterName: "test-cluster"},
 			wantSucceeded: false,
+			wantErr:       false,
+		},
+		{
+			name: "checkClusterUpdateResult should return false and error if workSynchronized failed terminally (SyncWorkFailed)",
+			binding: &placementv1beta1.ClusterResourceBinding{
+				ObjectMeta: metav1.ObjectMeta{Generation: 1},
+				Status: placementv1beta1.ResourceBindingStatus{
+					Conditions: []metav1.Condition{
+						{
+							Type:               string(placementv1beta1.ResourceBindingWorkSynchronized),
+							Status:             metav1.ConditionFalse,
+							ObservedGeneration: 1,
+							Reason:             condition.SyncWorkFailedReason,
+						},
+					},
+				},
+			},
+			clusterStatus: &placementv1beta1.ClusterUpdatingStatus{ClusterName: "test-cluster"},
+			wantSucceeded: false,
 			wantErr:       true,
 		},
 		{
diff --git a/pkg/utils/binding/binding.go b/pkg/utils/binding/binding.go
@@ -23,14 +23,51 @@ import (
 	"github.com/kubefleet-dev/kubefleet/pkg/utils/condition"
 )
 
-// HasBindingFailed checks if BindingObj has failed based on its conditions.
+// nonTerminalBindingFailureReasons is the closed set of `Reason` strings that the per-cluster
+// binding conditions (Overridden, WorkSynchronized, Applied, Available) can carry while still
+// `Status=False` *without* representing a terminal failure. Each one means the binding is still
+// progressing and the controller should keep waiting rather than treat the binding as failed.
+//
+// The set is intentionally an allowlist: any new `Status=False` reason added to the codebase is
+// treated as terminal until it is explicitly added here. This is the safer default because the
+// alternative (treat unknown reasons as transient) would stall the rollout controller forever
+// on a genuinely-failing binding it does not yet know how to classify.
+//
+// When you add a new in-progress / pending / "not yet" reason to pkg/utils/condition/reason.go
+// for any of the above conditions, also add it here.
+var nonTerminalBindingFailureReasons = map[string]struct{}{
+	condition.OverriddenPendingReason:      {},
+	condition.WorkNotSynchronizedYetReason: {},
+	condition.ApplyPendingReason:           {},
+	condition.NotAvailableYetReason:        {},
+}
+
+// HasBindingFailed reports whether a binding has reached a terminal failure on any of its
+// per-cluster conditions (Overridden, WorkSynchronized, Applied, Available).
+//
+// A condition counts as a terminal failure when its `Status` is `False` *and* its `Reason` is
+// not in `nonTerminalBindingFailureReasons`. The Reason check is necessary because several of
+// the in-progress states (e.g. `WorkNotSynchronizedYetReason`, `NotAvailableYetReason`) are
+// expressed as `Status=False` rather than `Status=Unknown`. Treating those as failures was the
+// previous bug — it caused the rollout controller to drop bindings that were still progressing
+// out of `canBeReadyBindings`, stalling rollout decisions.
+//
+// Unknown reasons fall through to "terminal" by design; see the comment on
+// `nonTerminalBindingFailureReasons`.
 func HasBindingFailed(binding placementv1beta1.BindingObj) bool {
 	for i := condition.OverriddenCondition; i <= condition.AvailableCondition; i++ {
-		if condition.IsConditionStatusFalse(binding.GetCondition(string(i.ResourceBindingConditionType())), binding.GetGeneration()) {
-			// TODO: parse the reason of the condition to see if the failure is recoverable/retriable or not
-			klog.V(2).Infof("binding %s has condition %s with status false", klog.KObj(binding), string(i.ResourceBindingConditionType()))
-			return true
+		c := binding.GetCondition(string(i.ResourceBindingConditionType()))
+		if !condition.IsConditionStatusFalse(c, binding.GetGeneration()) {
+			continue
+		}
+		if _, transient := nonTerminalBindingFailureReasons[c.Reason]; transient {
+			klog.V(3).Infof("binding %s has condition %s status false with non-terminal reason %q; treating as in-progress",
+				klog.KObj(binding), string(i.ResourceBindingConditionType()), c.Reason)
+			continue
 		}
+		klog.V(2).Infof("binding %s has terminal failure on condition %s (reason %q)",
+			klog.KObj(binding), string(i.ResourceBindingConditionType()), c.Reason)
+		return true
 	}
 	return false
 }
diff --git a/pkg/utils/binding/binding_test.go b/pkg/utils/binding/binding_test.go
@@ -22,6 +22,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1"
+	"github.com/kubefleet-dev/kubefleet/pkg/utils/condition"
 )
 
 func TestHasBindingFailed(t *testing.T) {
@@ -326,6 +327,102 @@ func TestHasBindingFailed(t *testing.T) {
 	}
 }
 
+// TestHasBindingFailed_NonTerminalReasons verifies that conditions with Status=False but a
+// known in-progress reason (WorkNotSynchronizedYet, NotAvailableYet, OverriddenPending,
+// ApplyPending) are NOT treated as failures. This is the bug fix for issue #648.
+func TestHasBindingFailed_NonTerminalReasons(t *testing.T) {
+	bindingWithCondition := func(condType string, reason string) *placementv1beta1.ClusterResourceBinding {
+		return &placementv1beta1.ClusterResourceBinding{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-binding", Generation: 1},
+			Status: placementv1beta1.ResourceBindingStatus{
+				Conditions: []metav1.Condition{
+					{
+						Type:               condType,
+						Status:             metav1.ConditionFalse,
+						ObservedGeneration: 1,
+						Reason:             reason,
+						Message:            "in progress",
+					},
+				},
+			},
+		}
+	}
+
+	tests := []struct {
+		name    string
+		binding *placementv1beta1.ClusterResourceBinding
+		want    bool
+	}{
+		{
+			name:    "WorkSynchronized=False with WorkNotSynchronizedYet is in-progress, not a failure",
+			binding: bindingWithCondition(string(placementv1beta1.ResourceBindingWorkSynchronized), condition.WorkNotSynchronizedYetReason),
+			want:    false,
+		},
+		{
+			name:    "Available=False with NotAvailableYet is in-progress, not a failure",
+			binding: bindingWithCondition(string(placementv1beta1.ResourceBindingAvailable), condition.NotAvailableYetReason),
+			want:    false,
+		},
+		{
+			name:    "Overridden=False with OverriddenPending is in-progress, not a failure",
+			binding: bindingWithCondition(string(placementv1beta1.ResourceBindingOverridden), condition.OverriddenPendingReason),
+			want:    false,
+		},
+		{
+			name:    "Applied=False with ApplyPending is in-progress, not a failure",
+			binding: bindingWithCondition(string(placementv1beta1.ResourceBindingApplied), condition.ApplyPendingReason),
+			want:    false,
+		},
+		{
+			name:    "Applied=False with ApplyFailed is a terminal failure",
+			binding: bindingWithCondition(string(placementv1beta1.ResourceBindingApplied), condition.ApplyFailedReason),
+			want:    true,
+		},
+		{
+			name:    "Overridden=False with OverriddenFailed is a terminal failure",
+			binding: bindingWithCondition(string(placementv1beta1.ResourceBindingOverridden), condition.OverriddenFailedReason),
+			want:    true,
+		},
+		{
+			name:    "Status=False with an unknown reason defaults to terminal (safer default)",
+			binding: bindingWithCondition(string(placementv1beta1.ResourceBindingApplied), "SomeReasonNobodyAddedToTheAllowlist"),
+			want:    true,
+		},
+		{
+			name: "non-terminal reason on one condition does not mask a terminal failure on another",
+			binding: &placementv1beta1.ClusterResourceBinding{
+				ObjectMeta: metav1.ObjectMeta{Name: "test-binding", Generation: 1},
+				Status: placementv1beta1.ResourceBindingStatus{
+					Conditions: []metav1.Condition{
+						{
+							Type:               string(placementv1beta1.ResourceBindingOverridden),
+							Status:             metav1.ConditionFalse,
+							ObservedGeneration: 1,
+							Reason:             condition.OverriddenPendingReason,
+						},
+						{
+							Type:               string(placementv1beta1.ResourceBindingApplied),
+							Status:             metav1.ConditionFalse,
+							ObservedGeneration: 1,
+							Reason:             condition.ApplyFailedReason,
+						},
+					},
+				},
+			},
+			want: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := HasBindingFailed(tc.binding)
+			if got != tc.want {
+				t.Errorf("HasBindingFailed(%v) = %v, want %v", tc.binding, got, tc.want)
+			}
+		})
+	}
+}
+
 func TestIsBindingReportDiff(t *testing.T) {
 	tests := []struct {
 		name    string
