From 7ba348810d4e91eb5531f8b86145455b3f10b307 Mon Sep 17 00:00:00 2001 From: Devin Buhl Date: Wed, 29 Jul 2026 14:24:40 -0400 Subject: [PATCH 1/2] test(topology): keep the condition deref inside the nil guard staticcheck only knows t.Fatalf terminates when the testing package facts are part of the run; with a restored golangci-lint cache they are not always there, and SA5011 fires on the deref that follows. Moving it into an else branch makes the guard structural. Signed-off-by: Devin Buhl --- internal/topology/conflict_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/topology/conflict_test.go b/internal/topology/conflict_test.go index 1881595..916828d 100644 --- a/internal/topology/conflict_test.go +++ b/internal/topology/conflict_test.go @@ -100,8 +100,7 @@ func TestConflictSinglePassCoversAllNodes(t *testing.T) { cond := meta.FindStatusCondition(n.Status.Conditions, ConditionAddressConflict) if cond == nil { t.Fatalf("one pass must stamp the condition on %s", name) - } - if got := cond.Status == metav1.ConditionTrue; got != conflicted { + } else if got := cond.Status == metav1.ConditionTrue; got != conflicted { t.Fatalf("%s: conflicted=%v, want %v (%+v)", name, got, conflicted, cond) } } From b3414f2d354873b9f3023f9dd491080ad1f43cd9 Mon Sep 17 00:00:00 2001 From: Devin Buhl Date: Thu, 30 Jul 2026 10:50:29 -0400 Subject: [PATCH 2/2] fix(ci): key the lint cache on the toolchain and drop the prefix fallback golangci-lint caches analyzer facts, so a stale or partial entry does not just cost time: it turns staticcheck's SA5011 into a false positive on untouched code, which is what went red in the merge queue. The key now covers .mise/mise.lock, so a golangci-lint or Go bump starts fresh instead of inheriting the previous binary's facts, and restore-keys is gone so a miss starts cold instead of falling back to the newest entry under a bare prefix; that fallback is what let one bad entry outlive its own commit. With the input fixed, the test goes back to the file's own short-circuit idiom instead of the else branch added in the previous commit: it needs no rationale comment and matches the other condition assertions in the package. It also compares the status exactly rather than folding Unknown in with False, and reports every node instead of aborting on whichever the randomized map order reached first. Also in the workflow: cancelled now counts as failed in the Build Success gate. !cancelled() still runs that job when individual needs were cancelled but the run was not, so a required job that never reached a verdict could clear branch protection. And the surrounding topology and agent code the failure led through: - Eventf passed an already-formatted message as the printf format string in two places. client-go does fmt.Sprintf(note, args...), so every PoolUsageHigh warning reached operators as "pool 85%!f(MISSING)ull (warn at 80%!)(MISSING)". - AddressConflict never set ObservedGeneration, leaving a stale True indistinguishable from a current one. - One node's failed status update aborted the whole conflict pass, leaving every later node unstamped and so invisible to placement. Per-node errors are now collected and joined, as the sibling node group reconciler already does. - The node group enqueue map func swallowed its List error, dropping the event with no requeue and no trace. Signed-off-by: Devin Buhl --- .github/workflows/ci.yaml | 17 ++++-- internal/agent/poolstats.go | 3 +- internal/agent/poolstats_test.go | 7 ++- internal/topology/conflict.go | 26 +++++---- internal/topology/conflict_test.go | 86 ++++++++++++++++++++++++++---- internal/topology/nodegroup.go | 3 ++ 6 files changed, 116 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 7711409..519d8f5 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -364,12 +364,18 @@ jobs: key: ${{ runner.os }}-go-${{ hashFiles('go.sum') }} restore-keys: ${{ runner.os }}-go- + # This cache holds analyzer facts (staticcheck's "does this call + # return?"), so a stale or partial entry does not just cost time -- it + # turns checks like SA5011 into false positives on untouched code. Hence + # the lock file in the key, so a golangci-lint or Go bump starts fresh + # rather than inheriting the previous binary's facts, and no restore-keys: + # a miss must start cold instead of falling back to the newest entry + # under the prefix, which is how one bad entry outlives its own commit. - name: Cache golangci-lint uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: path: ~/.cache/golangci-lint - key: ${{ runner.os }}-golangci-lint-${{ hashFiles('go.sum', '.golangci.yml') }} - restore-keys: ${{ runner.os }}-golangci-lint- + key: ${{ runner.os }}-golangci-lint-${{ hashFiles('go.sum', '.golangci.yml', '.mise/mise.lock') }} - name: Run linter run: mise run lint @@ -516,12 +522,15 @@ jobs: runs-on: ubuntu-24.04 permissions: {} steps: + # cancelled counts as failed: !cancelled() above still runs this job when + # individual needs were cancelled but the run was not, and a required job + # that never reached a verdict must not clear the merge gate. - name: Any jobs failed? - if: ${{ contains(needs.*.result, 'failure') }} + if: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }} run: | exit 1 - name: All jobs passed or skipped? - if: ${{ !(contains(needs.*.result, 'failure')) }} + if: ${{ !(contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')) }} run: | echo "All jobs passed or skipped" && echo "${{ toJSON(needs.*.result) }}" diff --git a/internal/agent/poolstats.go b/internal/agent/poolstats.go index 7fa3542..d00c814 100644 --- a/internal/agent/poolstats.go +++ b/internal/agent/poolstats.go @@ -179,7 +179,8 @@ func (p *PoolStatsPublisher) publish(ctx context.Context) error { return err } if high && !wasHigh && p.Recorder != nil { - p.Recorder.Eventf(cur, nil, corev1.EventTypeWarning, ConditionPoolUsageHigh, "Sample", msg) + // The message is data, not a format string: it embeds usage percentages. + p.Recorder.Eventf(cur, nil, corev1.EventTypeWarning, ConditionPoolUsageHigh, "Sample", "%s", msg) } return nil }) diff --git a/internal/agent/poolstats_test.go b/internal/agent/poolstats_test.go index 89ab158..60a4581 100644 --- a/internal/agent/poolstats_test.go +++ b/internal/agent/poolstats_test.go @@ -194,7 +194,12 @@ func TestPoolStatsPublisherRaisesHighDataUsage(t *testing.T) { t.Fatalf("expected DataUsageHigh True at 85%%, got %+v", c) } select { - case <-rec.Events: + case e := <-rec.Events: + // The note is Eventf's format string, and this message carries usage + // percentages: it has to reach the operator verbatim. + if !strings.Contains(e, c.Message) { + t.Fatalf("the event must carry the condition message %q, got %q", c.Message, e) + } default: t.Fatal("expected a PoolUsageHigh event on first crossing") } diff --git a/internal/topology/conflict.go b/internal/topology/conflict.go index 3cf3e5d..bb8dd65 100644 --- a/internal/topology/conflict.go +++ b/internal/topology/conflict.go @@ -22,6 +22,7 @@ package topology import ( "context" + "errors" "fmt" "slices" "strings" @@ -79,24 +80,28 @@ func (r *ConflictReconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctr return ctrl.Result{}, err } topo := nodemap.FromNodes(list.Items) + // One node's failed update — a 409 against the agent's per-minute status + // writes, say — must not leave the rest of the fleet unstamped and so + // invisible to placement, so per-node errors are collected and joined. + // The requeue replays the whole pass: SetStatusCondition skips the nodes + // that did land, so the retry is cheap and emits no duplicate events. + var errs []error for i := range list.Items { - // A mid-pass failure retries the whole pass: SetStatusCondition - // skips already-updated nodes, so the retry is cheap and emits no - // duplicate events. if err := r.reconcileNode(ctx, &list.Items[i], topo); err != nil { - return ctrl.Result{}, err + errs = append(errs, fmt.Errorf("node %s: %w", list.Items[i].Name, err)) } } - return ctrl.Result{}, nil + return ctrl.Result{}, errors.Join(errs...) } // reconcileNode updates one node's AddressConflict condition on change. func (r *ConflictReconciler) reconcileNode(ctx context.Context, node *miroirv1alpha1.MiroirNode, topo nodemap.Map) error { cond := metav1.Condition{ - Type: ConditionAddressConflict, - Status: metav1.ConditionFalse, - Reason: reasonAddressUnique, - Message: "replication address is unique (or unset)", + Type: ConditionAddressConflict, + Status: metav1.ConditionFalse, + Reason: reasonAddressUnique, + Message: "replication address is unique (or unset)", + ObservedGeneration: node.Generation, } if topo[node.Name].AddressConflict { // Group by the same canonical key the fold conflicts on: a raw @@ -127,8 +132,9 @@ func (r *ConflictReconciler) reconcileNode(ctx context.Context, node *miroirv1al return client.IgnoreNotFound(err) } if cond.Status == metav1.ConditionTrue && !wasConflicted && r.Recorder != nil { + // The message is data, not a format string: it embeds spec.address. r.Recorder.Eventf(node, nil, corev1.EventTypeWarning, ConditionAddressConflict, "Reconcile", - cond.Message) + "%s", cond.Message) } return nil } diff --git a/internal/topology/conflict_test.go b/internal/topology/conflict_test.go index 916828d..c508e52 100644 --- a/internal/topology/conflict_test.go +++ b/internal/topology/conflict_test.go @@ -17,6 +17,8 @@ limitations under the License. package topology import ( + "context" + "errors" "strings" "testing" @@ -29,6 +31,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" miroirv1alpha1 "github.com/home-operations/miroir/api/v1alpha1" ) @@ -92,16 +95,55 @@ func TestConflictSinglePassCoversAllNodes(t *testing.T) { }); err != nil { t.Fatal(err) } - for name, conflicted := range map[string]bool{nodeA: true, nodeB: true, nodeC: false} { + want := map[string]metav1.ConditionStatus{ + nodeA: metav1.ConditionTrue, + nodeB: metav1.ConditionTrue, + nodeC: metav1.ConditionFalse, + } + // Errorf, not Fatalf: map iteration order is randomized, so aborting on + // the first unstamped node would report a different one each run and hide + // the rest — the blast radius is the whole point of this test. + for name, status := range want { + n := &miroirv1alpha1.MiroirNode{} + if err := c.Get(t.Context(), types.NamespacedName{Name: name}, n); err != nil { + t.Fatal(err) + } + if cond := meta.FindStatusCondition(n.Status.Conditions, ConditionAddressConflict); cond == nil || + cond.Status != status { + t.Errorf("one pass must stamp AddressConflict=%s on %s, got %+v", status, name, cond) + } + } +} + +// One node's failed status update must not leave the rest of the fleet +// unstamped and so invisible to placement: the other nodes are still +// stamped and the error surfaces for the requeue. +func TestConflictOneBadNodeDoesNotBlockTheFleet(t *testing.T) { + base := newClient(t, addrNode(nodeA, "10.0.100.1"), addrNode(nodeB, "10.0.100.1"), + addrNode(nodeC, "10.0.100.3")) + // Stand in for the 409 the agent's per-minute status writes hand this + // reconciler mid-pass. The list is name-ordered, so node-a fails first. + c := interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, cl client.Client, sub string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if obj.GetName() == nodeA { + return errors.New("the object has been modified; please apply your changes to the latest version") + } + return cl.SubResource(sub).Update(ctx, obj, opts...) + }, + }) + + r := &ConflictReconciler{Client: c} + _, err := r.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Name: topologyRequestKey}}) + if err == nil || !strings.Contains(err.Error(), nodeA) { + t.Fatalf("the per-node failure must surface with the node named, got %v", err) + } + for _, name := range []string{nodeB, nodeC} { n := &miroirv1alpha1.MiroirNode{} if err := c.Get(t.Context(), types.NamespacedName{Name: name}, n); err != nil { t.Fatal(err) } - cond := meta.FindStatusCondition(n.Status.Conditions, ConditionAddressConflict) - if cond == nil { - t.Fatalf("one pass must stamp the condition on %s", name) - } else if got := cond.Status == metav1.ConditionTrue; got != conflicted { - t.Fatalf("%s: conflicted=%v, want %v (%+v)", name, got, conflicted, cond) + if meta.FindStatusCondition(n.Status.Conditions, ConditionAddressConflict) == nil { + t.Errorf("%s must still be stamped after the failure on %s", name, nodeA) } } } @@ -124,6 +166,19 @@ func TestConflictConditionRaisedAndNamesPeer(t *testing.T) { } } +// The condition records the generation it was computed from, so a True left +// behind by a failed or not-yet-run pass is distinguishable from a current +// one on a node whose address the operator just edited. +func TestConflictConditionRecordsObservedGeneration(t *testing.T) { + a := addrNode(nodeA, "10.0.100.1") + a.Generation = 7 + n := reconcile(t, newClient(t, a, addrNode(nodeB, "10.0.100.1")), nodeA) + if cond := meta.FindStatusCondition(n.Status.Conditions, ConditionAddressConflict); cond == nil || + cond.ObservedGeneration != a.Generation { + t.Fatalf("expected observedGeneration %d, got %+v", a.Generation, cond) + } +} + // The fold conflicts on the parsed address; the peer list must group the // same way, or equal-but-differently-spelled IPv6 addresses raise a // condition naming no peer. @@ -159,16 +214,27 @@ func TestConflictConditionClearsWhenResolved(t *testing.T) { } // The Warning event fires once per fresh conflict — repeat passes over an -// unchanged (or message-only-changed) topology must stay silent. +// unchanged topology, and passes that only rewrite an existing conflict's +// peer list, must stay silent. func TestConflictEventFiresOncePerFreshConflict(t *testing.T) { c := newClient(t, addrNode(nodeA, "10.0.100.1"), addrNode(nodeB, "10.0.100.1")) rec := events.NewFakeRecorder(8) r := &ConflictReconciler{Client: c, Recorder: rec} - req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "topology"}} + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: topologyRequestKey}} if _, err := r.Reconcile(t.Context(), req); err != nil { t.Fatal(err) } + // A third node on the same address rewrites node-a's and node-b's peer + // list while their status stays True: the condition changes, so only the + // wasConflicted gate keeps them quiet. node-c is the one fresh conflict. + if err := c.Create(t.Context(), addrNode(nodeC, "10.0.100.1")); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(t.Context(), req); err != nil { + t.Fatal(err) + } + // Unchanged topology: no condition changes at all. if _, err := r.Reconcile(t.Context(), req); err != nil { t.Fatal(err) } @@ -178,7 +244,7 @@ func TestConflictEventFiresOncePerFreshConflict(t *testing.T) { for range rec.Events { fired++ } - if fired != 2 { // one per freshly conflicted node, none on the repeat - t.Fatalf("want exactly one event per fresh conflict (2), got %d", fired) + if fired != 3 { // one per freshly conflicted node, none on the repeats + t.Fatalf("want exactly one event per fresh conflict (3), got %d", fired) } } diff --git a/internal/topology/nodegroup.go b/internal/topology/nodegroup.go index 45c7749..f4e3159 100644 --- a/internal/topology/nodegroup.go +++ b/internal/topology/nodegroup.go @@ -234,6 +234,9 @@ func (r *NodeGroupReconciler) SetupWithManager(mgr ctrl.Manager) error { func(ctx context.Context, _ client.Object) []ctrl.Request { groups := &miroirv1alpha1.MiroirNodeGroupList{} if err := r.List(ctx, groups, client.UnsafeDisableDeepCopy); err != nil { + // A map func has nothing to requeue against, so the event is + // lost: log it rather than let the controller look idle. + ctrl.LoggerFrom(ctx).Error(err, "listing node groups to enqueue") return nil } reqs := make([]ctrl.Request, 0, len(groups.Items))