Skip to content
Open
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
18 changes: 9 additions & 9 deletions stovepipe/controller/issuetrack/delivery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ func TestFinalizationOpensKeepsAndClosesIssues(t *testing.T) {
require.Equal(t, "build", f.states[issueKey{"c", "unit"}].FirstSeenBuildID)
require.Equal(t, int32(8), f.states[issueKey{"closed", ""}].Version)
require.Len(t, f.logs, 2)
require.Contains(t, f.logs, logKey{issueKey{"a", ""}, "build", entity.IssueStatusClosed})
require.Contains(t, f.logs, logKey{issueKey{"c", "unit"}, "build", entity.IssueStatusOpen})
require.Contains(t, f.logs, logKey{issueKey{"a", ""}, "build", entity.IssueEventClosed})
require.Contains(t, f.logs, logKey{issueKey{"c", "unit"}, "build", entity.IssueEventOpened})
}

func TestEmptyFinalBuildClosesIssuesButRetainsTheirRows(t *testing.T) {
Expand Down Expand Up @@ -266,7 +266,7 @@ func TestRetryRepairsHistoryBeforeFinalizing(t *testing.T) {
require.Equal(t, "task", f.progress.CursorTaskID)
require.Equal(t, "unit", f.progress.CursorUnitID)
require.Equal(t, int32(1), f.states[issueKey{"task", "unit"}].Version)
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", "unit"}, "build", entity.IssueStatusOpen}].TimestampMs)
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", "unit"}, "build", entity.IssueEventOpened}].TimestampMs)
}

func TestRetryFinishesTheCursorIssueBeforeScanningTheNextBatch(t *testing.T) {
Expand Down Expand Up @@ -428,7 +428,7 @@ func TestHistoryRecordsTheActualChangingBuildNotTheLatestProcessedBuild(t *testi
require.Empty(t, f.logs)
f.controller.nowMs = func() int64 { return 2000 }
require.NoError(t, f.deliver(msg, false))
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusClosed}].TimestampMs)
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", "unit"}, "build/100", entity.IssueEventClosed}].TimestampMs)
require.Equal(t, changed, f.states[issueKey{"task", "unit"}])

f.source("build/101", "101", entity.BuildStatusSucceeded)
Expand Down Expand Up @@ -466,7 +466,7 @@ func TestTaskWideFailurePreservesAllExistingIssues(t *testing.T) {
require.Equal(t, entity.IssueStatusClosed, f.states[issueKey{"old", "closed"}].Status)
require.Len(t, f.logs, 3)
for key := range f.logs {
require.Equal(t, entity.IssueStatusOpen, key.status)
require.Equal(t, entity.IssueEventOpened, key.event)
}
require.Equal(t, 1, f.failures["issue/list"])

Expand Down Expand Up @@ -506,7 +506,7 @@ func TestTaskWideFailureInLaterOpeningBatchPreventsAllClosures(t *testing.T) {
require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{"old", "unit"}].Status)
require.Len(t, f.logs, 106)
for key := range f.logs {
require.Equal(t, entity.IssueStatusOpen, key.status)
require.Equal(t, entity.IssueEventOpened, key.event)
}
}

Expand Down Expand Up @@ -610,7 +610,7 @@ func TestTaskWideHistoryRetryFinishesWithoutClosing(t *testing.T) {
require.NoError(t, f.deliver(msg, false))
require.Equal(t, entity.IssueTrackingPhaseFinalized, f.progress.Phase)
require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{"old", "unit"}].Status)
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", ""}, "build", entity.IssueStatusOpen}].TimestampMs)
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", ""}, "build", entity.IssueEventOpened}].TimestampMs)
require.Equal(t, 1, f.failures["issue/list"])
}

