Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/rfc/stovepipe/steps/buildanalysis.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Build issue observation

Build analysis runs alongside status polling and recording. Hosts supply a queue-resolved detector factory. The reference service uses a no-op detector so the analysis stage is always wired without inspecting provider output; deployments replace the factory with their integration. No-op results do not establish build success or recovery.
Build analysis runs alongside status polling and recording. Hosts supply a queue-resolved detector factory. The reference service uses a no-op detector so the analysis stage is always wired without inspecting provider output; deployments replace the factory with their integration. No-op results do not establish build success. The reference host registers an issue-tracking publication route but does not yet consume those notifications.

## Start and completion notices

Expand All @@ -12,7 +12,7 @@ Publication errors are returned through the producing stage's retry/DLQ policy.

## Observations and polling

The [analysis controller](../../../../stovepipe/controller/buildanalysis/README.md) owns detection, per-build observation persistence, polling, and the stop condition for Requests that end without a recorded build result. It only accumulates historical observations; it does not maintain open issues or infer recovery from absence.
The [analysis controller](../../../../stovepipe/controller/buildanalysis/README.md) owns detection, per-build observation persistence, polling, and the stop condition for Requests that end without a recorded build result. It accumulates per-build observations and publishes queue-partitioned issue-tracking notifications after persistence; it does not maintain open issues itself. Successful terminal analysis requests final reconciliation only after the Request records that Build as its winning outcome. Final reconciliation uses the accumulated observations, including issues omitted from the last detector response. See the controller documentation for handoff and deduplication rules.

Status-polling DLQ recovery does not publish a build-completion notice. The analysis polling loop observes the ended Request through storage instead.

Expand Down
7 changes: 6 additions & 1 deletion service/stovepipe/server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ func registerPrimaryControllers(
}
count++

