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 1881595..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,17 +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) } - cond := meta.FindStatusCondition(n.Status.Conditions, ConditionAddressConflict) - if cond == nil { - t.Fatalf("one pass must stamp the condition on %s", name) + 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) } - if got := cond.Status == metav1.ConditionTrue; got != conflicted { - t.Fatalf("%s: conflicted=%v, want %v (%+v)", name, got, conflicted, 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) + } + if meta.FindStatusCondition(n.Status.Conditions, ConditionAddressConflict) == nil { + t.Errorf("%s must still be stamped after the failure on %s", name, nodeA) } } } @@ -125,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. @@ -160,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) } @@ -179,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))