Expand Down Expand Up @@ -689,15 +689,15 @@ func TestRequestJobWaitsForAnalysisThenPreservesReopening(t *testing.T) {
require.NoError(t, f.deliverRequest(older, 0))
require.NoError(t, f.deliverRequest(older, -1))
require.Equal(t, entity.IssueStatusClosed, f.states[issueKey{"task", "unit"}].Status)
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusClosed})
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueEventClosed})
wait := _analysisWaitMs
if newerTerminal {
wait = 0
}
require.NoError(t, f.deliverRequest(newer, wait))
require.Equal(t, "build/101", f.states[issueKey{"task", "unit"}].FirstSeenBuildID)
require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{"task", "unit"}].Status)
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/101", entity.IssueStatusOpen})
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/101", entity.IssueEventOpened})
require.NoError(t, f.deliverRequest(older, -1))
require.Equal(t, "101", f.progress.RequestID)
})
Expand Down
8 changes: 4 additions & 4 deletions stovepipe/controller/issuetrack/dlq_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func TestRecoveryKeepsAcceptedIssueChanges(t *testing.T) {
for key, state := range f.states {
untouched[key] = state
}
delete(f.initialLogs, logKey{issueKey{"task", "unit"}, original.LastChangedBuildID, status})
delete(f.initialLogs, logKey{issueKey{"task", "unit"}, original.LastChangedBuildID, entity.IssueEvent(status)})
f.failures["issue/list"], f.failures["build-issues/list"], f.failures["build-issues/get"] = 1, 1, 1

require.NoError(t, f.recover(msg, false))
Expand All @@ -106,7 +106,7 @@ func TestRecoveryKeepsAcceptedIssueChanges(t *testing.T) {
}
require.Equal(t, expected, f.states[key])
}
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, original.LastChangedBuildID, status})
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, original.LastChangedBuildID, entity.IssueEvent(status)})
require.Len(t, f.events, 1)
require.Equal(t, 1, f.failures["issue/list"])
require.Equal(t, 1, f.failures["build-issues/list"])
Expand Down Expand Up @@ -193,7 +193,7 @@ func TestRecoveryDoesNotReopenAnExistingClosedCursor(t *testing.T) {
require.NoError(t, f.recover(msg, false))
require.Equal(t, entity.IssueStatusClosed, f.states[issueKey{"task", "unit"}].Status)
require.Equal(t, "older", f.states[issueKey{"task", "unit"}].FirstSeenBuildID)
require.NotContains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusOpen})
require.NotContains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueEventOpened})
}

