From 1dca98a2283d94e0687758364df5363c55e87450 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Sun, 16 Aug 2026 11:36:16 -0700 Subject: [PATCH] feat(orchestrator): make the build budget a per-queue setting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? How many builds a queue may have occupying CI at once was a constant in the wiring, with a `TODO` beside it saying so: ```go // TODO: move this onto entity.QueueConfig so operators can tune it per queue // without a code change. const defaultBuildBudget = 4 ``` Four is a reasonable default and a poor universal answer. It is the only rationing lever the allocator has, and it decides how much speculation a queue does at all: a queue allowed one build never hedges an outcome, and a queue with a large CI pool behind it has no way to say so. A deployment running a busy trunk queue beside a quiet one has to pick a number that suits neither, and changing it means editing Go and shipping a binary. It is also the setting a reader of the demo asks about first, because it is the one that visibly changes what a run does, and it was the only such knob with no way to set it. ### What? `profiles.yaml` gains a `speculator` block, per queue and in `defaults`, with one field: ```yaml defaults: speculator: {buildBudget: 4} queues: - name: demo-queue speculator: {buildBudget: 12} ``` It inherits and overrides exactly as the other extension blocks do — a queue that says nothing takes the default, and the default itself falls back to 4 when unstated, so every existing configuration and the built-in topology behave as they did. The block has no `type`. There is one speculator, composed from the queue's scorer, and what varies between queues is what it is allowed to spend — but the block is where an allocator choice would go if a second one ever exists, which a bare `buildBudget:` at queue level would not be. The `TODO` proposed `entity.QueueConfig` instead. That is the gateway's record of which queues exist; the budget is speculation policy, which is what profiles already carry per queue, and it is resolved a few lines from the scorer it shares a speculator with. `QueueConfig` is left holding just the queue name. **A negative budget is rejected at startup** rather than clamped. Sticky computes free slots as `budget - funded`, so a negative one yields no free slots ever: the queue would batch and then never build, which reads as a stuck queue rather than a misconfigured one. Absent or `0` takes the default — those are the same value in YAML and cannot be told apart, so the harmless reading wins. The number is logged alongside the other resolved defaults, since a queue building less than expected is otherwise a silent condition. ## Test Plan - ✅ a test that drives a real speculator per queue and counts what it proposes — eight dependency-free speculating batches against budgets of 5, 2 (inherited) and 2 (unlisted queue), asserting the proposals stop at the budget. Parsing a number proves nothing if it never reaches the allocator, so the assertion is on behaviour rather than on the parsed config - ✅ mutation-tested that assertion: reverting `withSpeculator` to the old constant fails all three cases, so it is not passing by construction - ✅ a negative budget fails `loadProfilesConfig`; an unstated one resolves to 4 while a stated one survives normalization - ✅ `make test`, `make lint`, `make gazelle`, `make check-tidy` - ✅ against a live stack, which is what proves the mounted file is read rather than just parsed in a test: `buildBudget: -1` fails the orchestrator at boot with `defaults: build budget -1 is negative`, and `buildBudget: 12` starts, logs `default_build_budget: 12`, and lands a six-change run whose deepest request records `speculating [building ×12, built ×12]` — the raised budget being spent Sticky's own budget arithmetic is unchanged and already covered; what is new here is only where the number comes from. --- doc/howto/QUICKSTART.md | 17 ++++- service/submitqueue/demo/provider/README.md | 2 +- .../demo/provider/fake/profiles.yaml | 5 ++ .../submitqueue/orchestrator/server/config.go | 44 ++++++++++- .../orchestrator/server/config_test.go | 76 +++++++++++++++++++ .../orchestrator/server/profiles.go | 16 ++-- .../orchestrator/server/profiles_test.go | 4 +- 7 files changed, 148 insertions(+), 16 deletions(-) diff --git a/doc/howto/QUICKSTART.md b/doc/howto/QUICKSTART.md index 70e06f32..b00382ed 100644 --- a/doc/howto/QUICKSTART.md +++ b/doc/howto/QUICKSTART.md @@ -70,6 +70,21 @@ Every change writes all of its files into one folder under `demo/`, and `FOLDERS Set it deliberately when you want a run to show one thing. `FOLDERS=1` puts every change in the same place, so the queue serializes the lot and each change speculates on the one before it. A number well above `COUNT` keeps them all apart, so they go out together. +How much speculation that turns into is capped by the queue's **build budget** — how many builds it may have occupying CI at once, counted across every in-flight batch rather than per batch. It defaults to 4 and is set per queue in the provider's `profiles.yaml`: + +```yaml +defaults: + speculator: {buildBudget: 4} + +queues: + - name: demo-queue + speculator: {buildBudget: 12} +``` + +It is the other half of `FOLDERS`. Folders decide how many dependencies there are to speculate *about*; the budget decides how many of the possible outcomes the queue may hedge at once. `FOLDERS=1 buildBudget: 1` explores one path at a time and lands the slowest; raising the budget lets the queue build the "it fails" branch alongside the "it succeeds" one, which is what makes a failure cost nothing. A trail like `speculating [building ×8, built ×8]` below is a queue that kept finding paths worth funding. + +Changing it needs a restart, since the file is read at startup — `make local-submitqueue-stop && make local-submitqueue-start`. + You can watch the queue reach that conclusion: ```bash @@ -108,7 +123,7 @@ accepted → started → validating → validated → batching → batched → speculating [building ×8, built ×8, waiting] → speculated → landing → landed ``` -Eight builds means the batch was speculating down eight paths at once, and `waiting` means one of them passed and then sat on a dependency that had not resolved. A request that sailed through reads `speculating [building, built]` instead — the same position, a very different amount of work behind it. +Eight builds means the batch explored eight paths before one of them landed it — not eight at the same time, since the build budget above caps how many may hold CI at once and a finished build frees its slot for the next. `waiting` means a path passed and then sat on a dependency that had not resolved. A request that sailed through reads `speculating [building, built]` instead — the same position, a very different amount of work behind it. `land-watch` fixes its set when it starts and exits non-zero if any request in that set finishes anywhere other than `landed`, which makes it usable from a script. A request accepted after the watch begins is not picked up: a watch that grew as the queue did would never finish. diff --git a/service/submitqueue/demo/provider/README.md b/service/submitqueue/demo/provider/README.md index cf8a95fc..679f0122 100644 --- a/service/submitqueue/demo/provider/README.md +++ b/service/submitqueue/demo/provider/README.md @@ -4,7 +4,7 @@ Each directory here is one **provider** — a code-hosting system SubmitQueue la | File | Selects | |---|---| -| `profiles.yaml` | the change provider, build runner, and conflict analyzer each queue resolves to (read by the orchestrator) | +| `profiles.yaml` | the change provider, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) | | `merge.yaml` | the merge target each queue lands on (read by Runway) | Neither holds a secret. Each integration names the *environment variable* carrying its credential, so these files stay committable and rotating a token needs no edit. diff --git a/service/submitqueue/demo/provider/fake/profiles.yaml b/service/submitqueue/demo/provider/fake/profiles.yaml index a0e51383..5b1fb27b 100644 --- a/service/submitqueue/demo/provider/fake/profiles.yaml +++ b/service/submitqueue/demo/provider/fake/profiles.yaml @@ -17,6 +17,11 @@ defaults: buildRunner: {type: fake} # Serialize conservatively unless a queue says otherwise. analyzer: {type: all} + # How many builds a queue may have occupying CI at once, across all of its + # in-flight batches. This is the dial on how much speculation a run shows: at + # 1 the queue explores one path at a time, and raising it lets it hedge more + # of the outcomes it is waiting on. Four is the built-in default. + speculator: {buildBudget: 4} queues: # The queue `make demo-requests` and `make land` use by default. diff --git a/service/submitqueue/orchestrator/server/config.go b/service/submitqueue/orchestrator/server/config.go index f3956009..d27c2a77 100644 --- a/service/submitqueue/orchestrator/server/config.go +++ b/service/submitqueue/orchestrator/server/config.go @@ -66,6 +66,12 @@ const ( // Ways a composite scorer combines its components. const combineAvg = "avg" +// defaultBuildBudget is how many builds a queue may have occupying CI at once +// when it states no budget of its own. Four is enough for speculation to be +// visible — a queue that can only build one path never speculates — while +// staying well inside what a modest CI pool absorbs. +const defaultBuildBudget = 4 + // Defaults for the provider integrations, matching each vendor's convention. const ( defaultGitHubTokenEnv = "GITHUB_TOKEN" @@ -98,6 +104,7 @@ type namedQueueProfileConfig struct { BuildRunner *buildRunnerConfig `yaml:"buildRunner"` Analyzer *analyzerConfig `yaml:"analyzer"` Scorer *scorerConfig `yaml:"scorer"` + Speculator *speculatorConfig `yaml:"speculator"` } // queueProfileConfig is the full set of extensions a queue resolves to. @@ -106,6 +113,7 @@ type queueProfileConfig struct { BuildRunner buildRunnerConfig `yaml:"buildRunner"` Analyzer analyzerConfig `yaml:"analyzer"` Scorer scorerConfig `yaml:"scorer"` + Speculator speculatorConfig `yaml:"speculator"` } // changeProviderConfig selects how change metadata is fetched. The github and @@ -189,6 +197,16 @@ type bucketConfig struct { Score float64 `yaml:"score"` } +// speculatorConfig tunes how much CI a queue's speculation may occupy. It has no +// `type`: there is one speculator, composed from the queue's scorer, and what +// varies between queues is what it is allowed to spend. +type speculatorConfig struct { + // BuildBudget caps how many builds this queue may have occupying CI at once, + // counted across every in-flight batch rather than per batch. Absent or 0 + // takes defaultBuildBudget; must not be negative. + BuildBudget int `yaml:"buildBudget"` +} + // loadProfilesConfig reads and validates the profiles configuration at path. func loadProfilesConfig(path string) (profilesConfig, error) { data, err := os.ReadFile(path) @@ -245,6 +263,11 @@ func (c *profilesConfig) normalizeAndValidate() error { return err } } + if q.Speculator != nil { + if err := q.Speculator.normalizeAndValidate(where); err != nil { + return err + } + } } return nil } @@ -265,6 +288,9 @@ func (c profilesConfig) resolve(q namedQueueProfileConfig) queueProfileConfig { if q.Scorer != nil { profile.Scorer = *q.Scorer } + if q.Speculator != nil { + profile.Speculator = *q.Speculator + } return profile } @@ -278,7 +304,10 @@ func (p *queueProfileConfig) normalizeAndValidate(where string) error { if err := p.Analyzer.normalizeAndValidate(where); err != nil { return err } - return p.Scorer.normalizeAndValidate(where) + if err := p.Scorer.normalizeAndValidate(where); err != nil { + return err + } + return p.Speculator.normalizeAndValidate(where) } func (c *changeProviderConfig) normalizeAndValidate(where string) error { @@ -433,6 +462,19 @@ func (s *scorerConfig) normalizeAndValidate(where string) error { return nil } +func (s *speculatorConfig) normalizeAndValidate(where string) error { + // A negative budget is rejected rather than clamped: sticky would compute no + // free slots from it, so the queue would batch and then never build anything, + // which looks like a stuck queue rather than a misconfigured one. + if s.BuildBudget < 0 { + return fmt.Errorf("%s: build budget %d is negative", where, s.BuildBudget) + } + if s.BuildBudget == 0 { + s.BuildBudget = defaultBuildBudget + } + return nil +} + // timeoutOr parses a Go duration string, falling back when it is empty or // unparseable — a bad value should not stop the service from starting. func timeoutOr(value string, fallback time.Duration) time.Duration { diff --git a/service/submitqueue/orchestrator/server/config_test.go b/service/submitqueue/orchestrator/server/config_test.go index a980d6f7..7ce27917 100644 --- a/service/submitqueue/orchestrator/server/config_test.go +++ b/service/submitqueue/orchestrator/server/config_test.go @@ -16,6 +16,7 @@ package main import ( "context" + "fmt" "os" "path/filepath" "testing" @@ -27,6 +28,7 @@ import ( "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/conflict" + "github.com/uber/submitqueue/submitqueue/extension/speculation/speculator" ) func writeProfiles(t *testing.T, contents string) string { @@ -375,6 +377,80 @@ func TestNewProfiles_ComposesASpeculatorPerQueue(t *testing.T) { } } +// TestNewProfiles_SpendsTheConfiguredBuildBudget is the assertion that matters +// for the setting: parsing a number proves nothing if it never reaches the +// allocator, so this drives a real speculator and counts what it proposes. +// +// Each batch speculates with no dependencies, so every one is a candidate and +// the only thing capping the proposals is the budget. +func TestNewProfiles_SpendsTheConfiguredBuildBudget(t *testing.T) { + path := writeProfiles(t, ` +defaults: + speculator: {buildBudget: 2} +queues: + - name: wide-queue + speculator: {buildBudget: 5} + - name: inherits-queue + analyzer: {type: none} +`) + cfg, err := loadProfilesConfig(path) + require.NoError(t, err) + profiles, err := newProfiles(zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) + require.NoError(t, err) + + batches := make([]entity.Batch, 0, 8) + for i := range 8 { + batches = append(batches, entity.Batch{ + ID: fmt.Sprintf("b%d", i), + State: entity.BatchStateSpeculating, + }) + } + + for _, tt := range []struct { + queue string + want int + }{ + {queue: "wide-queue", want: 5}, + {queue: "inherits-queue", want: 2}, + {queue: "unlisted-queue", want: 2}, + } { + t.Run(tt.queue, func(t *testing.T) { + spec, err := profiles.SpeculatorFactory().For(speculator.Config{QueueName: tt.queue}) + require.NoError(t, err) + + proposals, err := spec.Speculate(context.Background(), batches, nil) + require.NoError(t, err) + assert.Len(t, proposals, tt.want) + }) + } +} + +func TestLoadProfilesConfig_RejectsBudgets(t *testing.T) { + // A negative budget leaves sticky with no free slots forever, so a queue + // would batch and then never build — indistinguishable from a stuck queue. + path := writeProfiles(t, ` +defaults: + speculator: {buildBudget: -1} +`) + _, err := loadProfilesConfig(path) + require.Error(t, err) +} + +func TestLoadProfilesConfig_DefaultsAnUnstatedBudget(t *testing.T) { + path := writeProfiles(t, ` +defaults: {} +queues: + - name: q + speculator: {buildBudget: 9} +`) + cfg, err := loadProfilesConfig(path) + require.NoError(t, err) + + assert.Equal(t, defaultBuildBudget, cfg.Defaults.Speculator.BuildBudget) + require.NotNil(t, cfg.Queues[0].Speculator) + assert.Equal(t, 9, cfg.Queues[0].Speculator.BuildBudget) +} + func TestLoadProfilesConfig_RejectsBadScorers(t *testing.T) { tests := []struct { name string diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index 5109b732..578fa758 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -219,6 +219,7 @@ func newProfiles( zap.String("default_build_runner", cfg.Defaults.BuildRunner.Type), zap.String("default_analyzer", cfg.Defaults.Analyzer.Type), zap.String("default_scorer", cfg.Defaults.Scorer.Type), + zap.Int("default_build_budget", cfg.Defaults.Speculator.BuildBudget), zap.Int("queue_overrides", len(byQueue)), ) return Profiles{defaultProfile: defaultProfile, byQueue: byQueue}, nil @@ -265,32 +266,25 @@ func (b *profileBuilder) build(cfg queueProfileConfig, where string) (Profile, e Analyzer: analyzer, Storage: b.stores, Scorer: sc, - }), nil + }, cfg.Speculator.BuildBudget), nil } -// defaultBuildBudget caps how many builds a queue may have occupying CI at -// once. It is the only rationing lever the allocator has. -// -// TODO: move this onto entity.QueueConfig so operators can tune it per queue -// without a code change. QueueConfig carries only the queue name today. -const defaultBuildBudget = 4 - // withSpeculator returns the profile with its speculator composed from its own // scorer: bestfirst ranks a queue's candidate paths by how likely all their -// assumptions are to hold, and sticky spends the build budget down that ranking +// assumptions are to hold, and sticky spends buildBudget down that ranking // without preempting builds already running. Swapping either part changes the // policy without touching the speculate controller, which depends only on the // Speculator contract. // // The scorer is resolved lazily, at the queue the speculator itself was asked // for, so the queue's identity reaches one level down into the scorer too. -func withSpeculator(p Profile) Profile { +func withSpeculator(p Profile, buildBudget int) Profile { p.Speculator = speculatorFunc(func(c speculator.Config) (speculator.Speculator, error) { sc, err := p.Scorer.For(scorer.Config{QueueName: c.QueueName}) if err != nil { return nil, fmt.Errorf("failed to resolve scorer for queue %q: %w", c.QueueName, err) } - return specstandard.New(c, bestfirst.New(sc), sticky.New(defaultBuildBudget)), nil + return specstandard.New(c, bestfirst.New(sc), sticky.New(buildBudget)), nil }) return p } diff --git a/service/submitqueue/orchestrator/server/profiles_test.go b/service/submitqueue/orchestrator/server/profiles_test.go index f298ce77..a8b07adb 100644 --- a/service/submitqueue/orchestrator/server/profiles_test.go +++ b/service/submitqueue/orchestrator/server/profiles_test.go @@ -118,7 +118,7 @@ func TestProfilesForwardQueueNameToFactories(t *testing.T) { // ask for it at the queue it was itself asked for. func TestWithSpeculatorResolvesScorerAtSameQueue(t *testing.T) { var rec recorder - profile := withSpeculator(profileRecording(&rec)) + profile := withSpeculator(profileRecording(&rec), defaultBuildBudget) profiles := Profiles{defaultProfile: profile} spec, err := profiles.SpeculatorFactory().For(speculator.Config{QueueName: "unlisted-queue"}) @@ -135,7 +135,7 @@ func TestWithSpeculatorPropagatesScorerError(t *testing.T) { sentinel := errors.New("scorer unavailable") profile := withSpeculator(Profile{ Scorer: scorerFunc(func(scorer.Config) (scorer.Scorer, error) { return nil, sentinel }), - }) + }, defaultBuildBudget) spec, err := profile.Speculator.For(speculator.Config{QueueName: "any-queue"}) require.ErrorIs(t, err, sentinel)