Skip to content

Commit 1c84fac

Browse files
dannykoppingdannykopping
committed
chore: augment reconciler test to validate that broken publish does not affect behaviour
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
1 parent f6909ff commit 1c84fac

File tree

2 files changed

+109
-90
lines changed

2 files changed

+109
-90
lines changed

‎coderd/database/dbgen/dbgen.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -643,8 +643,8 @@ func ProvisionerJob(t testing.TB, db database.Store, ps pubsub.Pubsub, orig data
643643
})
644644
require.NoError(t, err, "insert job")
645645
if ps != nil {
646-
err = provisionerjobs.PostJob(ps, job)
647-
require.NoError(t, err, "post job to pubsub")
646+
// Advisory, but not essential since acquirer has a background poller to pick up missed jobs.
647+
_ = provisionerjobs.PostJob(ps, job)
648648
}
649649
if !orig.StartedAt.Time.IsZero() {
650650
job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{

‎enterprise/coderd/prebuilds/reconcile_test.go

Lines changed: 107 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"time"
1010

1111
"github.com/prometheus/client_golang/prometheus"
12+
"golang.org/x/xerrors"
1213

1314
"github.com/coder/coder/v2/coderd/database/dbtime"
1415
"github.com/coder/coder/v2/coderd/util/slice"
@@ -303,107 +304,125 @@ func TestPrebuildReconciliation(t *testing.T) {
303304
for _, prebuildLatestTransition := range tc.prebuildLatestTransitions {
304305
for _, prebuildJobStatus := range tc.prebuildJobStatuses {
305306
for _, templateDeleted := range tc.templateDeleted {
306-
t.Run(fmt.Sprintf("%s - %s - %s", tc.name, prebuildLatestTransition, prebuildJobStatus), func(t *testing.T) {
307-
t.Parallel()
308-
t.Cleanup(func() {
309-
if t.Failed() {
310-
t.Logf("failed to run test: %s", tc.name)
311-
t.Logf("templateVersionActive: %t", templateVersionActive)
312-
t.Logf("prebuildLatestTransition: %s", prebuildLatestTransition)
313-
t.Logf("prebuildJobStatus: %s", prebuildJobStatus)
307+
for _, useBrokenPubsub := range []bool{true, false} {
308+
t.Run(fmt.Sprintf("%s - %s - %s - pubsub_broken=%v", tc.name, prebuildLatestTransition, prebuildJobStatus, useBrokenPubsub), func(t *testing.T) {
309+
t.Parallel()
310+
t.Cleanup(func() {
311+
if t.Failed() {
312+
t.Logf("failed to run test: %s", tc.name)
313+
t.Logf("templateVersionActive: %t", templateVersionActive)
314+
t.Logf("prebuildLatestTransition: %s", prebuildLatestTransition)
315+
t.Logf("prebuildJobStatus: %s", prebuildJobStatus)
316+
}
317+
})
318+
clock := quartz.NewMock(t)
319+
ctx := testutil.Context(t, testutil.WaitShort)
320+
cfg := codersdk.PrebuildsConfig{}
321+
logger := slogtest.Make(
322+
t, &slogtest.Options{IgnoreErrors: true},
323+
).Leveled(slog.LevelDebug)
324+
db, pubSub := dbtestutil.NewDB(t)
325+
if useBrokenPubsub {
326+
pubSub = &brokenPublisher{Pubsub: pubSub}
314327
}
315-
})
316-
clock := quartz.NewMock(t)
317-
ctx := testutil.Context(t, testutil.WaitShort)
318-
cfg := codersdk.PrebuildsConfig{}
319-
logger := slogtest.Make(
320-
t, &slogtest.Options{IgnoreErrors: true},
321-
).Leveled(slog.LevelDebug)
322-
db, pubSub := dbtestutil.NewDB(t)
323-
controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry())
324-
325-
ownerID := uuid.New()
326-
dbgen.User(t, db, database.User{
327-
ID: ownerID,
328-
})
329-
org, template := setupTestDBTemplate(t, db, ownerID, templateDeleted)
330-
templateVersionID := setupTestDBTemplateVersion(
331-
ctx,
332-
t,
333-
clock,
334-
db,
335-
pubSub,
336-
org.ID,
337-
ownerID,
338-
template.ID,
339-
)
340-
preset := setupTestDBPreset(
341-
t,
342-
db,
343-
templateVersionID,
344-
1,
345-
uuid.New().String(),
346-
)
347-
prebuild := setupTestDBPrebuild(
348-
t,
349-
clock,
350-
db,
351-
pubSub,
352-
prebuildLatestTransition,
353-
prebuildJobStatus,
354-
org.ID,
355-
preset,
356-
template.ID,
357-
templateVersionID,
358-
)
359-
360-
if !templateVersionActive {
361-
// Create a new template version and mark it as active
362-
// This marks the template version that we care about as inactive
363-
setupTestDBTemplateVersion(ctx, t, clock, db, pubSub, org.ID, ownerID, template.ID)
364-
}
365-
366-
// Run the reconciliation multiple times to ensure idempotency
367-
// 8 was arbitrary, but large enough to reasonably trust the result
368-
for i := 1; i <= 8; i++ {
369-
require.NoErrorf(t, controller.ReconcileAll(ctx), "failed on iteration %d", i)
370-
371-
if tc.shouldCreateNewPrebuild != nil {
372-
newPrebuildCount := 0
373-
workspaces, err := db.GetWorkspacesByTemplateID(ctx, template.ID)
374-
require.NoError(t, err)
375-
for _, workspace := range workspaces {
376-
if workspace.ID != prebuild.ID {
377-
newPrebuildCount++
328+
329+
controller := prebuilds.NewStoreReconciler(db, pubSub, cfg, logger, quartz.NewMock(t), prometheus.NewRegistry())
330+
331+
ownerID := uuid.New()
332+
dbgen.User(t, db, database.User{
333+
ID: ownerID,
334+
})
335+
org, template := setupTestDBTemplate(t, db, ownerID, templateDeleted)
336+
templateVersionID := setupTestDBTemplateVersion(
337+
ctx,
338+
t,
339+
clock,
340+
db,
341+
pubSub,
342+
org.ID,
343+
ownerID,
344+
template.ID,
345+
)
346+
preset := setupTestDBPreset(
347+
t,
348+
db,
349+
templateVersionID,
350+
1,
351+
uuid.New().String(),
352+
)
353+
prebuild := setupTestDBPrebuild(
354+
t,
355+
clock,
356+
db,
357+
pubSub,
358+
prebuildLatestTransition,
359+
prebuildJobStatus,
360+
org.ID,
361+
preset,
362+
template.ID,
363+
templateVersionID,
364+
)
365+
366+
if !templateVersionActive {
367+
// Create a new template version and mark it as active
368+
// This marks the template version that we care about as inactive
369+
setupTestDBTemplateVersion(ctx, t, clock, db, pubSub, org.ID, ownerID, template.ID)
370+
}
371+
372+
// Run the reconciliation multiple times to ensure idempotency
373+
// 8 was arbitrary, but large enough to reasonably trust the result
374+
for i := 1; i <= 8; i++ {
375+
require.NoErrorf(t, controller.ReconcileAll(ctx), "failed on iteration %d", i)
376+
377+
if tc.shouldCreateNewPrebuild != nil {
378+
newPrebuildCount := 0
379+
workspaces, err := db.GetWorkspacesByTemplateID(ctx, template.ID)
380+
require.NoError(t, err)
381+
for _, workspace := range workspaces {
382+
if workspace.ID != prebuild.ID {
383+
newPrebuildCount++
384+
}
378385
}
386+
// This test configures a preset that desires one prebuild.
387+
// In cases where new prebuilds should be created, there should be exactly one.
388+
require.Equal(t, *tc.shouldCreateNewPrebuild, newPrebuildCount == 1)
379389
}
380-
// This test configures a preset that desires one prebuild.
381-
// In cases where new prebuilds should be created, there should be exactly one.
382-
require.Equal(t, *tc.shouldCreateNewPrebuild, newPrebuildCount == 1)
383-
}
384390

385-
if tc.shouldDeleteOldPrebuild != nil {
386-
builds, err := db.GetWorkspaceBuildsByWorkspaceID(ctx, database.GetWorkspaceBuildsByWorkspaceIDParams{
387-
WorkspaceID: prebuild.ID,
388-
})
389-
require.NoError(t, err)
390-
if *tc.shouldDeleteOldPrebuild {
391-
require.Equal(t, 2, len(builds))
392-
require.Equal(t, database.WorkspaceTransitionDelete, builds[0].Transition)
393-
} else {
394-
require.Equal(t, 1, len(builds))
395-
require.Equal(t, prebuildLatestTransition, builds[0].Transition)
391+
if tc.shouldDeleteOldPrebuild != nil {
392+
builds, err := db.GetWorkspaceBuildsByWorkspaceID(ctx, database.GetWorkspaceBuildsByWorkspaceIDParams{
393+
WorkspaceID: prebuild.ID,
394+
})
395+
require.NoError(t, err)
396+
if *tc.shouldDeleteOldPrebuild {
397+
require.Equal(t, 2, len(builds))
398+
require.Equal(t, database.WorkspaceTransitionDelete, builds[0].Transition)
399+
} else {
400+
require.Equal(t, 1, len(builds))
401+
require.Equal(t, prebuildLatestTransition, builds[0].Transition)
402+
}
396403
}
397404
}
398-
}
399-
})
405+
})
406+
}
400407
}
401408
}
402409
}
403410
}
404411
}
405412
}
406413

414+
// brokenPublisher is used to validate that Publish() calls which always fail do not affect the reconciler's behaviour,
415+
// since the messages published are not essential but merely advisory.
416+
type brokenPublisher struct {
417+
pubsub.Pubsub
418+
}
419+
420+
func (b *brokenPublisher) Publish(event string, _ []byte) error {
421+
// I'm explicitly _not_ checking for EventJobPosted (coderd/database/provisionerjobs/provisionerjobs.go) since that
422+
// required too much knowledge of the underlying implementation.
423+
return xerrors.Errorf("refusing to publish %q", event)
424+
}
425+
407426
func TestMultiplePresetsPerTemplateVersion(t *testing.T) {
408427
t.Parallel()
409428

0 commit comments

Comments
 (0)