func TestRecoveryRetriesOnlyRecoveryWork(t *testing.T) {
Expand Down Expand Up @@ -241,7 +241,7 @@ func TestRecoveryRetainsAnOpeningAcrossRetries(t *testing.T) {

require.NoError(t, f.recover(msg, false))
issue := f.states[issueKey{"task", "unit"}]
log := f.logs[logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusOpen}]
log := f.logs[logKey{issueKey{"task", "unit"}, "build/100", entity.IssueEventOpened}]
summary := f.summaries[issueKey{"task", "unit"}]
require.Equal(t, issue.FirstSeenBuildID, f.openIssues[issueKey{"task", "unit"}].FirstSeenBuildID)
require.Equal(t, entity.IssueStatusOpen, summary.Status)
Expand Down
2 changes: 1 addition & 1 deletion stovepipe/controller/issuetrack/fix_handoff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestOpeningPassPublishesRepairAfterIssueWrites(t *testing.T) {
require.Equal(t, entity.IssueTrackingPhaseRefreshing, f.progress.Phase)
for _, task := range []string{"new", "reopened"} {
require.Equal(t, entity.IssueStatusOpen, f.states[issueKey{task, "unit"}].Status)
require.Contains(t, f.logs, logKey{issueKey{task, "unit"}, "build", entity.IssueStatusOpen})
require.Contains(t, f.logs, logKey{issueKey{task, "unit"}, "build", entity.IssueEventOpened})
}
payload := &stovepipemq.FixRequest{}
require.NoError(t, stovepipemq.Unmarshal(msg.Payload, payload))
Expand Down
12 changes: 6 additions & 6 deletions stovepipe/controller/issuetrack/fixture_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ const testQueue = "validation/main"

type logKey struct {
issueKey
build string
status entity.IssueStatus
build string
event entity.IssueEvent
}

type fixture struct {
Expand Down Expand Up @@ -130,7 +130,7 @@ func newFixture(t *testing.T) *fixture {
if err := f.fail("history/create"); err != nil {
return err
}
key := logKey{issueKey{log.TaskID, log.UnitID}, log.BuildID, log.Status}
key := logKey{issueKey{log.TaskID, log.UnitID}, log.BuildID, log.Event}
if _, ok := f.initialLogs[key]; ok {
return storage.ErrAlreadyExists
}
Expand All @@ -140,11 +140,11 @@ func newFixture(t *testing.T) *fixture {
f.logs[key] = log
return nil
}).AnyTimes()
history.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, task, unit, build string, status entity.IssueStatus) (entity.IssueLog, error) {
history.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, task, unit, build string, event entity.IssueEvent) (entity.IssueLog, error) {
if err := f.fail("history/get"); err != nil {
return entity.IssueLog{}, err
}
key := logKey{issueKey{task, unit}, build, status}
key := logKey{issueKey{task, unit}, build, event}
if log, ok := f.logs[key]; ok {
return log, nil
}
Expand Down Expand Up @@ -433,7 +433,7 @@ func (f *fixture) reportIssue(build, task, unit string) {

func (f *fixture) seedIssue(task, unit string, status entity.IssueStatus) {
f.states[issueKey{task, unit}] = entity.IssueState{TaskID: task, UnitID: unit, Status: status, FirstSeenBuildID: "older", Version: 7, LastProcessedBuildID: "older", LastChangedBuildID: "older", ChangedAtMs: 500}
f.initialLogs[logKey{issueKey{task, unit}, "older", status}] = entity.IssueLog{TaskID: task, UnitID: unit, BuildID: "older", Status: status, TimestampMs: 500}
f.initialLogs[logKey{issueKey{task, unit}, "older", entity.IssueEvent(status)}] = entity.IssueLog{TaskID: task, UnitID: unit, BuildID: "older", Event: entity.IssueEvent(status), TimestampMs: 500}
f.summaries[issueKey{task, unit}] = entity.IssueSummary{TaskID: task, UnitID: unit, Status: status,
FirstSeenBuildID: "older", RequestID: "1", BuildID: "older", ChangedAtMs: 500, Version: 1}
if status == entity.IssueStatusOpen {
Expand Down
2 changes: 1 addition & 1 deletion stovepipe/controller/issuetrack/history.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func persistIssuesReconciledLog(ctx context.Context, materializer requestlog.Mat
}

func persistIssueHistory(ctx context.Context, materializer issuelog.Materializer, store storage.Storage, issue entity.IssueState) error {
log := entity.IssueLog{TaskID: issue.TaskID, UnitID: issue.UnitID, BuildID: issue.LastChangedBuildID, Status: issue.Status, TimestampMs: issue.ChangedAtMs}
log := entity.IssueLog{TaskID: issue.TaskID, UnitID: issue.UnitID, BuildID: issue.LastChangedBuildID, Event: entity.IssueEvent(issue.Status), TimestampMs: issue.ChangedAtMs}
if err := materializer.PersistLog(ctx, store, log, issue.FirstSeenBuildID); err != nil {
return fmt.Errorf("record issue transition: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions stovepipe/controller/issuetrack/issuetrack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,13 @@ func TestProcessIssueOpensAndCloses(t *testing.T) {
require.Equal(t, entity.IssueStatusOpen, issue.Status)
require.Equal(t, "build/100", issue.FirstSeenBuildID)
require.Equal(t, int32(1), issue.Version)
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusOpen})
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueEventOpened})
} else {
require.NotContains(t, f.openIssues, issueKey{"task", "unit"})
require.Equal(t, entity.IssueStatusClosed, issue.Status)
require.Equal(t, "older", issue.FirstSeenBuildID)
require.Equal(t, int32(8), issue.Version)
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusClosed})
require.Contains(t, f.logs, logKey{issueKey{"task", "unit"}, "build/100", entity.IssueEventClosed})
}
})
}
Expand Down Expand Up @@ -146,7 +146,7 @@ func TestHistoryRetryUsesTheStoredTransition(t *testing.T) {
_, err = f.controller.processIssue(context.Background(), f.store, f.progress, issueKey{"task", "unit"}, false)
require.NoError(t, err)
require.Equal(t, applied, f.states[issueKey{"task", "unit"}])
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", "unit"}, "build/100", entity.IssueStatusOpen}].TimestampMs)
require.Equal(t, int64(1000), f.logs[logKey{issueKey{"task", "unit"}, "build/100", entity.IssueEventOpened}].TimestampMs)
}

