From 46702d1af311a05c7a37ff90f6fee888c94b3e84 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 13:46:14 +0200 Subject: [PATCH] Keep the builder off a node whose capacity setting is 0, and start the setting at shanks=3,ace=2,g14=0,novox=0 (issue 1661) --- .../mesh-delivery-builder/settings_test.go | 7 ++- .../mesh-delivery-builder/files/settings.json | 2 +- .../internal/build/choose.go | 10 ++-- .../internal/build/places.go | 11 +++-- .../internal/build/places_test.go | 48 ++++++++++++++++++- .../internal/build/run.go | 2 +- .../mesh-delivery-builder/manifest_test.go | 8 ++-- modules/mesh-delivery-builder/module.json | 6 +-- 8 files changed, 72 insertions(+), 22 deletions(-) diff --git a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/settings_test.go b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/settings_test.go index ffa96d4..8b52444 100644 --- a/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/settings_test.go +++ b/modules/mesh-delivery-builder/cmd/mesh-delivery-builder/settings_test.go @@ -45,8 +45,11 @@ func mustCapacity(t *testing.T, path string) map[string]int { } func TestACapacitySettingThatIsNotOneIsRefusedNamingIt(t *testing.T) { - if _, err := capacityFrom(settingsFile(t, `{"capacity": "shanks=0"}`), ""); err == nil { - t.Error("shanks=0 accepted") + if _, err := capacityFrom(settingsFile(t, `{"capacity": "shanks=-1"}`), ""); err == nil { + t.Error("shanks=-1 accepted") + } + if c, err := capacityFrom(settingsFile(t, `{"capacity": "shanks=3,ace=2,g14=0,novox=0"}`), ""); err != nil || c["g14"] != 0 || len(c) != 4 { + t.Errorf("the starting value: %v, %v", c, err) } if _, err := capacityFrom(settingsFile(t, `not json`), ""); err == nil { t.Error("a file that is not JSON accepted") diff --git a/modules/mesh-delivery-builder/files/settings.json b/modules/mesh-delivery-builder/files/settings.json index 1354ea4..3175fe3 100644 --- a/modules/mesh-delivery-builder/files/settings.json +++ b/modules/mesh-delivery-builder/files/settings.json @@ -1,3 +1,3 @@ { - "capacity": "" + "capacity": "shanks=3,ace=2,g14=0,novox=0" } diff --git a/modules/mesh-delivery-builder/internal/build/choose.go b/modules/mesh-delivery-builder/internal/build/choose.go index a71c61c..7f87b88 100644 --- a/modules/mesh-delivery-builder/internal/build/choose.go +++ b/modules/mesh-delivery-builder/internal/build/choose.go @@ -34,6 +34,9 @@ func ChooseAnyNodes(language string, entries []delivery.NodeToolchain, places *P } func choose(language string, entries []delivery.NodeToolchain, places *Places, qualifies func(node string) bool, want int) ([]string, error) { + if places == nil { + places = &Places{} + } nodes := make([]string, 0, len(entries)) var reporting []string seen := map[string]bool{} @@ -43,16 +46,13 @@ func choose(language string, entries []delivery.NodeToolchain, places *Places, q } seen[e.Node] = true reporting = append(reporting, e.Node) - if qualifies == nil || qualifies(e.Node) { + if places.CapacityOf(e.Node) > 0 && (qualifies == nil || qualifies(e.Node)) { // a capacity of 0 keeps the builder off a node nodes = append(nodes, e.Node) } } if len(nodes) == 0 { sort.Strings(reporting) - return nil, fmt.Errorf("no %s build node qualifies: %s report the toolchain, and none is online with a healthy build module", language, strings.Join(reporting, ", ")) - } - if places == nil { - places = &Places{} + return nil, fmt.Errorf("no %s build node qualifies: %s report the toolchain, and none is online with a healthy build module and a build capacity", language, strings.Join(reporting, ", ")) } return places.choose(nodes, want), nil } diff --git a/modules/mesh-delivery-builder/internal/build/places.go b/modules/mesh-delivery-builder/internal/build/places.go index 18e2c30..65d41fb 100644 --- a/modules/mesh-delivery-builder/internal/build/places.go +++ b/modules/mesh-delivery-builder/internal/build/places.go @@ -14,7 +14,7 @@ const DefaultCapacity = 1 // ParseCapacity reads the builder's capacity setting: `node=steps` pairs separated by commas, such as // `shanks=3,ace=2`. A node without a pair has DefaultCapacity. A pair with no node, a count that is not a whole number of -// at least 1, and a node named twice are refused. +// at least 0, and a node named twice are refused. A count of 0 keeps the builder off the node: it is never chosen. func ParseCapacity(s string) (map[string]int, error) { out := map[string]int{} for _, pair := range strings.Split(s, ",") { @@ -25,8 +25,8 @@ func ParseCapacity(s string) (map[string]int, error) { node, count, ok := strings.Cut(pair, "=") node, count = strings.TrimSpace(node), strings.TrimSpace(count) n, err := strconv.Atoi(count) - if !ok || node == "" || err != nil || n < 1 { - return nil, fmt.Errorf("the capacity %q is not =", pair) + if !ok || node == "" || err != nil || n < 0 { + return nil, fmt.Errorf("the capacity %q is not =", pair) } if _, dup := out[node]; dup { return nil, fmt.Errorf("the capacity names %s twice", node) @@ -52,9 +52,10 @@ type Places struct { changed chan struct{} // closed, and replaced, each time a lane ends } -// CapacityOf is how many steps the builder runs on the node at once. +// CapacityOf is how many steps the builder runs on the node at once: DefaultCapacity for a node the setting does not +// name, and 0 for one it names so, where the builder never builds. func (p *Places) CapacityOf(node string) int { - if n, ok := p.Capacity[node]; ok && n >= 1 { + if n, ok := p.Capacity[node]; ok && n >= 0 { return n } return DefaultCapacity diff --git a/modules/mesh-delivery-builder/internal/build/places_test.go b/modules/mesh-delivery-builder/internal/build/places_test.go index 5cb3d24..27c7d8d 100644 --- a/modules/mesh-delivery-builder/internal/build/places_test.go +++ b/modules/mesh-delivery-builder/internal/build/places_test.go @@ -19,7 +19,7 @@ func TestParseCapacityReadsNodeEqualsCountPairs(t *testing.T) { if c, err := ParseCapacity(""); err != nil || len(c) != 0 { t.Errorf("empty: %v, %v", c, err) } - for _, bad := range []string{"shanks", "shanks=0", "shanks=x", "=2", "a=1,a=2", "shanks=-1"} { + for _, bad := range []string{"shanks", "shanks=x", "=2", "a=1,a=2", "shanks=-1"} { if _, err := ParseCapacity(bad); err == nil { t.Errorf("%q accepted", bad) } @@ -138,3 +138,49 @@ func TestTwoNodeAgreementStillTakesTwoDifferentNodesWhenCapacitiesAreSet(t *test t.Errorf("build = %+v, want done on ace and novox", v) } } + +// The operator's decision of 2026-10-21 13:40: the builder builds on shanks and ace only; a capacity of 0 keeps it off a node. +func TestACapacityOfZeroKeepsTheBuilderOffTheNodeAndANodeNotNamedKeepsOne(t *testing.T) { // issue 1661 + c, err := ParseCapacity("shanks=3,ace=2,g14=0,novox=0") + if err != nil || c["g14"] != 0 || c["novox"] != 0 || c["shanks"] != 3 || len(c) != 4 { + t.Fatalf("capacity = %v, %v", c, err) + } + p := &Places{Capacity: c} + if p.CapacityOf("g14") != 0 || p.CapacityOf("novox") != 0 || p.CapacityOf("ace") != 2 || p.CapacityOf("other") != DefaultCapacity { + t.Errorf("capacities = %d %d %d %d, want 0 0 2 and the default for a node not named", p.CapacityOf("g14"), p.CapacityOf("novox"), p.CapacityOf("ace"), p.CapacityOf("other")) + } + if _, err := ParseCapacity("shanks=-1"); err == nil { + t.Error("a negative capacity accepted") + } + entries := tcs("go", "go1.27.1", "ace", "g14", "novox", "shanks", "other") + for i := 0; i < 6; i++ { + nodes, _, err := ChooseNodes("go", entries, p, nil, 2) + if err != nil { + t.Fatal(err) + } + for _, n := range nodes { + if n == "g14" || n == "novox" { + t.Fatalf("choice %d: %v includes a node of capacity 0", i, nodes) + } + } + } + // Only nodes of capacity 0 report: nothing qualifies, and the build says why. + _, _, err = ChooseNodes("go", tcs("go", "go1.27.1", "g14", "novox"), p, nil, 2) + if err == nil || !strings.Contains(err.Error(), "capacity") { + t.Errorf("only capacity-0 nodes: error = %v, want it to name the capacity", err) + } +} + +func TestABuildNeverRunsOnANodeOfCapacityZero(t *testing.T) { // issue 1661 + r := newRig(t, catalogue.Artifact{Name: "tool", Kind: "bundle", Language: "go"}) // ace and novox report + r.b.Places = &Places{Capacity: map[string]int{"novox": 0}} + v := r.consider(t, "modules/m") + if v.State != "done" || !strings.Contains(v.Step, "on one node, ace") { + t.Fatalf("build = %+v, want done on ace alone", v) + } + for _, c := range r.bm.calls { + if c.node == "novox" { + t.Errorf("a step was called on novox, whose capacity is 0: %+v", c) + } + } +} diff --git a/modules/mesh-delivery-builder/internal/build/run.go b/modules/mesh-delivery-builder/internal/build/run.go index 2968cbc..53de028 100644 --- a/modules/mesh-delivery-builder/internal/build/run.go +++ b/modules/mesh-delivery-builder/internal/build/run.go @@ -455,7 +455,7 @@ func (b *Builder) online(node string) bool { return b.Presence == nil || b.Prese // qualifies is whether the node can build with the build module now: it is online and the build module on it is healthy. func (b *Builder) qualifies(node, bm string) bool { - return b.online(node) && (b.Health == nil || b.Health.Healthy(node, bm)) + return b.Places.CapacityOf(node) > 0 && b.online(node) && (b.Health == nil || b.Health.Healthy(node, bm)) } // moreNodes is whether any of these build modules has two nodes that qualify now. diff --git a/modules/mesh-delivery-builder/manifest_test.go b/modules/mesh-delivery-builder/manifest_test.go index 59bdf03..66ac3cf 100644 --- a/modules/mesh-delivery-builder/manifest_test.go +++ b/modules/mesh-delivery-builder/manifest_test.go @@ -138,8 +138,8 @@ func TestManifestProcess(t *testing.T) { // 1607/AC1 t.Errorf("restart-on = %v, want the settings file, so a changed capacity reaches the running program", p["restart-on"]) } c, ok := m.Config["capacity"] - if !ok || len(m.Config) != 1 || c.Type != "string" || c.Default != "" || c.Description == "" { - t.Errorf("config = %+v, want one string setting, capacity, empty by default", m.Config) + if !ok || len(m.Config) != 1 || c.Type != "string" || c.Default != "shanks=3,ace=2,g14=0,novox=0" || c.Description == "" { + t.Errorf("config = %+v, want one string setting, capacity, shanks=3,ace=2,g14=0,novox=0 to start", m.Config) } var settings map[string]any for _, r := range m.Resources { @@ -150,8 +150,8 @@ func TestManifestProcess(t *testing.T) { // 1607/AC1 if settings == nil || settings["type"] != "file" || settings["merge"] != "json" || !reflect.DeepEqual(settings["takes-config"], []any{"capacity"}) || settings["path"] != "${dir:state}/settings.json" { t.Errorf("settings resource = %v", settings) } - if raw, err := os.ReadFile("files/settings.json"); err != nil || !strings.Contains(string(raw), `"capacity": ""`) { - t.Errorf("files/settings.json = %q, %v, want the setting's empty default", raw, err) + if raw, err := os.ReadFile("files/settings.json"); err != nil || !strings.Contains(string(raw), `"capacity": "shanks=3,ace=2,g14=0,novox=0"`) { + t.Errorf("files/settings.json = %q, %v, want the setting's starting value", raw, err) } if _, bus := p["bus"]; bus { t.Errorf("the process declares a bus: it runs on its own broker credential") diff --git a/modules/mesh-delivery-builder/module.json b/modules/mesh-delivery-builder/module.json index e312535..292c9ae 100644 --- a/modules/mesh-delivery-builder/module.json +++ b/modules/mesh-delivery-builder/module.json @@ -28,9 +28,9 @@ "config": { "capacity": { "type": "string", - "description": "How many build steps the builder runs at once on each node, as node=steps pairs separated by commas, such as shanks=3,ace=2 (ADR 0478 clause 3); a node not named runs one at a time, and empty is one everywhere.", - "pattern": "|[a-z0-9_-]+=[1-9][0-9]*(,[a-z0-9_-]+=[1-9][0-9]*)*", - "default": "" + "description": "How many build steps the builder runs at once on each node, as node=steps pairs separated by commas, such as shanks=3,ace=2 (ADR 0478 clause 3); a node set to 0 never builds, a node not named runs one at a time, and empty is one everywhere. The builder builds on shanks and ace only (the operator, 2026-10-21).", + "pattern": "|[a-z0-9_-]+=[0-9]+(,[a-z0-9_-]+=[0-9]+)*", + "default": "shanks=3,ace=2,g14=0,novox=0" } }, "own-secrets": {