#315: FixedPlacementPolicy now honors the retry loop's unreachable set
CompositePeerLauncher.spawn retries a failed candidate on the next one and
rebuilds PlacementContext "so the policy excludes this profile" (its own
comment), but FixedPlacementPolicy.select never read ctx.unreachable(). Under
the default `fixed` placement policy (used when `placement` is unset or set
to `fixed`), every retry re-picked the same dead default and a second,
healthy, configured profile was never tried. This also covers the wiring-bug
branch (a candidate profile with no owning adapter), which hit the exact same
symptom for the same reason.
Not live on this fleet: fleetd.yaml sets placement: weighted, which already
consults ctx.unreachable() via PlacementPolicyUtil.available(). This is live
only for a deployment that leaves placement unset or sets it to fixed.
Fix is in FixedPlacementPolicy: consult ctx.unreachable() in the same two
places it already consults quarantined/coolingOff (the default check and the
fallback walk over candidates()), and add a fourth reason to the "no
candidate remains" exception. Considered fixing this in
CompositePeerLauncher's retry loop instead (break when select() returns an
already-unreachable profile), but that only fails faster on the same dead
profile — it cannot make the loop advance to a different candidate, because
only the policy decides which candidate is next. The defect is that one
policy implementation does not honor the loop's stated contract, so the fix
belongs in that policy, matching how weighted/round-robin already behave.
Also fixed: the "no reachable worker profile" exception message said
"trying N candidate(s)" where N was unreachable.size(), a count of DISTINCT
profiles (a HashSet dedupes a profile added twice), under wording that reads
as a count of attempts. Reworded to "N distinct candidate(s)" so the count
matches what is measured and the profile list that follows it.
Tests: two new failover tests next to the three existing ones in
CompositePeerLauncherTest (which all use PlacementPolicies.weighted(), which
is why this had no coverage) — one pinned to PlacementPolicies.fixed() for
the unreachable-default case, one for the wiring-bug (no adapter) case.
Mutation-proofed: reverted FixedPlacementPolicy.java, both new tests failed
with the exact bug ("no reachable worker profile available after trying 1
distinct candidate(s): a" / "...c"), then restored the fix.
This commit is contained in:
@@ -385,9 +385,12 @@ public final class CompositePeerLauncher implements PeerLauncher {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// unreachable.size() counts DISTINCT profiles, not attempts (a HashSet dedupes a profile
|
||||||
|
// added twice) — say "distinct" so the count matches the sentence and the profile list that
|
||||||
|
// follows, rather than reading as a count of attempts made (fleetd #315).
|
||||||
throw new PeerUnreachableException(
|
throw new PeerUnreachableException(
|
||||||
"no reachable worker profile available after trying " + unreachable.size()
|
"no reachable worker profile available after trying " + unreachable.size()
|
||||||
+ " candidate(s): " + String.join(", ", unreachable));
|
+ " distinct candidate(s): " + String.join(", ", unreachable));
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -8,7 +8,7 @@ import java.util.List;
|
|||||||
* profile, exactly as {@code CompositePeerLauncher} did before CB-518. This ignores caps and
|
* profile, exactly as {@code CompositePeerLauncher} did before CB-518. This ignores caps and
|
||||||
* reachability so that a pre-existing config behaves identically after upgrade.
|
* reachability so that a pre-existing config behaves identically after upgrade.
|
||||||
*
|
*
|
||||||
* <p>Three exceptions walk past the default instead of returning it unconditionally:
|
* <p>Four exceptions walk past the default instead of returning it unconditionally:
|
||||||
* <ul>
|
* <ul>
|
||||||
* <li>Quarantine (CB-578 stage B): a quarantined default is a credential that just refused on
|
* <li>Quarantine (CB-578 stage B): a quarantined default is a credential that just refused on
|
||||||
* a usage limit, not a transient capacity or reachability concern.
|
* a usage limit, not a transient capacity or reachability concern.
|
||||||
@@ -16,13 +16,21 @@ import java.util.List;
|
|||||||
* ({@code BackendOutagePolicy}) — a separate, shorter-lived source from quarantine. When a
|
* ({@code BackendOutagePolicy}) — a separate, shorter-lived source from quarantine. When a
|
||||||
* profile is both quarantined and cooling off, only the quarantine reason is reported
|
* profile is both quarantined and cooling off, only the quarantine reason is reported
|
||||||
* (exhaustion takes priority), matching {@code CompositePeerLauncher}'s explicit-spawn order.
|
* (exhaustion takes priority), matching {@code CompositePeerLauncher}'s explicit-spawn order.
|
||||||
|
* <li>Unreachable (fleetd #315): {@code CompositePeerLauncher.spawn} retries a failed candidate
|
||||||
|
* on the next one and rebuilds the {@link PlacementContext} so {@code ctx.unreachable()}
|
||||||
|
* names every profile that already failed with {@code PeerUnreachableException} in this same
|
||||||
|
* call. Without this check {@code select} kept handing back the same dead default forever —
|
||||||
|
* the retry loop's own comment says "so the policy excludes this profile", and this is what
|
||||||
|
* makes that true for {@code fixed} too, matching {@code weighted}/{@code round-robin}
|
||||||
|
* (both filter on {@code ctx.unreachable()} via {@link PlacementPolicyUtil#available}).
|
||||||
* <li>Weight 0 (CB-554): {@code fixed} is still automatic selection, so a profile the operator
|
* <li>Weight 0 (CB-554): {@code fixed} is still automatic selection, so a profile the operator
|
||||||
* marked "never auto-select me" ({@code weight <= 0}) must be skipped here exactly as
|
* marked "never auto-select me" ({@code weight <= 0}) must be skipped here exactly as
|
||||||
* {@code weighted}/{@code round-robin} skip it — an explicit {@code fleet_spawn} naming
|
* {@code weighted}/{@code round-robin} skip it — an explicit {@code fleet_spawn} naming
|
||||||
* the profile is unaffected, only this automatic fallback walk.
|
* the profile is unaffected, only this automatic fallback walk.
|
||||||
* </ul>
|
* </ul>
|
||||||
* A fleet where nothing is ever quarantined, cooling off, or weight-0 never exercises any of these
|
* A fleet where nothing is ever quarantined, cooling off, unreachable, or weight-0 never exercises
|
||||||
* paths, so today's behaviour is unchanged.
|
* any of these paths, so today's behaviour is unchanged — in particular, the very first selection
|
||||||
|
* of a spawn call always sees an empty {@code unreachable} set, so the first choice is untouched.
|
||||||
*/
|
*/
|
||||||
final class FixedPlacementPolicy implements PlacementPolicy {
|
final class FixedPlacementPolicy implements PlacementPolicy {
|
||||||
|
|
||||||
@@ -30,12 +38,12 @@ final class FixedPlacementPolicy implements PlacementPolicy {
|
|||||||
public PlacementCandidate select(PlacementContext ctx) {
|
public PlacementCandidate select(PlacementContext ctx) {
|
||||||
String d = ctx.defaultProfile();
|
String d = ctx.defaultProfile();
|
||||||
if (d != null && !d.isBlank() && !ctx.quarantined().contains(d) && !ctx.coolingOff().contains(d)
|
if (d != null && !d.isBlank() && !ctx.quarantined().contains(d) && !ctx.coolingOff().contains(d)
|
||||||
&& !weightExcluded(ctx, d)) {
|
&& !ctx.unreachable().contains(d) && !weightExcluded(ctx, d)) {
|
||||||
return new PlacementCandidate(d, null, 1.0f, null);
|
return new PlacementCandidate(d, null, 1.0f, null);
|
||||||
}
|
}
|
||||||
for (PlacementCandidate c : ctx.candidates()) {
|
for (PlacementCandidate c : ctx.candidates()) {
|
||||||
if (!ctx.quarantined().contains(c.profile()) && !ctx.coolingOff().contains(c.profile())
|
if (!ctx.quarantined().contains(c.profile()) && !ctx.coolingOff().contains(c.profile())
|
||||||
&& !c.excluded()) {
|
&& !ctx.unreachable().contains(c.profile()) && !c.excluded()) {
|
||||||
return new PlacementCandidate(c.profile(), null, c.weight(), c.maxLoad());
|
return new PlacementCandidate(c.profile(), null, c.weight(), c.maxLoad());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -44,8 +52,9 @@ final class FixedPlacementPolicy implements PlacementPolicy {
|
|||||||
// Exhaustion quarantine takes priority: reported only when quarantine is absent, so the
|
// Exhaustion quarantine takes priority: reported only when quarantine is absent, so the
|
||||||
// message never claims "cooling off" for a profile that is really backend-exhausted.
|
// message never claims "cooling off" for a profile that is really backend-exhausted.
|
||||||
boolean dCoolingOff = !dQuarantined && ctx.coolingOff().contains(d);
|
boolean dCoolingOff = !dQuarantined && ctx.coolingOff().contains(d);
|
||||||
|
boolean dUnreachable = ctx.unreachable().contains(d);
|
||||||
boolean dWeightExcluded = weightExcluded(ctx, d);
|
boolean dWeightExcluded = weightExcluded(ctx, d);
|
||||||
if (dQuarantined || dCoolingOff || dWeightExcluded) {
|
if (dQuarantined || dCoolingOff || dUnreachable || dWeightExcluded) {
|
||||||
List<String> reasons = new ArrayList<>();
|
List<String> reasons = new ArrayList<>();
|
||||||
if (dQuarantined) {
|
if (dQuarantined) {
|
||||||
reasons.add("is quarantined (backend exhausted)");
|
reasons.add("is quarantined (backend exhausted)");
|
||||||
@@ -53,6 +62,9 @@ final class FixedPlacementPolicy implements PlacementPolicy {
|
|||||||
if (dCoolingOff) {
|
if (dCoolingOff) {
|
||||||
reasons.add("is cooling off after repeated backend errors");
|
reasons.add("is cooling off after repeated backend errors");
|
||||||
}
|
}
|
||||||
|
if (dUnreachable) {
|
||||||
|
reasons.add("is unreachable");
|
||||||
|
}
|
||||||
if (dWeightExcluded) {
|
if (dWeightExcluded) {
|
||||||
reasons.add("has weight 0 (excluded from automatic selection)");
|
reasons.add("has weight 0 (excluded from automatic selection)");
|
||||||
}
|
}
|
||||||
@@ -62,7 +74,7 @@ final class FixedPlacementPolicy implements PlacementPolicy {
|
|||||||
}
|
}
|
||||||
if (!ctx.candidates().isEmpty()) {
|
if (!ctx.candidates().isEmpty()) {
|
||||||
throw new PlacementException("all worker profiles are excluded from automatic "
|
throw new PlacementException("all worker profiles are excluded from automatic "
|
||||||
+ "selection (quarantined, cooling off, or weight-0)");
|
+ "selection (quarantined, cooling off, unreachable, or weight-0)");
|
||||||
}
|
}
|
||||||
throw new PlacementException("no worker profiles configured");
|
throw new PlacementException("no worker profiles configured");
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -615,6 +615,67 @@ class CompositePeerLauncherTest {
|
|||||||
assertEquals(1, adapter.spawnCount("b"));
|
assertEquals(1, adapter.spawnCount("b"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* fleetd #315: {@code CompositePeerLauncher.spawn} rebuilds the {@link PlacementContext} after
|
||||||
|
* every failed attempt "so the policy excludes this profile" (see the comment at the retry call
|
||||||
|
* site) — but {@code FixedPlacementPolicy} never read {@code ctx.unreachable()}, so under the
|
||||||
|
* default {@code fixed} placement every retry re-picked the same dead default and a second,
|
||||||
|
* healthy, configured profile was never tried. This is the same scenario as
|
||||||
|
* {@link #failoverRetriesNextCandidateWhenProfileIsUnreachable}, but pinned to {@code fixed()}
|
||||||
|
* instead of {@code weighted()} — the three existing failover tests all use {@code weighted()},
|
||||||
|
* which is exactly why nobody caught this: the retry loop's contract has no coverage under its
|
||||||
|
* own default policy.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void failoverRetriesNextCandidateUnderFixedPlacementWhenProfileIsUnreachable() {
|
||||||
|
FakeHerdr herdr = new FakeHerdr();
|
||||||
|
Map<String, FleetConfig.Profile> profiles = ordered(
|
||||||
|
"a", stubWorker("a"),
|
||||||
|
"b", stubWorker("b"));
|
||||||
|
StubLauncher adapter = new StubLauncher("claude", herdr, profiles, "a", Set.of("a"));
|
||||||
|
CompositePeerLauncher composite = new CompositePeerLauncher(
|
||||||
|
List.of(adapter), "a", profiles, PlacementPolicies.fixed(), _ -> 0);
|
||||||
|
|
||||||
|
PeerHandle h = composite.spawn(new SpawnRequest(null, null, null));
|
||||||
|
assertEquals("b", h.profile(),
|
||||||
|
"fixed placement must fail over from the unreachable default a to the healthy b");
|
||||||
|
assertEquals(1, adapter.spawnCount("a"), "a was tried once and failed");
|
||||||
|
assertEquals(1, adapter.spawnCount("b"), "b was tried once and succeeded");
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* fleetd #315: the same fix — {@code FixedPlacementPolicy} consulting {@code ctx.unreachable()}
|
||||||
|
* — also covers the wiring-bug branch in {@code CompositePeerLauncher.spawn}: a profile that
|
||||||
|
* placement is allowed to choose (it is in the configured candidate list) but that no delegate
|
||||||
|
* declares ({@code byProfile.get(chosen.profile()) == null}). That branch adds the profile to
|
||||||
|
* {@code unreachable} and {@code continue}s without ever calling a launcher, so before this fix
|
||||||
|
* {@code fixed} handed back the same adapterless profile on every remaining attempt too.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void failoverSkipsAConfiguredProfileNoAdapterDeclaresUnderFixedPlacement() {
|
||||||
|
FakeHerdr herdr = new FakeHerdr();
|
||||||
|
// Placement's candidate list has three profiles, in this order (LinkedHashMap preserves it,
|
||||||
|
// and the fixed default resolves to the first — see the `ordered` helper's own javadoc).
|
||||||
|
Map<String, FleetConfig.Profile> profiles = new LinkedHashMap<>();
|
||||||
|
profiles.put("c", stubWorker("c"));
|
||||||
|
profiles.put("a", stubWorker("a"));
|
||||||
|
profiles.put("b", stubWorker("b"));
|
||||||
|
// The adapter only declares a and b — c is a configured profile with no owning adapter,
|
||||||
|
// the "wiring bug" the comment in CompositePeerLauncher.spawn calls out.
|
||||||
|
Map<String, FleetConfig.Profile> adapterProfiles = new LinkedHashMap<>();
|
||||||
|
adapterProfiles.put("a", stubWorker("a"));
|
||||||
|
adapterProfiles.put("b", stubWorker("b"));
|
||||||
|
StubLauncher adapter = new StubLauncher("claude", herdr, adapterProfiles, "a", Set.of());
|
||||||
|
CompositePeerLauncher composite = new CompositePeerLauncher(
|
||||||
|
List.of(adapter), "a", profiles, PlacementPolicies.fixed(), _ -> 0);
|
||||||
|
|
||||||
|
PeerHandle h = composite.spawn(new SpawnRequest(null, null, null));
|
||||||
|
assertEquals("a", h.profile(),
|
||||||
|
"c has no adapter, so fixed placement must skip it and land on the next candidate, a");
|
||||||
|
assertEquals(0, adapter.spawnCount("c"), "c is never spawned — no adapter owns it");
|
||||||
|
assertEquals(1, adapter.spawnCount("a"));
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void explicitSpawnAtMaxLoadThrowsPlacementExceptionNamingProfileLiveAndCap() {
|
void explicitSpawnAtMaxLoadThrowsPlacementExceptionNamingProfileLiveAndCap() {
|
||||||
FakeHerdr herdr = new FakeHerdr();
|
FakeHerdr herdr = new FakeHerdr();
|
||||||
|
|||||||
Reference in New Issue
Block a user