From 4f9fef2a98e163567c17b0ef6bbab8ed4080a0b0 Mon Sep 17 00:00:00 2001 From: Ilker Manap Date: Wed, 16 Sep 2026 23:36:35 +0200 Subject: [PATCH] Make settling outlast an agent reconnect backoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The window worked on the cluster and settled on the wrong answer. After a restart node05 registered in one second and the other four arrived seventeen seconds later, because a disconnected agent reconnects on a backoff that reaches thirty seconds. A five-second steady period called a fleet of one settled and resumed placement while four nodes were still coming — the exact situation the window exists to prevent, reached by being impatient about it. Nothing was misplaced this time only because the model that wanted placing needed two nodes on a fabric and could not have it. With a single-node model it would have landed on node05 and stayed there. A minute now, and steady for half of it rather than a quarter. Adoption is unaffected, so a restart still recovers everything already running immediately; only new placement waits. The test that covers it drives Reconcile the way the ticker does, because the node count is noticed on a pass rather than the instant a node appears — which the first version of the test got wrong and hid. Co-Authored-By: Claude Opus 5 (1M context) --- internal/control/reconciler/reconciler.go | 16 +++++-- .../control/reconciler/reconciler_test.go | 43 ++++++++++++++++++- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/internal/control/reconciler/reconciler.go b/internal/control/reconciler/reconciler.go index 9a72137..5e7c977 100644 --- a/internal/control/reconciler/reconciler.go +++ b/internal/control/reconciler/reconciler.go @@ -80,7 +80,9 @@ type Config struct { // // Adoption still runs throughout; only placement waits. The window ends // early once the fleet stops growing, so a small cluster does not sit idle - // for the whole duration. + // for the whole duration — but not too early: an agent that lost its + // connection reconnects on a backoff reaching thirty seconds, so "no new + // node for a few seconds" means nothing. SettleFor time.Duration } @@ -92,7 +94,7 @@ func DefaultConfig() Config { MaxBackoff: 10 * time.Minute, UnreachableAfter: 15 * time.Second, LostAfter: 60 * time.Second, - SettleFor: 20 * time.Second, + SettleFor: 60 * time.Second, DrainGrace: 5 * time.Minute, PortRange: [2]int{20000, 20999}, MgmtInterface: "eth0", @@ -280,7 +282,15 @@ func (r *Reconciler) fleetHasSettled() bool { if n == 0 { return false } - settled := now.Sub(r.steadySince) >= r.cfg.SettleFor/4 + // Half the window, not a quarter of it, and the window is a minute. + // + // Measured on the cluster: after a restart node05 registered in one second + // and the other four arrived seventeen seconds later, because a + // disconnected agent reconnects on a backoff that reaches thirty seconds. + // A five-second steady period declared a fleet of one settled and resumed + // placement while four nodes were still backing off — the exact situation + // this exists to avoid, reached by being impatient about it. + settled := now.Sub(r.steadySince) >= r.cfg.SettleFor/2 if settled { r.log.Info("fleet settled; placement resuming", "nodes", n, "waited", now.Sub(r.startedAt).Round(time.Millisecond)) diff --git a/internal/control/reconciler/reconciler_test.go b/internal/control/reconciler/reconciler_test.go index ffc4e19..646cb75 100644 --- a/internal/control/reconciler/reconciler_test.go +++ b/internal/control/reconciler/reconciler_test.go @@ -1052,8 +1052,9 @@ func TestPlacementWaitsForTheFleetToArrive(t *testing.T) { t.Fatalf("placed %d shards the instant the last node appeared", got) } - // Steady for a moment: now it may act, and on the whole fleet. - c.advance(DefaultConfig().SettleFor / 4) + // Steady for long enough that a reconnect backoff would have finished: now + // it may act, and on the whole fleet. + c.advance(DefaultConfig().SettleFor / 2) for _, id := range []string{"1", "2", "3", "4"} { touchAt(inv, id, c.now()) } @@ -1114,3 +1115,41 @@ func TestAdoptionIsNotBlockedByTheSettlingWindow(t *testing.T) { t.Fatalf("expected the replica to be adopted, got %d", got) } } + +func TestSettlingOutlastsAnAgentReconnectBackoff(t *testing.T) { + // The failure this was measured against. After a restart node05 registered + // in one second and the other four arrived seventeen seconds later, because + // a disconnected agent reconnects on a backoff reaching thirty seconds. A + // short steady period declared a fleet of one settled and resumed placement + // while four nodes were still coming — the exact situation the window + // exists to avoid, reached by being impatient about it. + r, inv, disp, c := settlingHarness(t, testModel(2)) + + addNode(inv, c, gpuNode("first", 8)) + // Fifteen seconds of one node, which a short threshold would call settled. + for range 3 { + c.advance(5 * time.Second) + touchAt(inv, "first", c.now()) + r.Reconcile(t.Context()) + } + if got := len(disp.startedShards()); got != 0 { + t.Fatalf("placed %d shards on one node while others were backing off", got) + } + + // The stragglers arrive, as they did at seventeen seconds. The loop runs on + // a ticker, so the new count is noticed on the next pass, not the instant + // the nodes appear. + for _, id := range []string{"2", "3", "4"} { + addNode(inv, c, gpuNode(id, 8)) + } + r.Reconcile(t.Context()) + + c.advance(DefaultConfig().SettleFor / 2) + for _, id := range []string{"first", "2", "3", "4"} { + touchAt(inv, id, c.now()) + } + r.Reconcile(t.Context()) + if got := len(disp.startedShards()); got == 0 { + t.Fatal("placement never resumed") + } +}