Skip to content

Commit 00cfaab

Browse files
committed
feat(buildrunner): drop per-call queueName, add Factory
## Summary ### Why? `BuildRunner.Trigger` took a `queueName` argument that selected the runner-specific job configuration on every call. That put queue routing on the hot path and on a single verb, leaving `Status` and `Cancel` to rediscover the queue from the build ID. A runner's connection pools, caches, and job defaults are all keyed to one queue's configuration, so the queue belongs at construction time, not per call. ### What? - Drop `queueName` from `BuildRunner.Trigger`; the verbs now speak only in builds and changes. - Add a `Factory` interface (`New(cfg Config) (BuildRunner, error)`) and a placeholder `Config` struct. A runner is bound to one queue's job configuration at construction. `Config` is intentionally empty for now — its fields (queue/job selection plus backend settings) land with the first real backend. - noop: add `NewFactory()` + factory `New`; keep `New()` so existing wiring is untouched. - build controller: call site drops `batch.Queue`. - Update the build-runner RFC (new Construction section, Interface and Lifecycle updates), the extension README, and regenerate the mock. ## Test Plan ✅ `make mocks`, `make gazelle`, `make fmt` ✅ `bazel build //...` ✅ `bazel test //extension/buildrunner/... //orchestrator/controller/build/...`
1 parent 1106427 commit 00cfaab

9 files changed

Lines changed: 126 additions & 21 deletions

File tree

doc/rfc/build-runner.md

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,27 @@ The build stage needs a vendor-agnostic abstraction for talking to a Build Runne
3838

3939
`BuildRunner` exposes three verbs, all keyed by a build identifier (`entity.BuildID`):
4040

41-
- **`Trigger`** — submit a build for a queue, given the ordered `base` and `head` change sets plus a free-form metadata map; returns the new build's ID. Runner-side work is asynchronous.
41+
- **`Trigger`** — submit a build given the ordered `base` and `head` change sets plus a free-form metadata map; returns the new build's ID. Runner-side work is asynchronous.
4242
- **`Status`** — fetch the current `BuildStatus` and runner-defined metadata for a build; MAY round-trip to the runner.
4343
- **`Cancel`** — request cancellation; returns once the request reaches the runner, not once the build stops.
4444

4545
See `extension/buildrunner/build_runner.go` for the exact Go signatures. The sections below record why the contract is shaped this way.
4646

47+
### Construction: a Factory, queue bound at build time
48+
49+
A `BuildRunner` does not take a queue selector on any verb. The queue whose job configuration a runner uses is fixed when the runner is constructed, and runners are constructed by a `Factory`.
50+
51+
- **`Factory`** — produces `BuildRunner` instances from a `Config`. A controller that drives builds for several queues holds one `Factory` and obtains one `BuildRunner` per queue.
52+
- **`Config`** — the per-runner configuration the factory binds in: the queue's job selection plus any backend-specific settings (endpoints, credentials, defaults). The schema is backend-defined and lands with the first real implementation; today it is an intentionally empty placeholder so the `Factory` contract can stabilize ahead of it.
53+
54+
Why bind the queue at construction rather than pass it per call:
55+
56+
- A runner's connection pool, caches, and job defaults are all keyed to one queue's configuration. Passing the queue per call would force every implementation to re-resolve that configuration on the hot path, or to maintain an internal queue→config map the factory already expresses cleanly.
57+
- It keeps the per-call verbs (`Trigger`, `Status`, `Cancel`) free of routing concerns — they speak only in builds and changes.
58+
- It matches the rest of the extension family, whose implementations are long-lived singletons bound to their configuration at construction.
59+
60+
Rejected: a `queueName` argument on `Trigger`. It put routing on the hot path and on a single verb, leaving `Status` and `Cancel` to rediscover the queue from the build ID. Moving the selection into `Config` makes one runner mean one queue everywhere.
61+
4762
### Trigger: base + head
4863

4964
`Trigger` takes two ordered lists of changes and a free-form metadata map:
@@ -125,7 +140,7 @@ Rejected: long-polling on `Status`. Not every backend supports efficient server-
125140

126141
### Lifecycle
127142

128-
Implementations are long-lived singletons bound to provider config at construction. Every method is concurrent-safe; connection pools and caches live inside the manager; anything that must survive a restart belongs in persistent storage, not the manager.
143+
Implementations are long-lived singletons constructed by a `Factory` and bound to one queue's provider config at construction (see *Construction* above). Every method is concurrent-safe; connection pools and caches live inside the manager; anything that must survive a restart belongs in persistent storage, not the manager.
129144

130145
### Transient failures
131146

extension/buildrunner/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ See [`doc/rfc/build-runner.md`](../../doc/rfc/build-runner.md) for the contract
66