func TestCursorFailureDoesNotMutateTheIssue(t *testing.T) {
Expand Down
32 changes: 26 additions & 6 deletions stovepipe/core/issuelog/materializer.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"errors"
"fmt"
"math"
"time"

"github.com/uber-go/tally"
"github.com/uber/submitqueue/platform/metrics"
Expand All @@ -35,11 +36,14 @@ type Materializer interface {
PersistLog(ctx context.Context, stores storage.Storage, log entity.IssueLog, firstSeenBuildID string) error
}

type materializer struct{ scope tally.Scope }
type materializer struct {
scope tally.Scope
now func() time.Time
}

// NewMaterializer creates an issue-log materializer.
func NewMaterializer(scope tally.Scope) Materializer {
return &materializer{scope: scope.SubScope("issue_log_materializer")}
return &materializer{scope: scope.SubScope("issue_log_materializer"), now: time.Now}
}

func (m *materializer) PersistLog(ctx context.Context, stores storage.Storage, log entity.IssueLog, firstSeenBuildID string) (retErr error) {
Expand All @@ -49,10 +53,17 @@ func (m *materializer) PersistLog(ctx context.Context, stores storage.Storage, l
if err := ctx.Err(); err != nil {
return err
}
if log.FirstSeenBuildID != "" && log.FirstSeenBuildID != firstSeenBuildID {
return fmt.Errorf("issue event opening disagrees with source state")
}
log.FirstSeenBuildID = firstSeenBuildID
if log.Event == entity.IssueEventReported && log.TimestampMs == 0 {
log.TimestampMs = m.now().UnixMilli()
}
if err := log.Validate(); err != nil {
return fmt.Errorf("invalid issue transition: %w", err)
}
if firstSeenBuildID == "" || (log.Status == entity.IssueStatusOpen && firstSeenBuildID != log.BuildID) {
if firstSeenBuildID == "" || (log.Event == entity.IssueEventOpened && firstSeenBuildID != log.BuildID) {
return fmt.Errorf("issue transition has an invalid opening build")
}

Expand All @@ -68,8 +79,14 @@ func (m *materializer) PersistLog(ctx context.Context, stores storage.Storage, l
return fmt.Errorf("issue transition build identity does not match retained history")
}

if retained.Event == entity.IssueEventReported {
return nil
}
if retained.FirstSeenBuildID != "" {
firstSeenBuildID = retained.FirstSeenBuildID
}
summary := entity.IssueSummary{
TaskID: retained.TaskID, UnitID: retained.UnitID, Status: retained.Status,
TaskID: retained.TaskID, UnitID: retained.UnitID, Status: entity.IssueStatus(retained.Event),
FirstSeenBuildID: firstSeenBuildID, RequestID: build.RequestID, BuildID: retained.BuildID,
ChangedAtMs: retained.TimestampMs, Version: 1,
}
Expand All @@ -87,14 +104,17 @@ func retainIssueLog(ctx context.Context, logs storage.IssueLogStore, log entity.
if !errors.Is(err, storage.ErrAlreadyExists) {
return entity.IssueLog{}, fmt.Errorf("retain issue transition: %w", err)
}
retained, err := logs.Get(ctx, log.TaskID, log.UnitID, log.BuildID, log.Status)
retained, err := logs.Get(ctx, log.TaskID, log.UnitID, log.BuildID, log.Event)
if err != nil {
return entity.IssueLog{}, fmt.Errorf("read retained issue transition: %w", err)
}
if retained.TaskID != log.TaskID || retained.UnitID != log.UnitID ||
retained.BuildID != log.BuildID || retained.Status != log.Status {
retained.BuildID != log.BuildID || retained.Event != log.Event {
return entity.IssueLog{}, fmt.Errorf("issue transition identity conflicts with retained history")
}
if retained.FirstSeenBuildID != "" && retained.FirstSeenBuildID != log.FirstSeenBuildID {
return entity.IssueLog{}, fmt.Errorf("issue event opening conflicts with retained history")
}
if err := retained.Validate(); err != nil {
return entity.IssueLog{}, fmt.Errorf("invalid retained issue transition: %w", err)
}
Expand Down
44 changes: 37 additions & 7 deletions stovepipe/core/issuelog/materializer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import (

type logKey struct {
task, unit, build string
status entity.IssueStatus
event entity.IssueEvent
}

type materializerFixture struct {
Expand Down Expand Up @@ -63,18 +63,18 @@ func newMaterializerFixture(t *testing.T) *materializerFixture {
if err := f.failures["log/create"]; err != nil {
return err
}
key := logKey{log.TaskID, log.UnitID, log.BuildID, log.Status}
key := logKey{log.TaskID, log.UnitID, log.BuildID, log.Event}
if _, ok := f.logs[key]; ok {
return storage.ErrAlreadyExists
}
f.logs[key] = log
return nil
}).AnyTimes()
logs.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, task, unit, build string, status entity.IssueStatus) (entity.IssueLog, error) {
logs.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, task, unit, build string, event entity.IssueEvent) (entity.IssueLog, error) {
if err := f.failures["log/get"]; err != nil {
return entity.IssueLog{}, err
}
log, ok := f.logs[logKey{task, unit, build, status}]
log, ok := f.logs[logKey{task, unit, build, event}]
if !ok {
return entity.IssueLog{}, storage.ErrNotFound
}
Expand Down Expand Up @@ -111,7 +111,7 @@ func newMaterializerFixture(t *testing.T) *materializerFixture {
if f.summary.Version != 0 {
return storage.ErrAlreadyExists
}
require.Equal(t, summary.ChangedAtMs, f.logs[logKey{summary.TaskID, summary.UnitID, summary.BuildID, summary.Status}].TimestampMs)
require.Equal(t, summary.ChangedAtMs, f.logs[logKey{summary.TaskID, summary.UnitID, summary.BuildID, entity.IssueEvent(summary.Status)}].TimestampMs)
f.summary = summary
return nil
}).AnyTimes()
Expand Down Expand Up @@ -191,7 +191,7 @@ func newMaterializerFixture(t *testing.T) *materializerFixture {
}

func testIssueLog(build string, status entity.IssueStatus) entity.IssueLog {
return entity.IssueLog{TaskID: "validation", UnitID: "unit", BuildID: build, Status: status, TimestampMs: 1000}
return entity.IssueLog{TaskID: "validation", UnitID: "unit", BuildID: build, Event: entity.IssueEvent(status), TimestampMs: 1000}
}

func TestMaterializerOpenCloseReopenAndReplay(t *testing.T) {
Expand Down Expand Up @@ -319,7 +319,7 @@ func TestMaterializerRejectsInvalidTransitions(t *testing.T) {
opening := log.BuildID
switch scenario {
case "invalid status":
log.Status = entity.IssueStatusUnknown
log.Event = ""
case "missing opening":
opening = ""
case "wrong opening":
Expand Down Expand Up @@ -517,3 +517,33 @@ func TestOpenIssueRetryHonorsCanceledContext(t *testing.T) {

require.ErrorIs(t, f.m.PersistLog(ctx, f.stores, log, log.BuildID), context.Canceled)
}

func TestReportedIssueDoesNotChangeLifecycle(t *testing.T) {
f := newMaterializerFixture(t)
opened := testIssueLog("build/z", entity.IssueStatusOpen)
require.NoError(t, f.m.PersistLog(context.Background(), f.stores, opened, opened.BuildID))
before, opening := f.summary, f.openIssue
report := entity.IssueLog{TaskID: "validation", UnitID: "unit", BuildID: "build/a", Event: entity.IssueEventReported}
require.NoError(t, f.m.PersistLog(context.Background(), f.stores, report, opened.BuildID))
require.Equal(t, before, f.summary)
require.Equal(t, opening, f.openIssue)
retained := f.logs[logKey{"validation", "unit", "build/a", entity.IssueEventReported}]
require.Equal(t, opened.BuildID, retained.FirstSeenBuildID)
require.Positive(t, retained.TimestampMs)
report.TimestampMs = retained.TimestampMs + 100
require.NoError(t, f.m.PersistLog(context.Background(), f.stores, report, opened.BuildID))
require.Equal(t, retained, f.logs[logKey{"validation", "unit", "build/a", entity.IssueEventReported}])
}

func TestReportedIssueCannotReopenClosedIssue(t *testing.T) {
f := newMaterializerFixture(t)
opened := testIssueLog("build/z", entity.IssueStatusOpen)
closed := testIssueLog("build/m", entity.IssueStatusClosed)
require.NoError(t, f.m.PersistLog(context.Background(), f.stores, opened, opened.BuildID))
require.NoError(t, f.m.PersistLog(context.Background(), f.stores, closed, opened.BuildID))
before := f.summary
report := entity.IssueLog{TaskID: "validation", UnitID: "unit", BuildID: "build/a", Event: entity.IssueEventReported}
require.NoError(t, f.m.PersistLog(context.Background(), f.stores, report, opened.BuildID))
require.Equal(t, before, f.summary)
require.Empty(t, f.openIssue)
}
Loading
Loading