From 809337ed1fb64cab7417af207f27060d776b1da0 Mon Sep 17 00:00:00 2001 From: Scott Dodson Date: Sun, 28 Jun 2026 22:15:26 -0400 Subject: [PATCH 1/2] status: sort isUpgrading messages to prevent ClusterOperator write loop isUpgrading() iterates over a map[string]string to build upgrade status messages. Go map iteration order is randomized, so when multiple operands are upgrading simultaneously (e.g. coredns and kube-rbac-proxy), the returned messages slice has a non-deterministic order on each call. This non-determinism caused a self-sustaining write loop during upgrades: 1. isUpgrading() produces messages in order [A, B] 2. operatorStatusesEqual() returns false (Message differs from stored) 3. ClusterOperator is written 4. The CO watch fires, triggering another reconcile 5. isUpgrading() produces messages in order [B, A] 6. operatorStatusesEqual() returns false again 7. ClusterOperator is written again -> goto 4 The result was the ClusterOperator being updated many times per second for the entire duration of an upgrade, as observed via 'oc get co -w'. Fix by sorting the messages slice before returning from isUpgrading(), making the output deterministic and breaking the loop. Adds a unit test that calls isUpgrading() 100 times with multiple upgrading components and asserts stable message ordering. rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- pkg/operator/controller/status/controller.go | 2 + .../controller/status/controller_test.go | 43 +++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/pkg/operator/controller/status/controller.go b/pkg/operator/controller/status/controller.go index bc88a8c9d..29a601b9a 100644 --- a/pkg/operator/controller/status/controller.go +++ b/pkg/operator/controller/status/controller.go @@ -3,6 +3,7 @@ package status import ( "context" "fmt" + "sort" "strings" "github.com/google/go-cmp/cmp" @@ -532,6 +533,7 @@ func isUpgrading(curVersions, oldVersions, newVersions map[string]string) (bool, messages = append(messages, fmt.Sprintf("Upgrading %s to %q.", name, newVersion)) } } + sort.Strings(messages) return upgrading, messages } diff --git a/pkg/operator/controller/status/controller_test.go b/pkg/operator/controller/status/controller_test.go index c5ece2791..713af989a 100644 --- a/pkg/operator/controller/status/controller_test.go +++ b/pkg/operator/controller/status/controller_test.go @@ -1,6 +1,7 @@ package status import ( + "reflect" "testing" "time" @@ -802,3 +803,45 @@ func TestComputeOperatorDegradedCondition(t *testing.T) { } } } + +// TestIsUpgradingMessageOrder verifies that isUpgrading returns messages in a +// stable, sorted order regardless of map iteration order. Non-deterministic +// message ordering caused a self-sustaining ClusterOperator write loop: each +// reconcile produced a different Message string, operatorStatusesEqual returned +// false, the ClusterOperator was written, the CO watch fired, and the cycle +// repeated indefinitely during upgrades. +func TestIsUpgradingMessageOrder(t *testing.T) { + // Two components upgrading simultaneously — map iteration order is random, + // so call isUpgrading many times and confirm the messages slice is always + // returned in the same sorted order. + curVersions := map[string]string{ + OperatorVersionName: "v1", + CoreDNSVersionName: "dns-v1", + KubeRBACProxyName: "rbac-v1", + } + oldVersions := map[string]string{ + OperatorVersionName: "v0", + CoreDNSVersionName: "dns-v0", + KubeRBACProxyName: "rbac-v0", + } + newVersions := map[string]string{ + OperatorVersionName: "v2", + CoreDNSVersionName: "dns-v2", + KubeRBACProxyName: "rbac-v2", + } + + var first []string + for i := 0; i < 100; i++ { + upgrading, messages := isUpgrading(curVersions, oldVersions, newVersions) + if !upgrading { + t.Fatal("expected upgrading=true") + } + if i == 0 { + first = messages + continue + } + if !reflect.DeepEqual(first, messages) { + t.Errorf("iteration %d: messages order changed\n first: %v\n current: %v", i, first, messages) + } + } +} From b31c7bbce98bc55ae7e3d61b17d8242ffda7a1ad Mon Sep 17 00:00:00 2001 From: Scott Dodson Date: Sun, 28 Jun 2026 22:25:20 -0400 Subject: [PATCH 2/2] status: assert explicit sorted slice in TestIsUpgradingMessageOrder The previous assertion compared each isUpgrading() result against the first observed slice. If the first call happened to return messages in a non-sorted order (e.g. after a future regression), subsequent calls matching that same bad order would still pass the test. Replace the first-observed baseline with an explicit, hardcoded expected slice in lexicographic order. This directly asserts the sorting contract rather than merely checking consistency across calls. rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED --- .../controller/status/controller_test.go | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/operator/controller/status/controller_test.go b/pkg/operator/controller/status/controller_test.go index 713af989a..92c88c85a 100644 --- a/pkg/operator/controller/status/controller_test.go +++ b/pkg/operator/controller/status/controller_test.go @@ -811,9 +811,6 @@ func TestComputeOperatorDegradedCondition(t *testing.T) { // false, the ClusterOperator was written, the CO watch fired, and the cycle // repeated indefinitely during upgrades. func TestIsUpgradingMessageOrder(t *testing.T) { - // Two components upgrading simultaneously — map iteration order is random, - // so call isUpgrading many times and confirm the messages slice is always - // returned in the same sorted order. curVersions := map[string]string{ OperatorVersionName: "v1", CoreDNSVersionName: "dns-v1", @@ -830,18 +827,27 @@ func TestIsUpgradingMessageOrder(t *testing.T) { KubeRBACProxyName: "rbac-v2", } - var first []string + // expectedMessages is the lexicographically sorted set of messages + // isUpgrading must produce for the above inputs. "Upgraded" sorts before + // "Upgrading" (byte 8: 'd' < 'i'), and within each prefix components are + // ordered alphabetically: coredns < kube-rbac-proxy < operator. + expectedMessages := []string{ + `Upgraded coredns to "dns-v1".`, + `Upgraded kube-rbac-proxy to "rbac-v1".`, + `Upgraded operator to "v1".`, + `Upgrading coredns to "dns-v2".`, + `Upgrading kube-rbac-proxy to "rbac-v2".`, + `Upgrading operator to "v2".`, + } + + // Call isUpgrading many times to exercise Go's randomized map iteration. for i := 0; i < 100; i++ { upgrading, messages := isUpgrading(curVersions, oldVersions, newVersions) if !upgrading { t.Fatal("expected upgrading=true") } - if i == 0 { - first = messages - continue - } - if !reflect.DeepEqual(first, messages) { - t.Errorf("iteration %d: messages order changed\n first: %v\n current: %v", i, first, messages) + if !reflect.DeepEqual(messages, expectedMessages) { + t.Errorf("iteration %d: unexpected messages\n want: %v\n got: %v", i, expectedMessages, messages) } } }