77
## Adding a new backend
88

9-
1. Create `extension/buildrunner/{backend}/` with a `BuildRunner` implementation bound to its runner configuration at construction.
9+
1. Create `extension/buildrunner/{backend}/` with a `Factory` whose `New` returns a `BuildRunner` bound to one queue's job configuration. The runner verbs carry no queue selector — that selection lives in the `Config` passed to the factory.
1010
2. Map the `base` and `head` change slices onto the backend's build primitives (apply `base`, apply `head`, validate the result).
1111
3. Map the runner's lifecycle states down to the `BuildStatus` values: `Accepted` (accepted for execution), `Running` (executing), and the terminal `Succeeded` / `Failed` / `Cancelled`.
1212
4. Implement internal reconnect / retry so transient failures surface as plain errors without blocking the caller.

extension/buildrunner/build_runner.go

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,35 @@ import (
2222
"github.com/uber/submitqueue/entity"
2323
)
2424

25+
// Config carries the per-runner configuration a Factory binds into a
26+
// BuildRunner: the queue's job selection that Trigger used to receive via a
27+
// queueName argument, plus any backend-specific settings (endpoints,
28+
// credentials, defaults). Its fields are defined by the concrete backend
29+
// and land with the first real implementation; today it is an
30+
// intentionally empty placeholder so the Factory contract can stabilize
31+
// ahead of them.
32+
type Config struct{}
33+
34+
// Factory constructs BuildRunner instances bound to a Config.
35+
//
36+
// A BuildRunner is bound at construction to a single Config — in particular
37+
// to one queue's job configuration — which is why Trigger no longer carries
38+
// a queueName: the routing that argument expressed now lives in Config and
39+
// is fixed when the runner is built. A controller that serves multiple
40+
// queues holds a Factory and obtains one BuildRunner per queue.
41+
//
42+
// Implementations must be safe for concurrent use by multiple goroutines.
43+
type Factory interface {
44+
// New returns a BuildRunner bound to cfg, ready to trigger builds.
45+
// Returns an error if a runner cannot be constructed from cfg (e.g.
46+
// invalid configuration or an unreachable backend).
47+
New(cfg Config) (BuildRunner, error)
48+
}
49+
2550
// BuildRunner triggers builds against an external Build Runner, queries
26-
// their status, and cancels them.
51+
// their status, and cancels them. A BuildRunner is bound to a single
52+
// queue's job configuration at construction (see Factory); the verbs below
53+
// carry no queue selector.
2754
//
2855
// Implementations are long-lived singletons and must:
2956
// - make every method safe for concurrent use by multiple goroutines;
@@ -55,11 +82,11 @@ type BuildRunner interface {
5582
// asynchronously. Callers learn the build's progress via Status, not
5683
// via Trigger.
5784
//
58-
// queueName selects the runner-specific job configuration.
59-
// Returns an error if the request is invalid.
85+
// The queue whose job configuration this runner uses is fixed at
86+
// construction (see Factory and Config); Trigger does not take a queue
87+
// selector. Returns an error if the request is invalid.
6088
Trigger(
6189
ctx context.Context,
62-
queueName string,
6390
base []entity.Change,
6491
head []entity.Change,
6592
metadata entity.BuildMetadata,

extension/buildrunner/mock/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ go_library(
77
visibility = ["//visibility:public"],
88
deps = [
99
"//entity",
10+
"//extension/buildrunner",
1011
"@org_uber_go_mock//gomock",
1112
],
1213
)

extension/buildrunner/mock/build_runner_mock.go

Lines changed: 44 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

extension/buildrunner/noop/noop.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,27 @@ type runner struct {
3434
counter atomic.Uint64
3535
}
3636

37+
// factory builds no-op runners. It ignores the supplied Config.
38+
type factory struct{}
39+
40+
// NewFactory returns a buildrunner.Factory that produces no-op runners.
41+
func NewFactory() buildrunner.Factory {
42+
return factory{}
43+
}
44+
45+
// New returns a no-op buildrunner.BuildRunner. The Config is ignored.
46+
func (factory) New(_ buildrunner.Config) (buildrunner.BuildRunner, error) {
47+
return New(), nil
48+
}
49+
3750
// New returns a buildrunner.BuildRunner that performs no real work.
3851
func New() buildrunner.BuildRunner {
3952
return &runner{}
4053
}
4154

4255
// Trigger returns a unique build ID without contacting any runner.
4356
// Inputs are ignored.
44-
func (r *runner) Trigger(_ context.Context, _ string, _ []entity.Change, _ []entity.Change, _ entity.BuildMetadata) (entity.BuildID, error) {
57+
func (r *runner) Trigger(_ context.Context, _ []entity.Change, _ []entity.Change, _ entity.BuildMetadata) (entity.BuildID, error) {
4558
return entity.BuildID{ID: fmt.Sprintf("noop-%d", r.counter.Add(1))}, nil
4659
}
4760

extension/buildrunner/noop/noop_test.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,18 @@ func TestNew_ImplementsInterface(t *testing.T) {
2828
var _ buildrunner.BuildRunner = New()
2929
}
3030

31+
func TestNewFactory_ImplementsInterface(t *testing.T) {
32+
var f buildrunner.Factory = NewFactory()
33+
r, err := f.New(buildrunner.Config{})
34+
require.NoError(t, err)
35+
assert.NotNil(t, r)
36+
}
37+
3138
func TestRunner_Trigger(t *testing.T) {
3239
r := New()
3340
ctx := context.Background()
3441

35-
id1, err := r.Trigger(ctx, "queueA",
42+
id1, err := r.Trigger(ctx,
3643
[]entity.Change{{URIs: []string{"github://owner/repo/pull/1"}}},
3744
[]entity.Change{{URIs: []string{"github://owner/repo/pull/2"}}},
3845
entity.BuildMetadata{"requester": "alice"},
@@ -41,7 +48,7 @@ func TestRunner_Trigger(t *testing.T) {
4148
assert.NotEmpty(t, id1.ID)
4249

4350
// IDs are unique across calls, even with empty inputs.
44-
id2, err := r.Trigger(ctx, "queueA", nil, nil, nil)
51+
id2, err := r.Trigger(ctx, nil, nil, nil)
4552
require.NoError(t, err)
4653
assert.NotEqual(t, id1, id2)
4754
}

orchestrator/controller/build/build.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,10 +112,12 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
112112
return fmt.Errorf("failed to assemble head changes for batch %s: %w", batch.ID, err)
113113
}
114114

115-
// Trigger the build with the configured build manager. metadata is nil
116-
// until a caller-supplied source materializes (e.g. requester / ticket
117-
// pulled off the originating LandRequest).
118-
buildID, err := c.buildRunner.Trigger(ctx, batch.Queue, base, head, nil)
115+
// Trigger the build with the configured build runner. The runner is
116+
// bound to this queue's job configuration at construction, so Trigger
117+
// takes no queue selector. metadata is nil until a caller-supplied
118+
// source materializes (e.g. requester / ticket pulled off the
119+
// originating LandRequest).
120+
buildID, err := c.buildRunner.Trigger(ctx, base, head, nil)
119121
if err != nil {
120122
metrics.NamedCounter(c.metricsScope, opName, "trigger_errors", 1)
121123
return fmt.Errorf("failed to trigger build for batch %s: %w", batch.ID, err)

orchestrator/controller/build/build_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func TestController_Process_TriggersWithBaseAndHead(t *testing.T) {
176176
br := buildrunnermock.NewMockBuildRunner(ctrl)
177177
wantBase := []entity.Change{depReq.Change}
178178
wantHead := []entity.Change{head1.Change, head2.Change}
179-
br.EXPECT().Trigger(gomock.Any(), headBatch.Queue, wantBase, wantHead, gomock.Nil()).Return(entity.BuildID{ID: "build-xyz"}, nil)
179+
br.EXPECT().Trigger(gomock.Any(), wantBase, wantHead, gomock.Nil()).Return(entity.BuildID{ID: "build-xyz"}, nil)
180180

181181
var publishedTopic string
182182
var published entity.BuildID
@@ -239,7 +239,7 @@ func TestController_Process_BuildStoreAlreadyExistsIsSwallowed(t *testing.T) {
239239
store.EXPECT().GetBuildStore().Return(mockBuildStore).AnyTimes()
240240

241241
br := buildrunnermock.NewMockBuildRunner(ctrl)
242-
br.EXPECT().Trigger(gomock.Any(), batch.Queue, gomock.Any(), gomock.Any(), gomock.Any()).Return(entity.BuildID{ID: "build-dup"}, nil)
242+
br.EXPECT().Trigger(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(entity.BuildID{ID: "build-dup"}, nil)
243243

244244
publishCalled := false
245245
mockPub := queuemock.NewMockPublisher(ctrl)
@@ -280,7 +280,7 @@ func TestController_Process_TriggerFailure(t *testing.T) {
280280
// No build store expectation: Trigger failure must short-circuit before Create.
281281

282282
br := buildrunnermock.NewMockBuildRunner(ctrl)
283-
br.EXPECT().Trigger(gomock.Any(), batch.Queue, gomock.Any(), gomock.Any(), gomock.Any()).
283+
br.EXPECT().Trigger(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
284284
Return(entity.BuildID{}, fmt.Errorf("provider down"))
285285

286286
registry, err := consumer.NewTopicRegistry(

0 commit comments

Comments
 (0)