analysisController := buildanalysis.NewController(logger, scope, store, detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis")
analysisController := buildanalysis.NewController(logger, scope, store, detectorFactory, registry, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis")
if err := c.Register(analysisController); err != nil {
return count, fmt.Errorf("failed to register buildanalysis controller: %w", err)
}
Expand Down Expand Up @@ -561,6 +561,11 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe
Queue: q,
Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-hook-dlq"),
},
{
Key: stovepipemq.TopicKeyIssueTrack,
Name: "issuetrack",
Queue: q,
},
{
Key: stovepipemq.TopicKeyBuildAnalysis,
Name: "buildanalysis",
Expand Down
9 changes: 9 additions & 0 deletions service/stovepipe/server/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,15 @@ func TestEveryRegisteredControllerResolvesInTheTopicRegistry(t *testing.T) {
}
}

func TestIssueTrackingHasPublicationRouteWithoutConsumption(t *testing.T) {
registry, primary, deadLetter := registeredControllers(t)
name, ok := registry.TopicName(stovepipemq.TopicKeyIssueTrack)
require.True(t, ok)
assert.Equal(t, "issuetrack", name)
assert.NotContains(t, topicKeys(primary), stovepipemq.TopicKeyIssueTrack)
assert.NotContains(t, topicKeys(deadLetter), dlq.TopicKey(stovepipemq.TopicKeyIssueTrack))
}

func TestHookStage(t *testing.T) {
registry, primary, deadLetter := registeredControllers(t)

Expand Down
31 changes: 26 additions & 5 deletions stovepipe/controller/build/build_analysis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,16 @@ func TestBuildAnalysisPipeline(t *testing.T) {

detectorFactory := buildissuemock.NewMockDetectorFactory(ctrl)
detector := buildissuemock.NewMockDetector(ctrl)
detectorFactory.EXPECT().For(buildissue.DetectorConfig{QueueName: testQueue}).Return(detector, nil).Times(3)
detectorFactory.EXPECT().For(buildissue.DetectorConfig{QueueName: testQueue}).Return(detector, nil).Times(4)
gomock.InOrder(
detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{a}, nil),
detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{a, b}, nil),
detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{a, b}, nil),
detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return([]entity.BuildIssue{b}, nil),
detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(nil, nil),
detector.EXPECT().Detect(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(nil, nil),
)

analysisController := buildanalysis.NewController(zap.NewNop().Sugar(), tally.NoopScope,
staticStorageFactory{store: m.store}, detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "analysis")
staticStorageFactory{store: m.store}, detectorFactory, buildController.registry, stovepipemq.TopicKeyBuildAnalysis, "analysis")
statusController := buildsignal.NewController(zap.NewNop().Sugar(), tally.NoopScope,
staticStorageFactory{store: m.store}, m.materializer, m.runnerFactory, buildController.registry,
stovepipemq.TopicKeyBuildSignal, "signal")
Expand All @@ -139,17 +140,37 @@ func TestBuildAnalysisPipeline(t *testing.T) {
assert.True(t, persisted[a])
assert.False(t, persisted[b])

secondPoll := delivered(published["buildanalysis"][0])
secondPoll.EXPECT().Hold(buildanalysis.PollDelayMs)
require.NoError(t, analysisController.Process(ctx, secondPoll))
assert.True(t, persisted[b])

require.NoError(t, statusController.Process(ctx, delivered(published["buildsignal"][0])))
require.Len(t, published["record"], 1)
require.Len(t, published["buildanalysis"], 2)
assert.Equal(t, entity.BuildStatusFailed, savedBuild.Status)
assert.Equal(t, entity.RequestStateFailed, request.State)
assert.Zero(t, queue.InFlightCount)
assert.False(t, persisted[b])
assert.True(t, persisted[b])
assert.NotEqual(t, published["buildanalysis"][0].ID, published["buildanalysis"][1].ID)

require.NoError(t, analysisController.Process(ctx, delivered(published["buildanalysis"][0])))
require.NoError(t, analysisController.Process(ctx, delivered(published["buildanalysis"][1])))
assert.Len(t, persisted, 2)
assert.True(t, persisted[b])
assert.True(t, persisted[a])

require.Len(t, published["issuetrack"], 4)
for i, msg := range published["issuetrack"] {
assert.Equal(t, testQueue, msg.PartitionKey)
assert.Equal(t, testQueue, msg.Tenant)
notification := &stovepipemq.IssueTrack{}
require.NoError(t, stovepipemq.Unmarshal(msg.Payload, notification))
assert.Equal(t, testBuildID, notification.GetBuildId())
assert.Equal(t, testQueue, notification.GetQueueName())
assert.Equal(t, i > 1, notification.GetFinalize())
}
assert.NotEqual(t, published["issuetrack"][0].ID, published["issuetrack"][1].ID)
assert.NotEqual(t, published["issuetrack"][1].ID, published["issuetrack"][2].ID)
assert.Equal(t, published["issuetrack"][2].ID, published["issuetrack"][3].ID)
}
1 change: 1 addition & 0 deletions stovepipe/controller/build/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, buildMoc
registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{
{Key: stovepipemq.TopicKeyBuildSignal, Name: "buildsignal", Queue: queue},
{Key: stovepipemq.TopicKeyBuildAnalysis, Name: "buildanalysis", Queue: queue},
{Key: stovepipemq.TopicKeyIssueTrack, Name: "issuetrack", Queue: queue},
{Key: stovepipemq.TopicKeyRecord, Name: "record", Queue: queue},
})
require.NoError(t, err)
Expand Down
3 changes: 3 additions & 0 deletions stovepipe/controller/buildanalysis/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ go_library(
"//platform/base/messagequeue:go_default_library",
"//platform/consumer:go_default_library",
"//platform/metrics:go_default_library",
"//platform/publish:go_default_library",
"//stovepipe/core/loader:go_default_library",
"//stovepipe/core/messagequeue:go_default_library",
"//stovepipe/core/requestlog:go_default_library",
Expand All @@ -36,7 +37,9 @@ go_test(
"//platform/consumer:go_default_library",
"//platform/consumer/mock:go_default_library",
"//platform/errs:go_default_library",
"//platform/extension/messagequeue/mock:go_default_library",
"//platform/metrics:go_default_library",
"//platform/publish:go_default_library",
"//stovepipe/core/messagequeue:go_default_library",
"//stovepipe/core/requestlog:go_default_library",
"//stovepipe/core/requestlog/mock:go_default_library",
Expand Down
16 changes: 12 additions & 4 deletions stovepipe/controller/buildanalysis/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,27 @@

The controller reloads the queue-scoped Build, invokes the queue's issue detector, and persists each returned observation individually. Observations already stored for the same build, task, and unit within a queue are not persisted again, so repeated polls and retries after partial writes are safe. Detector results accompanying an error are discarded.

This controller only accumulates historical observations. An empty detection result neither deletes earlier observations nor establishes recovery. Open-issue projection, lifecycle history, and resolution are outside its scope.
After persistence, the controller publishes an issue-tracking notification partitioned by queue. Issue tracking maintains open issues and lifecycle history; analysis never writes them directly. An empty detection result does not delete earlier observations.

## Issue-tracking handoff

Refresh notifications let issue tracking open observed issues without resolving absent ones. Final notifications let it also resolve open issues absent from the build's accumulated observations. A successful terminal detection must discover all remaining reportable issues; it need not repeat earlier observations. An empty terminal response still triggers finalization, but does not retract earlier observations from the same build. See the [detector contract](../../extension/buildissue/README.md).

All analysis deliveries for a build must share its build-ID partition and consumer group. The controller validates that partition key. Finalization requires a successful pass that began with a terminal Build and the Request's recorded winning build ID. If status becomes terminal before that outcome is recorded, analysis publishes a refresh and holds until the outcome is available. Known losing builds do not publish. A terminal Request without a recorded winning build permits a refresh but not absence-based resolution.

Every refresh gets a fresh notification ID because held polling deliveries do not identify individual passes. A fixed ID would suppress later refreshes after earlier notifications were consumed. Retries can therefore publish duplicate refreshes; issue tracking is idempotent. Final notifications use one deterministic ID per build, so duplicate terminal passes deduplicate. Publication errors return through the normal analysis retry/DLQ policy; no hold or successful completion occurs before publication succeeds.

## Polling and completion

Every delivery reloads the Build and its Request before detection. After a successful pass, a nonterminal Build holds the delivery for two minutes; a terminal Build completes the delivery after persistence. The status read before detection determines whether the pass holds: if the Build finishes during detection, another delivery is needed to analyze it after observing terminal status. More than one delivery may analyze the same terminal Build; observation persistence remains idempotent.
Every delivery reloads the Build and its Request before detection. After a successful pass, a nonterminal Build holds the delivery for two minutes; a terminal Build completes after persistence and publication once its Request outcome is known, or the Request has ended without one. A pass that begins with a nonterminal Build always holds, even if the Build finishes during detection, so another delivery analyzes it after observing terminal status. More than one delivery may analyze the same terminal Build; observation persistence remains idempotent.

If the Request is terminal but the loaded Build is nonterminal, analysis reloads the Build before deciding to stop. The Build may have finished between the initial Build read and the Request read; reloading avoids skipping its final analysis. If it is still nonterminal, analysis stops without detection or a hold. This can occur when the status-polling DLQ handler fails the Request without learning the actual build result, leaving the Build at its last observed status.

Status polling and updates remain the responsibility of buildsignal. Analysis never changes Request state or releases build slots.

## Errors

Detection, persistence, configuration, state-loading, and message-validation errors are returned to the consumer for its normal retry or DLQ policy, whether the Build is running or terminal. Analysis errors do not use the two-minute polling hold. The consumer classifies errors and applies its configured retry limits and backoff; a final analysis can still fail and dead-letter.
Detection, persistence, publication, configuration, state-loading, and message-validation errors are returned to the consumer for its normal retry or DLQ policy, whether the Build is running or terminal. Analysis errors do not use the two-minute polling hold. The consumer classifies errors and applies its configured retry limits and backoff; a final analysis can still fail and dead-letter.

## DLQ recovery

Expand All @@ -28,4 +36,4 @@ State-read and history-write errors are returned for retry. Hosts must run recov

## Host integration

The reference service supplies a no-op detector and publishes analysis notices at build start and completion. It registers the normal analysis controller with bounded retries and the recovery controller with the existing DLQ consumer. See [build issue observation](../../../doc/rfc/stovepipe/steps/buildanalysis.md) for the publication rules and retry configuration.
The reference service supplies a no-op detector and publishes analysis notices at build start and completion. It also registers a publish-only issue-tracking topic; issue-tracking consumption and recovery are not enabled yet. It registers the normal analysis controller with bounded retries and the recovery controller with the existing DLQ consumer. See [build issue observation](../../../doc/rfc/stovepipe/steps/buildanalysis.md) for the publication rules and retry configuration.
65 changes: 58 additions & 7 deletions stovepipe/controller/buildanalysis/buildanalysis.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
"github.com/uber/submitqueue/platform/consumer"
"github.com/uber/submitqueue/platform/metrics"
"github.com/uber/submitqueue/platform/publish"
"github.com/uber/submitqueue/stovepipe/core/loader"
stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue"
"github.com/uber/submitqueue/stovepipe/entity"
Expand All @@ -33,17 +34,18 @@ import (
"go.uber.org/zap"
)

// PollDelayMs is the delay in milliseconds after a successful nonterminal pass.
// PollDelayMs is the delay in milliseconds between successful polling passes.
// Two minutes limits provider traffic during long-running builds.
const PollDelayMs int64 = 120000

// Controller accumulates observations while builds run and after completion.
// Terminal analysis must finish persistence before its delivery completes.
// Terminal analysis must finish persistence and publication before its delivery completes.
type Controller struct {
logger *zap.SugaredLogger
metricsScope tally.Scope
stores storage.Factory
detectorFactory buildissue.DetectorFactory
registry consumer.TopicRegistry
topicKey consumer.TopicKey
consumerGroup string
}
Expand All @@ -53,6 +55,7 @@ func NewController(
scope tally.Scope,
stores storage.Factory,
detectorFactory buildissue.DetectorFactory,
registry consumer.TopicRegistry,
topicKey consumer.TopicKey,
consumerGroup string,
) *Controller {
Expand All @@ -61,13 +64,14 @@ func NewController(
metricsScope: scope.SubScope("buildanalysis_controller"),
stores: stores,
detectorFactory: detectorFactory,
registry: registry,
topicKey: topicKey,
consumerGroup: consumerGroup,
}
}

// Process holds after a successful nonterminal pass. Errors are returned for
// normal queue retry or DLQ handling; partial writes are safe to retry.
// Process persists before publication and holds while further analysis is needed.
// Errors use normal queue retry or DLQ handling; partial writes are safe to retry.
func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (retErr error) {
op := metrics.Begin(c.metricsScope, "process", metrics.LongLatencyBuckets, metrics.TagsFromContext(ctx)...)
defer func() { op.Complete(retErr) }()
Expand All @@ -85,6 +89,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
if analysis.GetId() == "" {
return fmt.Errorf("build analysis requires a build ID")
}
if msg.PartitionKey != analysis.GetId() {
return fmt.Errorf("build analysis must be partitioned by build %q", analysis.GetId())
}

store, err := c.stores.For(storage.Config{QueueName: analysis.GetQueueName()})
if err != nil {
Expand All @@ -104,6 +111,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
if request.Queue != analysis.GetQueueName() {
return fmt.Errorf("request queue %q does not match analysis queue %q", request.Queue, analysis.GetQueueName())
}
if request.TerminalBuildID != "" && request.TerminalBuildID != build.ID {
return nil
}

// The Build may finish between the initial Build read and the Request read.
// Recheck its status so stopping here does not skip final analysis.
Expand Down Expand Up @@ -137,9 +147,29 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
}
}

// A nonterminal status read before detection requires another pass,
// even if the build finishes while detection is running.
if !build.Status.IsTerminal() {
hold := !build.Status.IsTerminal()
finalize := false
if build.Status.IsTerminal() {
// Build status can become terminal before buildsignal records the winning build.
request, err = loader.ByID(ctx, build.RequestID, store.GetRequestStore().Get, "request")
if err != nil {
return err
}
if request.Queue != analysis.GetQueueName() {
return fmt.Errorf("request queue %q does not match analysis queue %q", request.Queue, analysis.GetQueueName())
}
if request.TerminalBuildID != "" && request.TerminalBuildID != build.ID {
return nil
}
finalize = request.TerminalBuildID == build.ID
hold = !finalize && !request.State.IsTerminal()
}

if err := c.publishIssueTracking(ctx, request.Queue, build.ID, finalize); err != nil {
return err
}

if hold {
delivery.Hold(PollDelayMs)
}

Expand All @@ -153,6 +183,27 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
return nil
}

func (c *Controller) publishIssueTracking(ctx context.Context, queue, buildID string, finalize bool) error {
payload, err := stovepipemq.Marshal(&stovepipemq.IssueTrack{
BuildId: buildID, QueueName: queue, Finalize: finalize,
})
if err != nil {
return fmt.Errorf("serialize issue tracking notification: %w", err)
}

// Polls reuse one delivery, so no stable occurrence ID distinguishes refreshes.
messageID := publish.UniqueID(buildID)
if finalize {
messageID = publish.IntentID(buildID, "finished")
}
if err := publish.Message(ctx, c.registry, stovepipemq.TopicKeyIssueTrack, publish.MessageParams{
Tenant: queue, ID: messageID, Payload: payload, PartitionKey: queue,
}); err != nil {
return fmt.Errorf("publish issue tracking notification: %w", err)
}
return nil
}

func (c *Controller) Name() string { return "buildanalysis" }

func (c *Controller) TopicKey() consumer.TopicKey { return c.topicKey }
Expand Down
Loading
Loading