Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) }}"
3 changes: 2 additions & 1 deletion internal/agent/poolstats.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand Down
7 changes: 6 additions & 1 deletion internal/agent/poolstats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
26 changes: 16 additions & 10 deletions internal/topology/conflict.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package topology

import (
"context"
"errors"
"fmt"
"slices"
"strings"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
85 changes: 75 additions & 10 deletions internal/topology/conflict_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package topology

import (
"context"
"errors"
"strings"
"testing"

Expand All @@ -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"
)
Expand Down Expand Up @@ -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)
}
}
}
Expand All @@ -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.
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
}
3 changes: 3 additions & 0 deletions internal/topology/nodegroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down