From d851b013db1f9d501f4aaa72b85bdee5079d0287 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 27 Feb 2025 22:54:58 +0200 Subject: [PATCH 1/3] Improve resilience of e2e test Signed-off-by: Danny Kopping --- site/e2e/tests/presets/prebuilds.spec.ts | 84 ++++++++++++++++++------ 1 file changed, 64 insertions(+), 20 deletions(-) diff --git a/site/e2e/tests/presets/prebuilds.spec.ts b/site/e2e/tests/presets/prebuilds.spec.ts index 38a296be96..d1e78287fb 100644 --- a/site/e2e/tests/presets/prebuilds.spec.ts +++ b/site/e2e/tests/presets/prebuilds.spec.ts @@ -1,5 +1,5 @@ import path from "node:path"; -import { expect, test } from "@playwright/test"; +import { type Locator, expect, test } from "@playwright/test"; import { currentUser, importTemplate, @@ -14,18 +14,22 @@ test.beforeEach(async ({ page }) => { await login(page); }); +const waitForBuildTimeout = 120_000; // Builds can take a while, let's give them at most 2m. + +const templateFiles = [ + path.join(__dirname, "basic-presets-with-prebuild/main.tf"), + path.join(__dirname, "basic-presets-with-prebuild/.terraform.lock.hcl"), +]; + +const expectedPrebuilds = 2; + // NOTE: requires the `workspace-prebuilds` experiment enabled! test("create template with desired prebuilds", async ({ page, baseURL }) => { requiresLicense(); - const expectedPrebuilds = 2; - // Create new template. const templateName = randomName(); - await importTemplate(page, templateName, [ - path.join(__dirname, "basic-presets-with-prebuild/main.tf"), - path.join(__dirname, "basic-presets-with-prebuild/.terraform.lock.hcl"), - ]); + await importTemplate(page, templateName, templateFiles); await page.goto( `/workspaces?filter=owner:prebuilds%20template:${templateName}&page=1`, @@ -34,13 +38,13 @@ test("create template with desired prebuilds", async ({ page, baseURL }) => { // Wait for prebuilds to show up. const prebuilds = page.getByTestId(/^workspace-.+$/); - await prebuilds.first().waitFor({ state: "visible", timeout: 120_000 }); - expect((await prebuilds.all()).length).toEqual(expectedPrebuilds); + await waitForExpectedCount(prebuilds, expectedPrebuilds); // Wait for prebuilds to start. - const runningPrebuilds = page.getByTestId("build-status").getByText("Running"); - await runningPrebuilds.first().waitFor({ state: "visible", timeout: 120_000 }); - expect((await runningPrebuilds.all()).length).toEqual(expectedPrebuilds); + const runningPrebuilds = page + .getByTestId("build-status") + .getByText("Running"); + await waitForExpectedCount(runningPrebuilds, expectedPrebuilds); }); // NOTE: requires the `workspace-prebuilds` experiment enabled! @@ -51,10 +55,7 @@ test("claim prebuild matching selected preset", async ({ page, baseURL }) => { // Create new template. const templateName = randomName(); - await importTemplate(page, templateName, [ - path.join(__dirname, "basic-presets-with-prebuild/main.tf"), - path.join(__dirname, "basic-presets-with-prebuild/.terraform.lock.hcl"), - ]); + await importTemplate(page, templateName, templateFiles); await page.goto( `/workspaces?filter=owner:prebuilds%20template:${templateName}&page=1`, @@ -63,11 +64,17 @@ test("claim prebuild matching selected preset", async ({ page, baseURL }) => { // Wait for prebuilds to show up. const prebuilds = page.getByTestId(/^workspace-.+$/); - await prebuilds.first().waitFor({ state: "visible", timeout: 120_000 }); + await waitForExpectedCount(prebuilds, expectedPrebuilds); + + const previousWorkspaceNames = await Promise.all( + (await prebuilds.all()).map((value) => { + return value.getByText(/prebuild-.+/).textContent(); + }), + ); // Wait for prebuilds to start. - const runningPrebuilds = page.getByTestId("build-status").getByText("Running"); - await runningPrebuilds.first().waitFor({ state: "visible", timeout: 120_000 }); + let runningPrebuilds = page.getByTestId("build-status").getByText("Running"); + await waitForExpectedCount(runningPrebuilds, expectedPrebuilds); // Open the first prebuild. await runningPrebuilds.first().click(); @@ -101,7 +108,7 @@ test("claim prebuild matching selected preset", async ({ page, baseURL }) => { // Wait for the workspace build display to be navigated to. const user = currentUser(page); await page.waitForURL(`/@${user.username}/${workspaceName}`, { - timeout: 120_000, // Account for workspace build time. + timeout: waitForBuildTimeout, // Account for workspace build time. }); // Validate the workspace metadata that it was indeed a claimed prebuild. @@ -109,4 +116,41 @@ test("claim prebuild matching selected preset", async ({ page, baseURL }) => { await indicator.waitFor({ timeout: 60_000 }); const text = indicator.locator("xpath=..").getByText("Yes"); await text.waitFor({ timeout: 30_000 }); + + // Navigate back to prebuilds page to see that a new prebuild replaced the claimed one. + await page.goto( + `/workspaces?filter=owner:prebuilds%20template:${templateName}&page=1`, + { waitUntil: "domcontentloaded" }, + ); + + // Wait for prebuilds to show up. + const newPrebuilds = page.getByTestId(/^workspace-.+$/); + await waitForExpectedCount(newPrebuilds, expectedPrebuilds); + + const currentWorkspaceNames = await Promise.all( + (await newPrebuilds.all()).map((value) => { + return value.getByText(/prebuild-.+/).textContent(); + }), + ); + + // Ensure the prebuilds have changed. + expect(currentWorkspaceNames).not.toEqual(previousWorkspaceNames); + + // Wait for prebuilds to start. + runningPrebuilds = page.getByTestId("build-status").getByText("Running"); + await waitForExpectedCount(runningPrebuilds, expectedPrebuilds); }); + +function waitForExpectedCount(prebuilds: Locator, expectedCount: number) { + return expect + .poll( + async () => { + return (await prebuilds.all()).length === expectedCount; + }, + { + intervals: [100], + timeout: waitForBuildTimeout, + }, + ) + .toBe(true); +} From 6c2eb32552c28e9a66e2a4a06f5d6cbcaa05dc7f Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 28 Feb 2025 11:00:55 +0000 Subject: [PATCH 2/3] WIP: db tests --- enterprise/coderd/prebuilds/claim.go | 4 +- .../coderd/prebuilds/controller_test.go | 383 ++++++++++++------ enterprise/coderd/prebuilds/reconciliation.go | 8 +- 3 files changed, 270 insertions(+), 125 deletions(-) diff --git a/enterprise/coderd/prebuilds/claim.go b/enterprise/coderd/prebuilds/claim.go index fa4f48a389..9b76ff0b93 100644 --- a/enterprise/coderd/prebuilds/claim.go +++ b/enterprise/coderd/prebuilds/claim.go @@ -20,8 +20,8 @@ func (e EnterpriseClaimer) Claim(ctx context.Context, store database.Store, user err := store.InTx(func(db database.Store) error { // TODO: do we need this? //// Ensure no other replica can claim a prebuild for this user simultaneously. - //err := store.AcquireLock(ctx, database.GenLockID(fmt.Sprintf("prebuild-user-claim-%s", userID.String()))) - //if err != nil { + // err := store.AcquireLock(ctx, database.GenLockID(fmt.Sprintf("prebuild-user-claim-%s", userID.String()))) + // if err != nil { // return xerrors.Errorf("acquire claim lock for user %q: %w", userID.String(), err) //} diff --git a/enterprise/coderd/prebuilds/controller_test.go b/enterprise/coderd/prebuilds/controller_test.go index 6a78d25be0..bfd9b45931 100644 --- a/enterprise/coderd/prebuilds/controller_test.go +++ b/enterprise/coderd/prebuilds/controller_test.go @@ -1,6 +1,7 @@ package prebuilds import ( + "context" "database/sql" "testing" "time" @@ -11,6 +12,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/database/pubsub" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" "github.com/coder/serpent" @@ -108,6 +110,128 @@ func TestNoReconciliationActionsIfNoPrebuilds(t *testing.T) { require.Empty(t, jobs) } +func setupTestDBTemplate( + t *testing.T, + ctx context.Context, + db database.Store, +) ( + orgID uuid.UUID, + userID uuid.UUID, + templateID uuid.UUID, +) { + t.Helper() + org := dbgen.Organization(t, db, database.Organization{}) + user := dbgen.User(t, db, database.User{}) + + template := dbgen.Template(t, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + }) + + return org.ID, user.ID, template.ID +} +func setupTestDBPrebuild( + t *testing.T, + ctx context.Context, + db database.Store, + pubsub pubsub.Pubsub, + prebuildStatus database.WorkspaceStatus, + orgID uuid.UUID, + userID uuid.UUID, + templateID uuid.UUID, +) ( + templateVersionID uuid.UUID, + presetID uuid.UUID, + prebuildID uuid.UUID, +) { + templateVersionJob := dbgen.ProvisionerJob(t, db, pubsub, database.ProvisionerJob{ + ID: uuid.New(), + CreatedAt: time.Now().Add(-2 * time.Hour), + CompletedAt: sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true}, + OrganizationID: orgID, + InitiatorID: userID, + }) + templateVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: templateID, Valid: true}, + OrganizationID: orgID, + CreatedBy: userID, + JobID: templateVersionJob.ID, + }) + db.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{ + ID: templateID, + ActiveVersionID: templateVersion.ID, + }) + preset, err := db.InsertPreset(ctx, database.InsertPresetParams{ + TemplateVersionID: templateVersion.ID, + Name: "test", + }) + require.NoError(t, err) + _, err = db.InsertPresetParameters(ctx, database.InsertPresetParametersParams{ + TemplateVersionPresetID: preset.ID, + Names: []string{"test"}, + Values: []string{"test"}, + }) + require.NoError(t, err) + _, err = db.InsertPresetPrebuild(ctx, database.InsertPresetPrebuildParams{ + ID: uuid.New(), + PresetID: preset.ID, + DesiredInstances: 1, + }) + require.NoError(t, err) + + completedAt := sql.NullTime{} + cancelledAt := sql.NullTime{} + transition := database.WorkspaceTransitionStart + deleted := false + buildError := sql.NullString{} + switch prebuildStatus { + case database.WorkspaceStatusRunning: + completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} + case database.WorkspaceStatusStopped: + completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} + transition = database.WorkspaceTransitionStop + case database.WorkspaceStatusFailed: + completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} + buildError = sql.NullString{String: "build failed", Valid: true} + case database.WorkspaceStatusCanceled: + completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} + cancelledAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} + case database.WorkspaceStatusDeleted: + completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} + transition = database.WorkspaceTransitionDelete + deleted = true + case database.WorkspaceStatusPending: + completedAt = sql.NullTime{} + transition = database.WorkspaceTransitionStart + default: + } + + workspace := dbgen.Workspace(t, db, database.WorkspaceTable{ + TemplateID: templateID, + OrganizationID: orgID, + OwnerID: OwnerID, + Deleted: deleted, + }) + job := dbgen.ProvisionerJob(t, db, pubsub, database.ProvisionerJob{ + InitiatorID: OwnerID, + CreatedAt: time.Now().Add(-2 * time.Hour), + CompletedAt: completedAt, + CanceledAt: cancelledAt, + OrganizationID: orgID, + Error: buildError, + }) + prebuild := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: workspace.ID, + InitiatorID: OwnerID, + TemplateVersionID: templateVersion.ID, + JobID: job.ID, + TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, + Transition: transition, + }) + + return templateVersion.ID, preset.ID, prebuild.ID +} + func TestPrebuildCreation(t *testing.T) { t.Parallel() @@ -116,41 +240,63 @@ func TestPrebuildCreation(t *testing.T) { name string prebuildStatus database.WorkspaceStatus shouldCreateNewPrebuild bool + shouldDeleteOldPrebuild bool + templateVersionActive bool } testCases := []testCase{ { name: "running prebuild", prebuildStatus: database.WorkspaceStatusRunning, + templateVersionActive: true, shouldCreateNewPrebuild: false, + shouldDeleteOldPrebuild: false, }, - { - name: "stopped prebuild", - prebuildStatus: database.WorkspaceStatusStopped, - shouldCreateNewPrebuild: true, - }, - { - name: "failed prebuild", - prebuildStatus: database.WorkspaceStatusFailed, - shouldCreateNewPrebuild: true, - }, - { - name: "canceled prebuild", - prebuildStatus: database.WorkspaceStatusCanceled, - shouldCreateNewPrebuild: true, - }, - { - name: "deleted prebuild", - prebuildStatus: database.WorkspaceStatusDeleted, - shouldCreateNewPrebuild: true, - }, - { - name: "pending prebuild", - prebuildStatus: database.WorkspaceStatusPending, - shouldCreateNewPrebuild: false, - }, + // { + // name: "stopped prebuild", + // prebuildStatus: database.WorkspaceStatusStopped, + // templateVersionActive: true, + // shouldCreateNewPrebuild: true, + // shouldDeleteOldPrebuild: false, + // }, + // { + // name: "failed prebuild", + // prebuildStatus: database.WorkspaceStatusFailed, + // templateVersionActive: true, + // shouldCreateNewPrebuild: true, + // shouldDeleteOldPrebuild: true, + // }, + // { + // name: "canceled prebuild", + // prebuildStatus: database.WorkspaceStatusCanceled, + // templateVersionActive: true, + // shouldCreateNewPrebuild: true, + // shouldDeleteOldPrebuild: true, + // }, + // { + // name: "deleted prebuild", + // prebuildStatus: database.WorkspaceStatusDeleted, + // templateVersionActive: true, + // shouldCreateNewPrebuild: true, + // shouldDeleteOldPrebuild: false, + // }, + // { + // name: "pending prebuild", + // prebuildStatus: database.WorkspaceStatusPending, + // templateVersionActive: true, + // shouldCreateNewPrebuild: false, + // shouldDeleteOldPrebuild: false, + // }, + // { + // name: "inactive template version", + // prebuildStatus: database.WorkspaceStatusRunning, + // shouldDeleteOldPrebuild: true, + // templateVersionActive: false, + // shouldCreateNewPrebuild: false, + // }, } for _, tc := range testCases { + tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) @@ -159,102 +305,50 @@ func TestPrebuildCreation(t *testing.T) { logger := testutil.Logger(t) controller := NewController(db, pubsub, cfg, logger) - // given a user - org := dbgen.Organization(t, db, database.Organization{}) - user := dbgen.User(t, db, database.User{}) - - template := dbgen.Template(t, db, database.Template{ - CreatedBy: user.ID, - OrganizationID: org.ID, - }) - templateVersionJob := dbgen.ProvisionerJob(t, db, pubsub, database.ProvisionerJob{ - ID: uuid.New(), - CreatedAt: time.Now().Add(-2 * time.Hour), - CompletedAt: sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true}, - OrganizationID: org.ID, - InitiatorID: user.ID, - }) - templateVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{ - TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, - OrganizationID: org.ID, - CreatedBy: user.ID, - JobID: templateVersionJob.ID, - }) - db.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{ - ID: template.ID, - ActiveVersionID: templateVersion.ID, - }) - preset, err := db.InsertPreset(ctx, database.InsertPresetParams{ - TemplateVersionID: templateVersion.ID, - Name: "test", - }) - require.NoError(t, err) - _, err = db.InsertPresetParameters(ctx, database.InsertPresetParametersParams{ - TemplateVersionPresetID: preset.ID, - Names: []string{"test"}, - Values: []string{"test"}, - }) - require.NoError(t, err) - _, err = db.InsertPresetPrebuild(ctx, database.InsertPresetPrebuildParams{ - ID: uuid.New(), - PresetID: preset.ID, - DesiredInstances: 1, - }) - require.NoError(t, err) - completedAt := sql.NullTime{} - cancelledAt := sql.NullTime{} - transition := database.WorkspaceTransitionStart - deleted := false - buildError := sql.NullString{} - switch tc.prebuildStatus { - case database.WorkspaceStatusRunning: - completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} - case database.WorkspaceStatusStopped: - completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} - transition = database.WorkspaceTransitionStop - case database.WorkspaceStatusFailed: - completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} - buildError = sql.NullString{String: "build failed", Valid: true} - case database.WorkspaceStatusCanceled: - completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} - cancelledAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} - case database.WorkspaceStatusDeleted: - completedAt = sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true} - transition = database.WorkspaceTransitionDelete - deleted = true - case database.WorkspaceStatusPending: - completedAt = sql.NullTime{} - transition = database.WorkspaceTransitionStart - default: + orgID, userID, templateID := setupTestDBTemplate(t, ctx, db) + _, _, prebuildID := setupTestDBPrebuild( + t, + ctx, + db, + pubsub, + tc.prebuildStatus, + orgID, + userID, + templateID, + ) + if !tc.templateVersionActive { + _, _, _ = setupTestDBPrebuild( + t, + ctx, + db, + pubsub, + tc.prebuildStatus, + orgID, + userID, + templateID, + ) } - - workspace := dbgen.Workspace(t, db, database.WorkspaceTable{ - TemplateID: template.ID, - OrganizationID: org.ID, - OwnerID: OwnerID, - Deleted: deleted, - }) - job := dbgen.ProvisionerJob(t, db, pubsub, database.ProvisionerJob{ - InitiatorID: OwnerID, - CreatedAt: time.Now().Add(-2 * time.Hour), - CompletedAt: completedAt, - CanceledAt: cancelledAt, - OrganizationID: org.ID, - Error: buildError, - }) - dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ - WorkspaceID: workspace.ID, - InitiatorID: OwnerID, - TemplateVersionID: templateVersion.ID, - JobID: job.ID, - TemplateVersionPresetID: uuid.NullUUID{UUID: preset.ID, Valid: true}, - Transition: transition, - }) - controller.reconcile(ctx, nil) - jobs, err := db.GetProvisionerJobsCreatedAfter(ctx, time.Now().Add(-time.Hour)) + + createdNewPrebuild := false + deletedOldPrebuild := false + workspaces, err := db.GetWorkspacesByTemplateID(ctx, templateID) require.NoError(t, err) - require.Equal(t, tc.shouldCreateNewPrebuild, len(jobs) == 1) + for _, workspace := range workspaces { + if workspace.ID == prebuildID && workspace.Deleted { + deletedOldPrebuild = true + } + + if workspace.ID != prebuildID && !workspace.Deleted { + createdNewPrebuild = true + } + } + require.Equal(t, tc.shouldCreateNewPrebuild, createdNewPrebuild) + + // workspacebuilds, err := db.GetWorkspaceBuildsCreatedAfter(ctx, time.Now().Add(-24*time.Hour)) + // require.NoError(t, err) + + require.Equal(t, tc.shouldDeleteOldPrebuild, deletedOldPrebuild) }) } } @@ -269,9 +363,60 @@ func TestDeleteUnwantedPrebuilds(t *testing.T) { logger := testutil.Logger(t) controller := NewController(db, pubsub, cfg, logger) + type testCase struct { + name string + prebuildStatus database.WorkspaceStatus + shouldDelete bool + } + + testCases := []testCase{ + { + name: "running prebuild", + prebuildStatus: database.WorkspaceStatusRunning, + shouldDelete: false, + }, + { + name: "stopped prebuild", + prebuildStatus: database.WorkspaceStatusStopped, + shouldDelete: true, + }, + { + name: "failed prebuild", + prebuildStatus: database.WorkspaceStatusFailed, + shouldDelete: true, + }, + { + name: "canceled prebuild", + prebuildStatus: database.WorkspaceStatusCanceled, + shouldDelete: true, + }, + { + name: "deleted prebuild", + prebuildStatus: database.WorkspaceStatusDeleted, + shouldDelete: true, + }, + { + name: "pending prebuild", + prebuildStatus: database.WorkspaceStatusPending, + shouldDelete: false, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + db, pubsub := dbtestutil.NewDB(t) + cfg := codersdk.PrebuildsConfig{} + logger := testutil.Logger(t) + controller := NewController(db, pubsub, cfg, logger) + controller.reconcile(ctx, nil) + jobs, err := db.GetProvisionerJobsCreatedAfter(ctx, time.Now().Add(-time.Hour)) + require.NoError(t, err) + require.Equal(t, tc.shouldDelete, len(jobs) == 1) + }) + } // when does a prebuild get deleted? // * when it is in some way permanently ineligible to be claimed - // * this could be because the build failed or was canceled // * or it belongs to a template version that is no longer active // * or it belongs to a template version that is deprecated // * when there are more prebuilds than the preset desires diff --git a/enterprise/coderd/prebuilds/reconciliation.go b/enterprise/coderd/prebuilds/reconciliation.go index bf14ab735c..e0efd8339f 100644 --- a/enterprise/coderd/prebuilds/reconciliation.go +++ b/enterprise/coderd/prebuilds/reconciliation.go @@ -153,7 +153,7 @@ func (p presetState) calculateActions() (*reconciliationActions, error) { // TODO: optimization: we should probably be able to create prebuilds while others are deleting for a given preset. if (toCreate+toDelete) > 0 && (starting+stopping+deleting) > 0 { // TODO: move up - //c.logger.Warn(ctx, "prebuild operations in progress, skipping reconciliation", + // c.logger.Warn(ctx, "prebuild operations in progress, skipping reconciliation", // slog.F("template_id", p.preset.TemplateID.String()), slog.F("starting", starting), // slog.F("stopping", stopping), slog.F("deleting", deleting), // slog.F("wanted_to_create", create), slog.F("wanted_to_delete", toDelete)) @@ -181,7 +181,7 @@ func (p presetState) calculateActions() (*reconciliationActions, error) { if i >= len(p.running) { // This should never happen. // TODO: move up - //c.logger.Warn(ctx, "unexpected reconciliation state; extraneous count exceeds running prebuilds count!", + // c.logger.Warn(ctx, "unexpected reconciliation state; extraneous count exceeds running prebuilds count!", // slog.F("running_count", len(p.running)), // slog.F("extraneous", extraneous)) continue @@ -193,7 +193,7 @@ func (p presetState) calculateActions() (*reconciliationActions, error) { actions.deleteIDs = append(actions.deleteIDs, victims...) // TODO: move up - //c.logger.Warn(ctx, "found extra prebuilds running, picking random victim(s)", + // c.logger.Warn(ctx, "found extra prebuilds running, picking random victim(s)", // slog.F("template_id", p.preset.TemplateID.String()), slog.F("desired", desired), slog.F("actual", actual), slog.F("extra", extraneous), // slog.F("victims", victims)) @@ -205,7 +205,7 @@ func (p presetState) calculateActions() (*reconciliationActions, error) { if toDelete > 0 && len(p.running) != toDelete { // TODO: move up - //c.logger.Warn(ctx, "mismatch between running prebuilds and expected deletion count!", + // c.logger.Warn(ctx, "mismatch between running prebuilds and expected deletion count!", // slog.F("template_id", s.preset.TemplateID.String()), slog.F("running", len(p.running)), slog.F("to_delete", toDelete)) } From e16d763241336c812957b24399003f9660e7c400 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 3 Mar 2025 10:20:47 +0000 Subject: [PATCH 3/3] add tests to ensure that preubilds are correctly provisioned for active template versions --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/prebuilds.sql | 2 +- enterprise/coderd/prebuilds/controller.go | 3 + .../coderd/prebuilds/controller_test.go | 183 ++++-------------- 4 files changed, 43 insertions(+), 147 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 222846fdf0..e4d70d576f 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5624,7 +5624,7 @@ FROM workspace_prebuilds p ON tvp_curr.id = p.current_preset_id -- See https://github.com/coder/internal/issues/398. WHERE (b.transition = 'start'::workspace_transition -- Jobs that are not in terminal states. - OR pj.job_status IN ('failed'::provisioner_job_status, 'canceled'::provisioner_job_status, + AND pj.job_status NOT IN ('failed'::provisioner_job_status, 'canceled'::provisioner_job_status, 'unknown'::provisioner_job_status)) ` diff --git a/coderd/database/queries/prebuilds.sql b/coderd/database/queries/prebuilds.sql index ef8f4f0779..e98999935f 100644 --- a/coderd/database/queries/prebuilds.sql +++ b/coderd/database/queries/prebuilds.sql @@ -18,7 +18,7 @@ FROM workspace_prebuilds p ON tvp_curr.id = p.current_preset_id -- See https://github.com/coder/internal/issues/398. WHERE (b.transition = 'start'::workspace_transition -- Jobs that are not in terminal states. - OR pj.job_status IN ('failed'::provisioner_job_status, 'canceled'::provisioner_job_status, + AND pj.job_status NOT IN ('failed'::provisioner_job_status, 'canceled'::provisioner_job_status, 'unknown'::provisioner_job_status)); -- name: GetTemplatePresetsWithPrebuilds :many diff --git a/enterprise/coderd/prebuilds/controller.go b/enterprise/coderd/prebuilds/controller.go index 7801b6f826..5e8b08e173 100644 --- a/enterprise/coderd/prebuilds/controller.go +++ b/enterprise/coderd/prebuilds/controller.go @@ -223,6 +223,9 @@ func (c *Controller) determineState(ctx context.Context, store database.Store, t } presetsWithPrebuilds, err := db.GetTemplatePresetsWithPrebuilds(ctx, dbTemplateID) + if err != nil { + return xerrors.Errorf("failed to get template presets with prebuilds: %w", err) + } if len(presetsWithPrebuilds) == 0 { return nil } diff --git a/enterprise/coderd/prebuilds/controller_test.go b/enterprise/coderd/prebuilds/controller_test.go index bfd9b45931..d31be04c18 100644 --- a/enterprise/coderd/prebuilds/controller_test.go +++ b/enterprise/coderd/prebuilds/controller_test.go @@ -112,7 +112,6 @@ func TestNoReconciliationActionsIfNoPrebuilds(t *testing.T) { func setupTestDBTemplate( t *testing.T, - ctx context.Context, db database.Store, ) ( orgID uuid.UUID, @@ -220,7 +219,7 @@ func setupTestDBPrebuild( OrganizationID: orgID, Error: buildError, }) - prebuild := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ WorkspaceID: workspace.ID, InitiatorID: OwnerID, TemplateVersionID: templateVersion.ID, @@ -229,71 +228,58 @@ func setupTestDBPrebuild( Transition: transition, }) - return templateVersion.ID, preset.ID, prebuild.ID + return templateVersion.ID, preset.ID, workspace.ID } -func TestPrebuildCreation(t *testing.T) { +func TestActiveTemplateVersionPrebuilds(t *testing.T) { t.Parallel() - // Scenario: Prebuilds are created if and only if they are needed type testCase struct { name string prebuildStatus database.WorkspaceStatus shouldCreateNewPrebuild bool shouldDeleteOldPrebuild bool - templateVersionActive bool } testCases := []testCase{ { name: "running prebuild", prebuildStatus: database.WorkspaceStatusRunning, - templateVersionActive: true, shouldCreateNewPrebuild: false, shouldDeleteOldPrebuild: false, }, - // { - // name: "stopped prebuild", - // prebuildStatus: database.WorkspaceStatusStopped, - // templateVersionActive: true, - // shouldCreateNewPrebuild: true, - // shouldDeleteOldPrebuild: false, - // }, - // { - // name: "failed prebuild", - // prebuildStatus: database.WorkspaceStatusFailed, - // templateVersionActive: true, - // shouldCreateNewPrebuild: true, - // shouldDeleteOldPrebuild: true, - // }, - // { - // name: "canceled prebuild", - // prebuildStatus: database.WorkspaceStatusCanceled, - // templateVersionActive: true, - // shouldCreateNewPrebuild: true, - // shouldDeleteOldPrebuild: true, - // }, + { + name: "stopped prebuild", + prebuildStatus: database.WorkspaceStatusStopped, + shouldCreateNewPrebuild: true, + shouldDeleteOldPrebuild: false, + }, + { + name: "failed prebuild", + prebuildStatus: database.WorkspaceStatusFailed, + shouldCreateNewPrebuild: true, + shouldDeleteOldPrebuild: false, + }, + { + name: "canceled prebuild", + prebuildStatus: database.WorkspaceStatusCanceled, + shouldCreateNewPrebuild: true, + shouldDeleteOldPrebuild: false, + }, // { // name: "deleted prebuild", // prebuildStatus: database.WorkspaceStatusDeleted, - // templateVersionActive: true, + // shouldConsiderPrebuildRunning: false, + // shouldConsiderPrebuildInProgress: false, // shouldCreateNewPrebuild: true, // shouldDeleteOldPrebuild: false, // }, - // { - // name: "pending prebuild", - // prebuildStatus: database.WorkspaceStatusPending, - // templateVersionActive: true, - // shouldCreateNewPrebuild: false, - // shouldDeleteOldPrebuild: false, - // }, - // { - // name: "inactive template version", - // prebuildStatus: database.WorkspaceStatusRunning, - // shouldDeleteOldPrebuild: true, - // templateVersionActive: false, - // shouldCreateNewPrebuild: false, - // }, + { + name: "pending prebuild", + prebuildStatus: database.WorkspaceStatusPending, + shouldCreateNewPrebuild: false, + shouldDeleteOldPrebuild: false, + }, } for _, tc := range testCases { tc := tc @@ -305,7 +291,7 @@ func TestPrebuildCreation(t *testing.T) { logger := testutil.Logger(t) controller := NewController(db, pubsub, cfg, logger) - orgID, userID, templateID := setupTestDBTemplate(t, ctx, db) + orgID, userID, templateID := setupTestDBTemplate(t, db) _, _, prebuildID := setupTestDBPrebuild( t, ctx, @@ -316,125 +302,32 @@ func TestPrebuildCreation(t *testing.T) { userID, templateID, ) - if !tc.templateVersionActive { - _, _, _ = setupTestDBPrebuild( - t, - ctx, - db, - pubsub, - tc.prebuildStatus, - orgID, - userID, - templateID, - ) - } + controller.reconcile(ctx, nil) createdNewPrebuild := false - deletedOldPrebuild := false + deletedOldPrebuild := true workspaces, err := db.GetWorkspacesByTemplateID(ctx, templateID) require.NoError(t, err) for _, workspace := range workspaces { - if workspace.ID == prebuildID && workspace.Deleted { - deletedOldPrebuild = true + if workspace.ID == prebuildID { + deletedOldPrebuild = false } - if workspace.ID != prebuildID && !workspace.Deleted { + if workspace.ID != prebuildID { createdNewPrebuild = true } } require.Equal(t, tc.shouldCreateNewPrebuild, createdNewPrebuild) - - // workspacebuilds, err := db.GetWorkspaceBuildsCreatedAfter(ctx, time.Now().Add(-24*time.Hour)) - // require.NoError(t, err) - require.Equal(t, tc.shouldDeleteOldPrebuild, deletedOldPrebuild) }) } } -func TestDeleteUnwantedPrebuilds(t *testing.T) { - // Scenario: Prebuilds are deleted if and only if they are extraneous +func TestInactiveTemplateVersionPrebuilds(t *testing.T) { + // Scenario: Prebuilds are never created and always deleted if the template version is inactive t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - db, pubsub := dbtestutil.NewDB(t) - cfg := codersdk.PrebuildsConfig{} - logger := testutil.Logger(t) - controller := NewController(db, pubsub, cfg, logger) - - type testCase struct { - name string - prebuildStatus database.WorkspaceStatus - shouldDelete bool - } - - testCases := []testCase{ - { - name: "running prebuild", - prebuildStatus: database.WorkspaceStatusRunning, - shouldDelete: false, - }, - { - name: "stopped prebuild", - prebuildStatus: database.WorkspaceStatusStopped, - shouldDelete: true, - }, - { - name: "failed prebuild", - prebuildStatus: database.WorkspaceStatusFailed, - shouldDelete: true, - }, - { - name: "canceled prebuild", - prebuildStatus: database.WorkspaceStatusCanceled, - shouldDelete: true, - }, - { - name: "deleted prebuild", - prebuildStatus: database.WorkspaceStatusDeleted, - shouldDelete: true, - }, - { - name: "pending prebuild", - prebuildStatus: database.WorkspaceStatusPending, - shouldDelete: false, - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - ctx := testutil.Context(t, testutil.WaitShort) - db, pubsub := dbtestutil.NewDB(t) - cfg := codersdk.PrebuildsConfig{} - logger := testutil.Logger(t) - controller := NewController(db, pubsub, cfg, logger) - controller.reconcile(ctx, nil) - jobs, err := db.GetProvisionerJobsCreatedAfter(ctx, time.Now().Add(-time.Hour)) - require.NoError(t, err) - require.Equal(t, tc.shouldDelete, len(jobs) == 1) - }) - } - // when does a prebuild get deleted? - // * when it is in some way permanently ineligible to be claimed - // * or it belongs to a template version that is no longer active - // * or it belongs to a template version that is deprecated - // * when there are more prebuilds than the preset desires - // * someone could have manually created a workspace for the prebuild user - // * any workspaces that were created for the prebuilds user and don't match a preset should be deleted - deferred - - // given a preset that desires 2 prebuilds - // and there are 3 running prebuilds for the preset - // and there are 4 non-running prebuilds for the preset - // * one is not running because its latest build was a stop transition - // * another is not running because its latest build was a delete transition - // * a third is not running because its latest build was a start transition but the build failed - // * a fourth is not running because its latest build was a start transition but the build was canceled - // when we trigger the reconciliation loop for all templates - controller.reconcile(ctx, nil) - // then the four non running prebuilds are deleted - // and 1 of the running prebuilds is deleted - // because stopped, deleted and failed builds are not considered running in terms of the definition of "running" above. + t.Skip("todo") } // TODO (sasswart): test claim (success, fail) x (eligible)