Skip to content

Commit 86635af

Browse files
authored
feat(orchestrator): make the build budget a per-queue setting (#602)
## 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.
1 parent 4a13ed1 commit 86635af

7 files changed

Lines changed: 148 additions & 16 deletions

File tree

doc/howto/QUICKSTART.md

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,21 @@ Every change writes all of its files into one folder under `demo/`, and `FOLDERS
7070

7171
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.
7272

73+
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`:
74+
75+
```yaml
76+
defaults:
77+
speculator: {buildBudget: 4}
78+
79+
queues:
80+
- name: demo-queue
81+
speculator: {buildBudget: 12}
82+
```
83+
84+
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.
85+
86+
Changing it needs a restart, since the file is read at startup — `make local-submitqueue-stop && make local-submitqueue-start`.
87+
7388
You can watch the queue reach that conclusion:
7489

7590
```bash
@@ -108,7 +123,7 @@ accepted → started → validating → validated → batching → batched →
108123
speculating [building ×8, built ×8, waiting] → speculated → landing → landed
109124
```
110125

111-
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.
126+
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.
112127

113128
`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.
114129

service/submitqueue/demo/provider/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Each directory here is one **provider** — a code-hosting system SubmitQueue la
44

55
| File | Selects |
66
|---|---|
7-
| `profiles.yaml` | the change provider, build runner, and conflict analyzer each queue resolves to (read by the orchestrator) |
7+
| `profiles.yaml` | the change provider, build runner, conflict analyzer, scorer and build budget each queue resolves to (read by the orchestrator) |
88
| `merge.yaml` | the merge target each queue lands on (read by Runway) |
99

1010
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.

service/submitqueue/demo/provider/fake/profiles.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,11 @@ defaults:
1717
buildRunner: {type: fake}
1818
# Serialize conservatively unless a queue says otherwise.
1919
analyzer: {type: all}
20+
# How many builds a queue may have occupying CI at once, across all of its
21+
# in-flight batches. This is the dial on how much speculation a run shows: at
22+
# 1 the queue explores one path at a time, and raising it lets it hedge more
23+
# of the outcomes it is waiting on. Four is the built-in default.
24+
speculator: {buildBudget: 4}
2025

2126
queues:
2227
# The queue `make demo-requests` and `make land` use by default.

service/submitqueue/orchestrator/server/config.go

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,12 @@ const (
6666
// Ways a composite scorer combines its components.
6767
const combineAvg = "avg"
6868

69+
// defaultBuildBudget is how many builds a queue may have occupying CI at once
70+
// when it states no budget of its own. Four is enough for speculation to be
71+
// visible — a queue that can only build one path never speculates — while
72+
// staying well inside what a modest CI pool absorbs.
73+
const defaultBuildBudget = 4
74+
6975
// Defaults for the provider integrations, matching each vendor's convention.
7076
const (
7177
defaultGitHubTokenEnv = "GITHUB_TOKEN"
@@ -98,6 +104,7 @@ type namedQueueProfileConfig struct {
98104
BuildRunner *buildRunnerConfig `yaml:"buildRunner"`
99105
Analyzer *analyzerConfig `yaml:"analyzer"`
100106
Scorer *scorerConfig `yaml:"scorer"`
107+
Speculator *speculatorConfig `yaml:"speculator"`
101108
}
102109

103110
// queueProfileConfig is the full set of extensions a queue resolves to.
@@ -106,6 +113,7 @@ type queueProfileConfig struct {
106113
BuildRunner buildRunnerConfig `yaml:"buildRunner"`
107114
Analyzer analyzerConfig `yaml:"analyzer"`
108115
Scorer scorerConfig `yaml:"scorer"`
116+
Speculator speculatorConfig `yaml:"speculator"`
109117
}
110118

111119
// changeProviderConfig selects how change metadata is fetched. The github and
@@ -189,6 +197,16 @@ type bucketConfig struct {
189197
Score float64 `yaml:"score"`
190198
}
191199

200+
// speculatorConfig tunes how much CI a queue's speculation may occupy. It has no
201+
// `type`: there is one speculator, composed from the queue's scorer, and what
202+
// varies between queues is what it is allowed to spend.
203+
type speculatorConfig struct {
204+
// BuildBudget caps how many builds this queue may have occupying CI at once,
205+
// counted across every in-flight batch rather than per batch. Absent or 0
206+
// takes defaultBuildBudget; must not be negative.
207+
BuildBudget int `yaml:"buildBudget"`
208+
}
209+
192210
// loadProfilesConfig reads and validates the profiles configuration at path.
193211
func loadProfilesConfig(path string) (profilesConfig, error) {
194212
data, err := os.ReadFile(path)
@@ -245,6 +263,11 @@ func (c *profilesConfig) normalizeAndValidate() error {
245263
return err
246264
}
247265
}
266+
if q.Speculator != nil {
267+
if err := q.Speculator.normalizeAndValidate(where); err != nil {
268+
return err
269+
}
270+
}
248271
}
249272
return nil
250273
}
@@ -265,6 +288,9 @@ func (c profilesConfig) resolve(q namedQueueProfileConfig) queueProfileConfig {
265288
if q.Scorer != nil {
266289
profile.Scorer = *q.Scorer
267290
}
291+
if q.Speculator != nil {
292+
profile.Speculator = *q.Speculator
293+
}
268294
return profile
269295
}
270296

@@ -278,7 +304,10 @@ func (p *queueProfileConfig) normalizeAndValidate(where string) error {
278304
if err := p.Analyzer.normalizeAndValidate(where); err != nil {
279305
return err
280306
}
281-
return p.Scorer.normalizeAndValidate(where)
307+
if err := p.Scorer.normalizeAndValidate(where); err != nil {
308+
return err
309+
}
310+
return p.Speculator.normalizeAndValidate(where)
282311
}
283312

284313
func (c *changeProviderConfig) normalizeAndValidate(where string) error {
@@ -433,6 +462,19 @@ func (s *scorerConfig) normalizeAndValidate(where string) error {
433462
return nil
434463
}
435464

465+
func (s *speculatorConfig) normalizeAndValidate(where string) error {
466+
// A negative budget is rejected rather than clamped: sticky would compute no
467+
// free slots from it, so the queue would batch and then never build anything,
468+
// which looks like a stuck queue rather than a misconfigured one.
469+
if s.BuildBudget < 0 {
470+
return fmt.Errorf("%s: build budget %d is negative", where, s.BuildBudget)
471+
}
472+
if s.BuildBudget == 0 {
473+
s.BuildBudget = defaultBuildBudget
474+
}
475+
return nil
476+
}
477+
436478
// timeoutOr parses a Go duration string, falling back when it is empty or
437479
// unparseable — a bad value should not stop the service from starting.
438480
func timeoutOr(value string, fallback time.Duration) time.Duration {

service/submitqueue/orchestrator/server/config_test.go

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ package main
1616

1717
import (
1818
"context"
19+
"fmt"
1920
"os"
2021
"path/filepath"
2122
"testing"
@@ -27,6 +28,7 @@ import (
2728

2829
"github.com/uber/submitqueue/submitqueue/entity"
2930
"github.com/uber/submitqueue/submitqueue/extension/conflict"
31+
"github.com/uber/submitqueue/submitqueue/extension/speculation/speculator"
3032
)
3133

3234
func writeProfiles(t *testing.T, contents string) string {
@@ -375,6 +377,80 @@ func TestNewProfiles_ComposesASpeculatorPerQueue(t *testing.T) {
375377
}
376378
}
377379

380+
// TestNewProfiles_SpendsTheConfiguredBuildBudget is the assertion that matters
381+
// for the setting: parsing a number proves nothing if it never reaches the
382+
// allocator, so this drives a real speculator and counts what it proposes.
383+
//
384+
// Each batch speculates with no dependencies, so every one is a candidate and
385+
// the only thing capping the proposals is the budget.
386+
func TestNewProfiles_SpendsTheConfiguredBuildBudget(t *testing.T) {
387+
path := writeProfiles(t, `
388+
defaults:
389+
speculator: {buildBudget: 2}
390+
queues:
391+
- name: wide-queue
392+
speculator: {buildBudget: 5}
393+
- name: inherits-queue
394+
analyzer: {type: none}
395+
`)
396+
cfg, err := loadProfilesConfig(path)
397+
require.NoError(t, err)
398+
profiles, err := newProfiles(zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg)
399+
require.NoError(t, err)
400+
401+
batches := make([]entity.Batch, 0, 8)
402+
for i := range 8 {
403+
batches = append(batches, entity.Batch{
404+
ID: fmt.Sprintf("b%d", i),
405+
State: entity.BatchStateSpeculating,
406+
})
407+
}
408+
409+
for _, tt := range []struct {
410+
queue string
411+
want int
412+
}{
413+
{queue: "wide-queue", want: 5},
414+
{queue: "inherits-queue", want: 2},
415+
{queue: "unlisted-queue", want: 2},
416+
} {
417+
t.Run(tt.queue, func(t *testing.T) {
418+
spec, err := profiles.SpeculatorFactory().For(speculator.Config{QueueName: tt.queue})
419+
require.NoError(t, err)
420+
421+
proposals, err := spec.Speculate(context.Background(), batches, nil)
422+
require.NoError(t, err)
423+
assert.Len(t, proposals, tt.want)
424+
})
425+
}
426+
}
427+
428+
func TestLoadProfilesConfig_RejectsBudgets(t *testing.T) {
429+
// A negative budget leaves sticky with no free slots forever, so a queue
430+
// would batch and then never build — indistinguishable from a stuck queue.
431+
path := writeProfiles(t, `
432+
defaults:
433+
speculator: {buildBudget: -1}
434+
`)
435+
_, err := loadProfilesConfig(path)
436+
require.Error(t, err)
437+
}
438+
439+
func TestLoadProfilesConfig_DefaultsAnUnstatedBudget(t *testing.T) {
440+
path := writeProfiles(t, `
441+
defaults: {}
442+
queues:
443+
- name: q
444+
speculator: {buildBudget: 9}
445+
`)
446+
cfg, err := loadProfilesConfig(path)
447+
require.NoError(t, err)
448+
449+
assert.Equal(t, defaultBuildBudget, cfg.Defaults.Speculator.BuildBudget)
450+
require.NotNil(t, cfg.Queues[0].Speculator)
451+
assert.Equal(t, 9, cfg.Queues[0].Speculator.BuildBudget)
452+
}
453+
378454
func TestLoadProfilesConfig_RejectsBadScorers(t *testing.T) {
379455
tests := []struct {
380456
name string

service/submitqueue/orchestrator/server/profiles.go

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ func newProfiles(
219219
zap.String("default_build_runner", cfg.Defaults.BuildRunner.Type),
220220
zap.String("default_analyzer", cfg.Defaults.Analyzer.Type),
221221
zap.String("default_scorer", cfg.Defaults.Scorer.Type),
222+
zap.Int("default_build_budget", cfg.Defaults.Speculator.BuildBudget),
222223
zap.Int("queue_overrides", len(byQueue)),
223224
)
224225
return Profiles{defaultProfile: defaultProfile, byQueue: byQueue}, nil
@@ -265,32 +266,25 @@ func (b *profileBuilder) build(cfg queueProfileConfig, where string) (Profile, e
265266
Analyzer: analyzer,
266267
Storage: b.stores,
267268
Scorer: sc,
268-
}), nil
269+
}, cfg.Speculator.BuildBudget), nil
269270
}
270271

271-
// defaultBuildBudget caps how many builds a queue may have occupying CI at
272-
// once. It is the only rationing lever the allocator has.
273-
//
274-
// TODO: move this onto entity.QueueConfig so operators can tune it per queue
275-
// without a code change. QueueConfig carries only the queue name today.
276-
const defaultBuildBudget = 4
277-
278272
// withSpeculator returns the profile with its speculator composed from its own
279273
// scorer: bestfirst ranks a queue's candidate paths by how likely all their
280-
// assumptions are to hold, and sticky spends the build budget down that ranking
274+
// assumptions are to hold, and sticky spends buildBudget down that ranking
281275
// without preempting builds already running. Swapping either part changes the
282276
// policy without touching the speculate controller, which depends only on the
283277
// Speculator contract.
284278
//
285279
// The scorer is resolved lazily, at the queue the speculator itself was asked
286280
// for, so the queue's identity reaches one level down into the scorer too.
287-
func withSpeculator(p Profile) Profile {
281+
func withSpeculator(p Profile, buildBudget int) Profile {
288282
p.Speculator = speculatorFunc(func(c speculator.Config) (speculator.Speculator, error) {
289283
sc, err := p.Scorer.For(scorer.Config{QueueName: c.QueueName})
290284
if err != nil {
291285
return nil, fmt.Errorf("failed to resolve scorer for queue %q: %w", c.QueueName, err)
292286
}
293-
return specstandard.New(c, bestfirst.New(sc), sticky.New(defaultBuildBudget)), nil
287+
return specstandard.New(c, bestfirst.New(sc), sticky.New(buildBudget)), nil
294288
})
295289
return p
296290
}

service/submitqueue/orchestrator/server/profiles_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func TestProfilesForwardQueueNameToFactories(t *testing.T) {
118118
// ask for it at the queue it was itself asked for.
119119
func TestWithSpeculatorResolvesScorerAtSameQueue(t *testing.T) {
120120
var rec recorder
121-
profile := withSpeculator(profileRecording(&rec))
121+
profile := withSpeculator(profileRecording(&rec), defaultBuildBudget)
122122
profiles := Profiles{defaultProfile: profile}
123123

124124
spec, err := profiles.SpeculatorFactory().For(speculator.Config{QueueName: "unlisted-queue"})
@@ -135,7 +135,7 @@ func TestWithSpeculatorPropagatesScorerError(t *testing.T) {
135135
sentinel := errors.New("scorer unavailable")
136136
profile := withSpeculator(Profile{
137137
Scorer: scorerFunc(func(scorer.Config) (scorer.Scorer, error) { return nil, sentinel }),
138-
})
138+
}, defaultBuildBudget)
139139

140140
spec, err := profile.Speculator.For(speculator.Config{QueueName: "any-queue"})
141141
require.ErrorIs(t, err, sentinel)

0 commit comments

Comments
 (0)