From 4657931657ba99f85823964e8216d7a5432a516f Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 7 Sep 2026 22:51:46 -0400 Subject: [PATCH] feat(labelfilter): synthesize the pool's platform label so platform-keyed tasks scale the pool (RIG-1471) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NewPoolFilter` models which pending tasks the elastic pool could run. It synthesized `repo="*"` and `org-id="*"` but never `platform` — a label every real agent self-reports at registration (`cmd/agent/core/agent.go`) and the model has no agent to ask. Once the runner-label taxonomy keys pool selectors on `platform` (e.g. `linux/arm64`), a pending task carrying `platform` is unsatisfiable in the model, so `calcAgents` skips it when counting eligible pending work. With `MIN_AGENTS=0` a cold pool would then **never** scale up for those tasks: they strand forever, with no error. Adds `config.PoolPlatform`, sourced from `WOODPECKER_POOL_PLATFORM` (`--pool-platform`), and synthesizes `platform=` **before** `maps.Copy(labels, extra)` so an explicit `ExtraAgentLabels` entry still wins. `org-id` stays last and non-overridable. An empty value synthesizes no `platform` key at all, so an unconfigured deployment keeps today's behaviour instead of asserting a platform it cannot know. Co-authored-by: Matt Wilkinson --- cmd/woodpecker-autoscaler/flags.go | 5 + cmd/woodpecker-autoscaler/main.go | 11 ++ config/config.go | 6 ++ engine/autoscaler.go | 2 +- engine/autoscaler_test.go | 85 ++++++++++++++++ engine/labelfilter/labelfilter.go | 26 ++++- engine/labelfilter/labelfilter_test.go | 134 ++++++++++++++++++++++++- 7 files changed, 259 insertions(+), 10 deletions(-) diff --git a/cmd/woodpecker-autoscaler/flags.go b/cmd/woodpecker-autoscaler/flags.go index 5ba05321..815001d5 100644 --- a/cmd/woodpecker-autoscaler/flags.go +++ b/cmd/woodpecker-autoscaler/flags.go @@ -118,4 +118,9 @@ var flags = []cli.Flag{ Usage: "add additional labels the agent will report to the server. list with key=value pairs", Sources: cli.EnvVars("WOODPECKER_AGENT_LABELS"), }, + &cli.StringFlag{ + Name: "pool-platform", + Usage: "platform the pool's agents self-report, as os/arch (e.g. linux/arm64). must match what the agents will report; leave empty to disable platform synthesis in the pool filter", + Sources: cli.EnvVars("WOODPECKER_POOL_PLATFORM"), + }, } diff --git a/cmd/woodpecker-autoscaler/main.go b/cmd/woodpecker-autoscaler/main.go index d156a36d..4af06e60 100644 --- a/cmd/woodpecker-autoscaler/main.go +++ b/cmd/woodpecker-autoscaler/main.go @@ -88,6 +88,17 @@ func run(ctx context.Context, cmd *cli.Command) error { UserData: cmd.String("cloudinit-template"), ExtraAgentLabels: agentLabels, Environment: agentEnvironment, + PoolPlatform: cmd.String("pool-platform"), + } + + // A platform-keyed task is unmatchable against a pool filter carrying no + // platform key, and calcAgents skips unsatisfiable tasks silently — so an + // unset value strands that work forever with MIN_AGENTS=0 and no error + // anywhere. The opt-out is deliberate (it preserves pre-platform + // behavior), so warn rather than fail: a deployment that never uses + // platform-keyed selectors is still valid. + if config.PoolPlatform == "" { + log.Warn().Msg("WOODPECKER_POOL_PLATFORM is unset: no platform label is synthesized for the pool, so platform-keyed tasks will never scale it up") } provider, err := setupProvider(ctx, cmd, config) diff --git a/config/config.go b/config/config.go index 1d7eb4b2..fcb543d2 100644 --- a/config/config.go +++ b/config/config.go @@ -19,6 +19,12 @@ type Config struct { AgentIdleTimeout time.Duration UserData string // cloudinit template ExtraAgentLabels map[string]string + // PoolPlatform is the Woodpecker `platform` label the pool's agents will + // self-report — the fused os/arch string (e.g. "linux/arm64"). A real + // agent stamps this itself at registration; the modeled pool filter has no + // agent to ask, so it must be configured here or a platform-keyed task + // looks unrunnable to the scaler. Empty disables the synthesis. + PoolPlatform string // BillingModel is taken from the selected provider and selects the teardown // policy the engine applies to idle agents. diff --git a/engine/autoscaler.go b/engine/autoscaler.go index b731eb6c..b6e61d0d 100644 --- a/engine/autoscaler.go +++ b/engine/autoscaler.go @@ -521,7 +521,7 @@ func (a *Autoscaler) calcAgents(ctx context.Context) (float64, error) { return 0, err } - poolFilter := labelfilter.NewPoolFilter(a.config.ExtraAgentLabels) + poolFilter := labelfilter.NewPoolFilter(a.config.ExtraAgentLabels, a.config.PoolPlatform) staticSlots := a.nonPoolFreeSlots(queueInfo.Running) eligiblePending := 0 diff --git a/engine/autoscaler_test.go b/engine/autoscaler_test.go index 95bbda1f..84f7a0cc 100644 --- a/engine/autoscaler_test.go +++ b/engine/autoscaler_test.go @@ -46,6 +46,12 @@ func macTask() woodpecker.Task { return woodpecker.Task{Labels: map[string]string{"type": "macos", "repo": "rigel/x", "org-id": "1"}} } +// platformTask is a pending task keyed on the agent-self-reported platform +// label, as the taxonomy's pool selectors emit. +func platformTask() woodpecker.Task { + return woodpecker.Task{Labels: map[string]string{"type": "linux", "platform": "linux/arm64", "repo": "rigel/x", "org-id": "1"}} +} + // elasticLabels is the pool's WOODPECKER_AGENT_LABELS in the parent's model. func elasticLabels() map[string]string { return map[string]string{"type": "linux", "pool": "elastic"} @@ -108,6 +114,85 @@ func Test_calcAgents(t *testing.T) { assert.Equal(t, float64(2), value, "only the two bare-linux tasks are eligible") }) + // Regression guard for the strand-forever case: with MIN_AGENTS=0 a cold + // pool only ever scales from eligible pending work, so a platform-keyed + // task the model cannot satisfy strands with no error and no scale-up. + t.Run("platform-keyed pending ⇒ scales when PoolPlatform matches", func(t *testing.T) { + autoscaler := Autoscaler{client: &MockClient{ + pending: []woodpecker.Task{platformTask(), platformTask()}, + }, config: &config.Config{ + WorkflowsPerAgent: 1, + MaxAgents: 8, + MinAgents: 0, + ExtraAgentLabels: elasticLabels(), + PoolPlatform: "linux/arm64", + }} + + value, err := autoscaler.calcAgents(t.Context()) + assert.NoError(t, err) + assert.Equal(t, float64(2), value, "a platform the pool's agents will report must count as eligible demand") + }) + + t.Run("platform-keyed pending ⇒ no scale-up when PoolPlatform is unset", func(t *testing.T) { + autoscaler := Autoscaler{client: &MockClient{ + pending: []woodpecker.Task{platformTask(), platformTask()}, + }, config: &config.Config{ + WorkflowsPerAgent: 1, + MaxAgents: 8, + MinAgents: 0, + ExtraAgentLabels: elasticLabels(), + }} + + value, _ := autoscaler.calcAgents(t.Context()) + assert.Equal(t, float64(0), value, "with no configured platform the model cannot claim the task") + }) + + t.Run("platform-keyed pending ⇒ no scale-up when PoolPlatform mismatches", func(t *testing.T) { + autoscaler := Autoscaler{client: &MockClient{ + pending: []woodpecker.Task{platformTask(), platformTask()}, + }, config: &config.Config{ + WorkflowsPerAgent: 1, + MaxAgents: 8, + MinAgents: 0, + ExtraAgentLabels: elasticLabels(), + PoolPlatform: "linux/amd64", + }} + + value, _ := autoscaler.calcAgents(t.Context()) + assert.Equal(t, float64(0), value, "an amd64 pool must not scale for arm64 work") + }) + + // The money case for modeling the agent's self-reported platform: an idle + // arm64 static builder with free slots must NET OUT platform-keyed work + // rather than have the pool boot Spot agents beside it. Before AgentFilter + // read Agent.Platform this returned 2 — two paid boots next to an idle + // machine that could run both tasks. + t.Run("an idle static nets out platform-keyed work via its self-reported platform", func(t *testing.T) { + builder := &woodpecker.Agent{ + ID: 7, Name: "mattmini", OrgID: -1, Capacity: 2, + Platform: "linux/arm64", + CustomLabels: map[string]string{"builder": "image"}, + LastContact: time.Now().Unix(), + } + buildTask := woodpecker.Task{Labels: map[string]string{"builder": "image", "platform": "linux/arm64", "repo": "rigel/x", "org-id": "1"}} + autoscaler := Autoscaler{ + client: &MockClient{pending: []woodpecker.Task{buildTask, buildTask}}, + allAgents: []*woodpecker.Agent{builder}, + config: &config.Config{ + WorkflowsPerAgent: 1, + MaxAgents: 8, + PoolID: "1", + ExtraAgentLabels: map[string]string{"builder": "image"}, + PoolPlatform: "linux/arm64", + AgentInactivityTimeout: 10 * time.Minute, + }, + } + + value, err := autoscaler.calcAgents(t.Context()) + assert.NoError(t, err) + assert.Equal(t, float64(0), value, "two free slots on the idle arm64 builder absorb both arm64 builds") + }) + t.Run("WorkflowsPerAgent packs multiple eligible tasks per agent", func(t *testing.T) { autoscaler := Autoscaler{client: &MockClient{ pending: []woodpecker.Task{linuxTask(), linuxTask(), linuxTask(), linuxTask(), linuxTask(), linuxTask()}, diff --git a/engine/labelfilter/labelfilter.go b/engine/labelfilter/labelfilter.go index 73565526..d00386d7 100644 --- a/engine/labelfilter/labelfilter.go +++ b/engine/labelfilter/labelfilter.go @@ -59,13 +59,20 @@ type Filter struct { } // NewPoolFilter builds the modeled filter for the elastic pool this autoscaler -// manages, from the pool's WOODPECKER_AGENT_LABELS (config.ExtraAgentLabels). +// manages, from the pool's WOODPECKER_AGENT_LABELS (config.ExtraAgentLabels) +// and the platform its agents will self-report (config.PoolPlatform). // // It synthesizes the same defaults the real agents get so server-stamped task // labels don't make every task unmatchable: // - repo="*" — the agent default (cmd/agent/core/agent.go: LabelFilterRepo // = "*" "allow all repos by default"), overridable by an explicit custom // label; +// - platform= — a real agent reports its own fused os/arch +// (cmd/agent/core/agent.go stamps LabelFilterPlatform before the custom +// labels), but the model has no agent to ask, so the value must be +// configured. Overridable by an explicit custom label. When platform is +// "" no platform key is synthesized at all: an unconfigured deployment +// keeps today's behavior rather than asserting a platform it cannot know; // - org-id="*" — autoscaler-created agents are system agents (OrgID unset), // and the server enforces org-id="*" for them // (server/model/agent.go GetServerLabels). @@ -73,9 +80,12 @@ type Filter struct { // A custom label of the same key overrides the default (agent.go applies // customLabels last via maps.Copy); org-id is server-enforced, so it is applied // after the customs and always wins for the pool's system agents. -func NewPoolFilter(extra map[string]string) Filter { - labels := make(map[string]string, len(extra)+2) +func NewPoolFilter(extra map[string]string, platform string) Filter { + labels := make(map[string]string, len(extra)+3) labels[pipeline.LabelFilterRepo] = "*" + if platform != "" { + labels[pipeline.LabelFilterPlatform] = platform + } maps.Copy(labels, extra) // org-id is enforced by the server for system (autoscaler-created) agents, // so it is applied last and is not overridable by ExtraAgentLabels. @@ -87,11 +97,19 @@ func NewPoolFilter(extra map[string]string) Filter { // shared-demand netting step to test whether a non-pool/static agent can // absorb a task). It mirrors the agent + server label synthesis: // - repo="*" default under the agent's CustomLabels; +// - platform from the agent's own self-report. Unlike the pool's value this +// needs no config: the agent already told the server its fused os/arch and +// the API carries it as a first-class field, so it is truthful by +// construction. It sits under CustomLabels for the same reason the real +// agent's does (agent.go stamps it, then copies customs over the top); // - org-id from the server ownership rule (server/model/agent.go // GetServerLabels): OrgID unset (== idNotSet, -1) ⇒ "*", else the id. func AgentFilter(a *woodpecker.Agent) Filter { - labels := make(map[string]string, len(a.CustomLabels)+2) + labels := make(map[string]string, len(a.CustomLabels)+3) labels[pipeline.LabelFilterRepo] = "*" + if a.Platform != "" { + labels[pipeline.LabelFilterPlatform] = a.Platform + } maps.Copy(labels, a.CustomLabels) if a.OrgID != idNotSet { labels[pipeline.LabelFilterOrg] = strconv.FormatInt(a.OrgID, 10) diff --git a/engine/labelfilter/labelfilter_test.go b/engine/labelfilter/labelfilter_test.go index 04332aaf..5809f9ca 100644 --- a/engine/labelfilter/labelfilter_test.go +++ b/engine/labelfilter/labelfilter_test.go @@ -234,24 +234,109 @@ func TestRequiredLabelsMissing(t *testing.T) { // configured WOODPECKER_AGENT_LABELS, so server-stamped task labels do not make // every task unmatchable. func TestNewPoolFilter(t *testing.T) { - f := NewPoolFilter(map[string]string{"type": "linux", "pool": "elastic"}) + f := NewPoolFilter(map[string]string{"type": "linux", "pool": "elastic"}, "") assert.Equal(t, "*", f.labels[pipeline.LabelFilterRepo], "repo default synthesized") assert.Equal(t, "*", f.labels[pipeline.LabelFilterOrg], "org-id default synthesized") assert.Equal(t, "linux", f.labels["type"]) assert.Equal(t, "elastic", f.labels["pool"]) t.Run("nil extra still synthesizes defaults", func(t *testing.T) { - f := NewPoolFilter(nil) + f := NewPoolFilter(nil, "") assert.Equal(t, "*", f.labels[pipeline.LabelFilterRepo]) assert.Equal(t, "*", f.labels[pipeline.LabelFilterOrg]) }) t.Run("custom label overrides repo default", func(t *testing.T) { - f := NewPoolFilter(map[string]string{"repo": "sealed/only"}) + f := NewPoolFilter(map[string]string{"repo": "sealed/only"}, "") assert.Equal(t, "sealed/only", f.labels[pipeline.LabelFilterRepo]) // org-id stays server-enforced "*" for system agents assert.Equal(t, "*", f.labels[pipeline.LabelFilterOrg]) }) + + t.Run("empty platform synthesizes no platform key", func(t *testing.T) { + f := NewPoolFilter(map[string]string{"type": "linux"}, "") + _, ok := f.labels[pipeline.LabelFilterPlatform] + assert.False(t, ok, "an unconfigured pool must not assert a platform it cannot know") + }) +} + +// TestNewPoolFilterPlatform is the platform-synthesis table. It is deliberately +// separate from TestMatchFilter, which is a verbatim transliteration of the +// upstream scheduler's own table and must not gain local cases — this one +// exercises our synthesis through the same (agent labels ⇒ verdict) shape, via +// the public Satisfiable path. +// +// A real agent self-reports platform (cmd/agent/core/agent.go) before its +// custom labels; the model has no agent, so NewPoolFilter must synthesize it +// from config.PoolPlatform or every platform-keyed task looks unrunnable. +func TestNewPoolFilterPlatform(t *testing.T) { + tests := []struct { + name string + extra map[string]string + poolPlatform string + taskLabels map[string]string + wantMatched bool + }{ + { + name: "matching platform request matches when PoolPlatform is set", + extra: map[string]string{"builder": "image"}, + poolPlatform: "linux/arm64", + taskLabels: map[string]string{"builder": "image", "platform": "linux/arm64", "repo": "rigel/x", "org-id": "1"}, + wantMatched: true, + }, + { + name: "non-matching platform request does not match", + extra: map[string]string{"builder": "image"}, + poolPlatform: "linux/arm64", + taskLabels: map[string]string{"builder": "image", "platform": "linux/amd64", "repo": "rigel/x", "org-id": "1"}, + wantMatched: false, + }, + { + // Today's behavior, preserved exactly: with no platform key on the + // modeled agent, matchFilter hard-rejects any task label the agent + // does not carry, so a platform-requesting task is unmatchable. + name: "empty PoolPlatform: platform-requesting task is unmatchable", + extra: map[string]string{"builder": "image"}, + poolPlatform: "", + taskLabels: map[string]string{"builder": "image", "platform": "linux/arm64", "repo": "rigel/x", "org-id": "1"}, + wantMatched: false, + }, + { + name: "empty PoolPlatform: a task not requesting platform is unaffected", + extra: map[string]string{"builder": "image"}, + poolPlatform: "", + taskLabels: map[string]string{"builder": "image", "repo": "rigel/x", "org-id": "1"}, + wantMatched: true, + }, + { + name: "explicit ExtraAgentLabels platform overrides the synthesized default", + extra: map[string]string{"builder": "image", "platform": "linux/amd64"}, + poolPlatform: "linux/arm64", + taskLabels: map[string]string{"builder": "image", "platform": "linux/amd64", "repo": "rigel/x", "org-id": "1"}, + wantMatched: true, + }, + { + name: "explicit ExtraAgentLabels platform wins, so the synthesized value no longer matches", + extra: map[string]string{"builder": "image", "platform": "linux/amd64"}, + poolPlatform: "linux/arm64", + taskLabels: map[string]string{"builder": "image", "platform": "linux/arm64", "repo": "rigel/x", "org-id": "1"}, + wantMatched: false, + }, + { + name: "org-id stays server-enforced under an explicit override attempt", + extra: map[string]string{"builder": "image", "org-id": "7"}, + poolPlatform: "linux/arm64", + taskLabels: map[string]string{"builder": "image", "platform": "linux/arm64", "repo": "rigel/x", "org-id": "1"}, + wantMatched: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := NewPoolFilter(tt.extra, tt.poolPlatform) + assert.Equal(t, tt.wantMatched, f.Satisfiable(woodpecker.Task{Labels: tt.taskLabels}), "Matched result") + }) + } } // TestSatisfiableWorkedExamples is the three worked examples from the design, @@ -259,7 +344,7 @@ func TestNewPoolFilter(t *testing.T) { // on every task (the case the synthesis exists to survive). func TestSatisfiableWorkedExamples(t *testing.T) { // pool advertises type=linux,pool=elastic (its WOODPECKER_AGENT_LABELS) - pool := NewPoolFilter(map[string]string{"type": "linux", "pool": "elastic"}) + pool := NewPoolFilter(map[string]string{"type": "linux", "pool": "elastic"}, "") t.Run("size=large excluded (the waste case)", func(t *testing.T) { task := woodpecker.Task{Labels: map[string]string{ @@ -286,7 +371,7 @@ func TestSatisfiableWorkedExamples(t *testing.T) { // TestSatisfiableClauses covers the individual filter clauses through the // public Satisfiable path (internal-label strip, empty-value skip). func TestSatisfiableClauses(t *testing.T) { - pool := NewPoolFilter(map[string]string{"type": "linux"}) + pool := NewPoolFilter(map[string]string{"type": "linux"}, "") t.Run("internal woodpecker-ci.org labels are stripped before matching", func(t *testing.T) { task := woodpecker.Task{Labels: map[string]string{ @@ -331,6 +416,45 @@ func TestAgentFilter(t *testing.T) { assert.True(t, f.Satisfiable(woodpecker.Task{Labels: map[string]string{"type": "linux", "org-id": "7"}})) assert.False(t, f.Satisfiable(woodpecker.Task{Labels: map[string]string{"type": "linux", "org-id": "8"}})) }) + + // The agent's platform is a first-class API field, NOT a custom label + // (verified against the live fleet: every agent reports a platform and no + // agent carries one in custom_labels). Modeling it from CustomLabels alone + // therefore makes a platform-keyed task look unrunnable on a static that + // can in fact run it — so the netting step stops crediting its idle slots + // and the pool boots paid agents beside an idle machine. + t.Run("self-reported platform is modeled, so a platform-keyed task is absorbable", func(t *testing.T) { + a := &woodpecker.Agent{ + OrgID: -1, + Platform: "linux/arm64", + CustomLabels: map[string]string{"builder": "image"}, + } + f := AgentFilter(a) + assert.Equal(t, "linux/arm64", f.labels[pipeline.LabelFilterPlatform]) + task := woodpecker.Task{Labels: map[string]string{"builder": "image", "platform": "linux/arm64", "repo": "rigel/x", "org-id": "1"}} + assert.True(t, f.Satisfiable(task), "the idle arm64 builder can run its own platform's work") + }) + + t.Run("a mismatched platform request is not absorbable", func(t *testing.T) { + a := &woodpecker.Agent{ + OrgID: -1, + Platform: "linux/arm64", + CustomLabels: map[string]string{"builder": "image"}, + } + f := AgentFilter(a) + task := woodpecker.Task{Labels: map[string]string{"builder": "image", "platform": "linux/amd64", "repo": "rigel/x", "org-id": "1"}} + assert.False(t, f.Satisfiable(task), "an arm64 agent must not absorb amd64 work") + }) + + t.Run("an explicit custom platform label overrides the self-report", func(t *testing.T) { + a := &woodpecker.Agent{ + OrgID: -1, + Platform: "linux/arm64", + CustomLabels: map[string]string{"platform": "linux/amd64"}, + } + f := AgentFilter(a) + assert.Equal(t, "linux/amd64", f.labels[pipeline.LabelFilterPlatform], "customs are copied over the self-reported default") + }) } // TestParityVersionPin is a tripwire on the manual parity invariant. The match