diff --git a/Makefile b/Makefile index 4fb97bb..d71dfd0 100644 --- a/Makefile +++ b/Makefile @@ -274,7 +274,7 @@ local-stovepipe-stop: ## Stop Stovepipe service mocks: ## Generate mock files using mockgen @echo "Generating mocks..." - @$(BAZEL) run @rules_go//go -- generate ./extension/storage/... ./extension/changestore/... ./extension/counter/... ./extension/queue/... ./extension/queueconfig/... ./extension/mergechecker/... ./extension/pusher/... ./extension/scorer/... ./extension/conflict/... ./core/consumer/... + @$(BAZEL) run @rules_go//go -- generate ./extension/storage/... ./extension/build/... ./extension/changestore/... ./extension/counter/... ./extension/queue/... ./extension/queueconfig/... ./extension/mergechecker/... ./extension/pusher/... ./extension/scorer/... ./extension/conflict/... ./core/consumer/... @echo "Mocks generated successfully!" proto: ## Generate protobuf files from .proto definitions diff --git a/entity/build.go b/entity/build.go index f5956d1..7fe80cb 100644 --- a/entity/build.go +++ b/entity/build.go @@ -16,41 +16,43 @@ package entity import "encoding/json" -// BuildStatus defines the possible states of a build. +// BuildStatus defines the possible states of a build. The set is +// intentionally narrow: every supported build provider must be able to map +// its native lifecycle into one of these values without leaking +// provider-specific stages. type BuildStatus string const ( - // BuildStatusUnknown is the unreachable state. It is set by default when the structure is initialized. It should never be seen in the system. + // BuildStatusUnknown is the unreachable zero value, set by default when + // the structure is initialized. It should never be seen in the system. BuildStatusUnknown BuildStatus = "" - // BuildStatusQueued indicates the build has been scheduled but not yet started. - BuildStatusQueued BuildStatus = "queued" + // BuildStatusAccepted indicates the build has been accepted (or queued) + // by the provider but has not started executing yet. + BuildStatusAccepted BuildStatus = "accepted" // BuildStatusRunning indicates the build is currently executing. BuildStatusRunning BuildStatus = "running" - // BuildStatusPassed indicates the build completed successfully. + // BuildStatusSucceeded indicates the build completed successfully. // This is a terminal state. - BuildStatusPassed BuildStatus = "passed" + BuildStatusSucceeded BuildStatus = "succeeded" - // BuildStatusFailed indicates the build completed with failures. - // This is a terminal state. + // BuildStatusFailed indicates the build did not complete successfully. + // This is a terminal state. Provider-initiated cancellations (timeout, + // resource limits, etc.) are reported as Failed, not Cancelled; + // Cancelled is reserved for cancellations the caller initiated itself. BuildStatusFailed BuildStatus = "failed" - // BuildStatusCancelled indicates the build was cancelled before completion. + // BuildStatusCancelled indicates the build was cancelled by the caller. // This is a terminal state. BuildStatusCancelled BuildStatus = "cancelled" - - // BuildStatusBlocked indicates the build is waiting for manual approval or unblocking. - // Some CI systems (like BuildKite) support manual approval steps. - BuildStatusBlocked BuildStatus = "blocked" ) -// IsTerminal returns true if the build state represents a final state (passed, failed, or cancelled). -// Terminal states indicate the build has finished and will not change state again. -// Note: BuildStatusBlocked is NOT terminal as blocked builds can be unblocked and continue execution. +// IsTerminal returns true if the status represents a final state +// (Succeeded, Failed, or Cancelled). func (s BuildStatus) IsTerminal() bool { - return s == BuildStatusPassed || s == BuildStatusFailed || s == BuildStatusCancelled + return s == BuildStatusSucceeded || s == BuildStatusFailed || s == BuildStatusCancelled } // SpeculationPathInfo represents the base and head commits of a speculation path used in a build. @@ -88,3 +90,31 @@ func BuildFromBytes(data []byte) (Build, error) { err := json.Unmarshal(data, &build) return build, err } + +// ChangeAction defines the action to perform on a change submitted to the build provider. +type ChangeAction string + +const ( + // ChangeActionUnknown is the unreachable zero value, set by default when + // the structure is initialized. It should never be seen in the system. + ChangeActionUnknown ChangeAction = "" + // ChangeActionApply applies the change to the target branch. + ChangeActionApply ChangeAction = "apply" + // ChangeActionValidate applies the change and then validates it by + // running the associated test / validation suites. + ChangeActionValidate ChangeAction = "validate" +) + +// BuildChange pairs a Change with the action the build provider should +// perform on it. Used as input to BuildManager.Trigger. +type BuildChange struct { + // Change identifies the code change to process. + Change Change + // Action specifies what the build provider should do with the change. + Action ChangeAction +} + +// BuildMetadata carries provider-defined free-form metadata about a build +// (e.g. build URL, duration, commit SHA). Keys and values are +// implementation-defined; callers should not assume any particular schema. +type BuildMetadata map[string]string diff --git a/entity/build_test.go b/entity/build_test.go index e31938a..97de019 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -28,8 +28,8 @@ func TestBuildStatus_IsTerminal(t *testing.T) { expected bool }{ { - name: "passed is terminal", - status: BuildStatusPassed, + name: "succeeded is terminal", + status: BuildStatusSucceeded, expected: true, }, { @@ -43,8 +43,8 @@ func TestBuildStatus_IsTerminal(t *testing.T) { expected: true, }, { - name: "queued is not terminal", - status: BuildStatusQueued, + name: "accepted is not terminal", + status: BuildStatusAccepted, expected: false, }, { @@ -52,11 +52,6 @@ func TestBuildStatus_IsTerminal(t *testing.T) { status: BuildStatusRunning, expected: false, }, - { - name: "blocked is not terminal", - status: BuildStatusBlocked, - expected: false, - }, { name: "unknown is not terminal", status: BuildStatusUnknown, @@ -79,7 +74,7 @@ func TestBuild_ToBytes(t *testing.T) { Base: []string{"batch-0", "batch-prev"}, }, Score: 0.85, - Status: BuildStatusQueued, + Status: BuildStatusAccepted, } data, err := build.ToBytes() @@ -90,7 +85,7 @@ func TestBuild_ToBytes(t *testing.T) { jsonStr := string(data) assert.Contains(t, jsonStr, "build-1") assert.Contains(t, jsonStr, "batch-1") - assert.Contains(t, jsonStr, "queued") + assert.Contains(t, jsonStr, "accepted") } func TestBuildFromBytes(t *testing.T) { @@ -101,7 +96,7 @@ func TestBuildFromBytes(t *testing.T) { Base: []string{"batch-5", "batch-6"}, }, Score: 0.92, - Status: BuildStatusRunning, + Status: BuildStatusAccepted, } // Serialize @@ -146,7 +141,7 @@ func TestBuild_SerializationRoundTrip(t *testing.T) { build Build }{ { - name: "queued build with speculation path", + name: "accepted build with speculation path", build: Build{ ID: "build-100", BatchID: "batch-50", @@ -154,16 +149,16 @@ func TestBuild_SerializationRoundTrip(t *testing.T) { Base: []string{"batch-48", "batch-49"}, }, Score: 0.75, - Status: BuildStatusQueued, + Status: BuildStatusAccepted, }, }, { - name: "passed build with no speculation base", + name: "succeeded build with no speculation base", build: Build{ ID: "build-200", BatchID: "batch-60", Score: 1.0, - Status: BuildStatusPassed, + Status: BuildStatusSucceeded, }, }, { diff --git a/extension/build/BUILD.bazel b/extension/build/BUILD.bazel new file mode 100644 index 0000000..61a273e --- /dev/null +++ b/extension/build/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "build", + srcs = ["build_manager.go"], + importpath = "github.com/uber/submitqueue/extension/build", + visibility = ["//visibility:public"], + deps = ["//entity"], +) diff --git a/extension/build/README.md b/extension/build/README.md new file mode 100644 index 0000000..a7f5c9e --- /dev/null +++ b/extension/build/README.md @@ -0,0 +1,58 @@ +# Build Manager + +Pluggable abstraction for triggering builds against an external provider +(BuildKite, Jenkins, an internal job runner, etc.), querying their status, +and cancelling them. + +## Interface + +`BuildManager` exposes `Trigger`, `Status`, `Cancel`, and `Close`. +Implementations are long-lived singletons bound to their provider +configuration at construction; the interface itself stays vendor-agnostic. + +### Lifecycle and state + +Implementations are long-lived singletons. They must make every method safe +for concurrent use and should not serialize calls beyond what the provider's +rate limits require. They may hold transient local state (connection pools, +caches), but anything that must survive a restart belongs in `Storage`. + +### Async vs sync contract + +- `Trigger` returns promptly; provider-side work is asynchronous. MAY return + a terminal status when the input maps to an already-finished build; + otherwise returns `BuildStatusAccepted`. +- `Status` MAY be synchronous and lengthy — a provider round trip is typical. +- `Cancel` returns once the request reaches the provider; it does not wait + for the build to stop. No-op on already-terminal builds. +- `Close` is idempotent; subsequent calls on other methods return errors. + +### Transient errors + +Implementations recover from transient connectivity failures (network blips, +provider 5xx) internally — reconnect, retry-with-backoff, etc. During the +recovery window methods return plain errors and never block the caller +indefinitely. + +### Errors + +Methods return plain errors. Per the `core/errs` convention, the calling +controller decides classification (user vs infra, retryable vs not); the +implementation should wrap with `errs.NewRetryableError` only when it has +specific knowledge that a failure is transient and safely retryable. Domain +sentinels (e.g. a "build not found" error) will be introduced alongside the +first implementation that needs them. + +## Adding a new backend + +1. Create `extension/build/{backend}/` with a `BuildManager` implementation + bound to its provider configuration at construction. +2. Map each `entity.BuildChange` (with its `ChangeAction`) onto the + backend's build primitives. +3. Map the provider's lifecycle states down to the `BuildStatus` values: + `Accepted` (accepted/queued, not started), `Running` (executing), and the + terminal `Succeeded` / `Failed` / `Cancelled`. Provider-initiated + cancellations (timeout, resource limits) map to `Failed`; only + caller-initiated cancellations map to `Cancelled`. +4. Implement internal reconnect / retry so transient failures surface as + plain errors without blocking the caller. diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go new file mode 100644 index 0000000..d186b22 --- /dev/null +++ b/extension/build/build_manager.go @@ -0,0 +1,70 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package build + +//go:generate mockgen -source=build_manager.go -destination=mock/build_manager_mock.go -package=mock + +import ( + "context" + + "github.com/uber/submitqueue/entity" +) + +// BuildManager triggers builds against an external provider, queries their +// status, and cancels them. +// +// Implementations are long-lived singletons and must: +// - make every method safe for concurrent use by multiple goroutines; +// - recover from transient connectivity failures internally, returning +// plain errors during the recovery window rather than blocking the +// caller indefinitely; +// - keep only transient local state (caches, pools) — anything that must +// survive a restart belongs in Storage; +// - return plain errors and leave classification (user vs infra, +// retryable or not) to the calling controller, per core/errs. +type BuildManager interface { + // Trigger submits changes (in order — order is significant) to the + // provider and returns the build ID and current status. It MUST return + // promptly; provider-side work happens asynchronously. Implementations + // MAY return a terminal status when the input maps to an already-finished + // build; otherwise return BuildStatusAccepted. + // + // queueName selects the provider-specific job configuration. + // Returns an error if the request is invalid. + Trigger( + ctx context.Context, + queueName string, + changes []entity.BuildChange, + ) (buildID string, status entity.BuildStatus, err error) + + // Status returns the current status and provider-defined metadata + // (build URL, duration, etc.) for a build. Unlike Trigger, Status MAY be + // synchronous and lengthy — a provider round trip is typical. + // + // Returns an error if the build does not exist. + Status( + ctx context.Context, + buildID string, + ) (entity.BuildStatus, entity.BuildMetadata, error) + + // Cancel requests cancellation and returns once the request has reached + // the provider; it does not wait for the build to actually stop. A no-op + // on already-terminal builds. Returns an error if the build does not exist. + Cancel(ctx context.Context, buildID string) error + + // Close releases resources held by the manager. Idempotent. After Close, + // every other method returns an error. + Close() error +} diff --git a/extension/build/mock/BUILD.bazel b/extension/build/mock/BUILD.bazel new file mode 100644 index 0000000..68b524b --- /dev/null +++ b/extension/build/mock/BUILD.bazel @@ -0,0 +1,12 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "mock", + srcs = ["build_manager_mock.go"], + importpath = "github.com/uber/submitqueue/extension/build/mock", + visibility = ["//visibility:public"], + deps = [ + "//entity", + "@org_uber_go_mock//gomock", + ], +) diff --git a/extension/build/mock/build_manager_mock.go b/extension/build/mock/build_manager_mock.go new file mode 100644 index 0000000..e2b27b7 --- /dev/null +++ b/extension/build/mock/build_manager_mock.go @@ -0,0 +1,102 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: build_manager.go +// +// Generated by this command: +// +// mockgen -source=build_manager.go -destination=mock/build_manager_mock.go -package=mock +// + +// Package mock is a generated GoMock package. +package mock + +import ( + context "context" + reflect "reflect" + + entity "github.com/uber/submitqueue/entity" + gomock "go.uber.org/mock/gomock" +) + +// MockBuildManager is a mock of BuildManager interface. +type MockBuildManager struct { + ctrl *gomock.Controller + recorder *MockBuildManagerMockRecorder + isgomock struct{} +} + +// MockBuildManagerMockRecorder is the mock recorder for MockBuildManager. +type MockBuildManagerMockRecorder struct { + mock *MockBuildManager +} + +// NewMockBuildManager creates a new mock instance. +func NewMockBuildManager(ctrl *gomock.Controller) *MockBuildManager { + mock := &MockBuildManager{ctrl: ctrl} + mock.recorder = &MockBuildManagerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockBuildManager) EXPECT() *MockBuildManagerMockRecorder { + return m.recorder +} + +// Cancel mocks base method. +func (m *MockBuildManager) Cancel(ctx context.Context, buildID string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Cancel", ctx, buildID) + ret0, _ := ret[0].(error) + return ret0 +} + +// Cancel indicates an expected call of Cancel. +func (mr *MockBuildManagerMockRecorder) Cancel(ctx, buildID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Cancel", reflect.TypeOf((*MockBuildManager)(nil).Cancel), ctx, buildID) +} + +// Close mocks base method. +func (m *MockBuildManager) Close() error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Close") + ret0, _ := ret[0].(error) + return ret0 +} + +// Close indicates an expected call of Close. +func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockBuildManager)(nil).Close)) +} + +// Status mocks base method. +func (m *MockBuildManager) Status(ctx context.Context, buildID string) (entity.BuildStatus, entity.BuildMetadata, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Status", ctx, buildID) + ret0, _ := ret[0].(entity.BuildStatus) + ret1, _ := ret[1].(entity.BuildMetadata) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// Status indicates an expected call of Status. +func (mr *MockBuildManagerMockRecorder) Status(ctx, buildID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Status", reflect.TypeOf((*MockBuildManager)(nil).Status), ctx, buildID) +} + +// Trigger mocks base method. +func (m *MockBuildManager) Trigger(ctx context.Context, queueName string, changes []entity.BuildChange) (string, entity.BuildStatus, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Trigger", ctx, queueName, changes) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(entity.BuildStatus) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// Trigger indicates an expected call of Trigger. +func (mr *MockBuildManagerMockRecorder) Trigger(ctx, queueName, changes any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Trigger", reflect.TypeOf((*MockBuildManager)(nil).Trigger), ctx, queueName, changes) +} diff --git a/orchestrator/controller/build/build.go b/orchestrator/controller/build/build.go index 48d4cf1..0a45087 100644 --- a/orchestrator/controller/build/build.go +++ b/orchestrator/controller/build/build.go @@ -102,7 +102,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r build := entity.Build{ ID: batch.ID, BatchID: batch.ID, - Status: entity.BuildStatusQueued, + Status: entity.BuildStatusAccepted, } // Publish build to build signal topic diff --git a/orchestrator/controller/buildsignal/buildsignal_test.go b/orchestrator/controller/buildsignal/buildsignal_test.go index 44db77a..7ed8cf8 100644 --- a/orchestrator/controller/buildsignal/buildsignal_test.go +++ b/orchestrator/controller/buildsignal/buildsignal_test.go @@ -88,7 +88,7 @@ func TestController_Process_Success(t *testing.T) { build := entity.Build{ ID: "build-123", BatchID: "test-queue/batch/1", - Status: entity.BuildStatusQueued, + Status: entity.BuildStatusAccepted, } payload, err := build.ToBytes() @@ -130,7 +130,7 @@ func TestController_Process_PublishFailure(t *testing.T) { build := entity.Build{ ID: "build-456", BatchID: "test-queue/batch/2", - Status: entity.BuildStatusRunning, + Status: entity.BuildStatusAccepted, } payload, err := build.ToBytes()