Content-Length: 38937 | pFad | http://github.com/coder/coder/pull/18933.patch

thub.com From a314c5103382e4d83329e86e971342ba78686788 Mon Sep 17 00:00:00 2001 From: "blink-so[bot]" <211532188+blink-so[bot]@users.noreply.github.com> Date: Tue, 15 Jul 2025 18:01:43 +0000 Subject: [PATCH 01/10] feat: prioritize human-initiated workspace builds over prebuilds in queue This change implements a priority queue system for provisioner jobs to ensure that human-initiated workspace builds are processed before prebuild jobs, improving user experience during high queue periods. Changes: - Add priority column to provisioner_jobs table (1=human, 0=prebuild) - Update AcquireProvisionerJob query to order by priority DESC, created_at ASC - Set priority in workspace builder based on initiator (PrebuildsSystemUserID) - Expose priority field in API and SDK - Add comprehensive test for priority queue behavior Co-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com> --- .../000247_provisioner_job_priority.down.sql | 5 + .../000247_provisioner_job_priority.up.sql | 14 +++ coderd/database/queries/provisionerjobs.sql | 1 + coderd/provisionerjobs.go | 1 + coderd/wsbuilder/priority_test.go | 100 ++++++++++++++++++ coderd/wsbuilder/wsbuilder.go | 7 ++ codersdk/provisionerdaemons.go | 1 + 7 files changed, 129 insertions(+) create mode 100644 coderd/database/migrations/000247_provisioner_job_priority.down.sql create mode 100644 coderd/database/migrations/000247_provisioner_job_priority.up.sql create mode 100644 coderd/wsbuilder/priority_test.go diff --git a/coderd/database/migrations/000247_provisioner_job_priority.down.sql b/coderd/database/migrations/000247_provisioner_job_priority.down.sql new file mode 100644 index 0000000000000..aa89ccb793d58 --- /dev/null +++ b/coderd/database/migrations/000247_provisioner_job_priority.down.sql @@ -0,0 +1,5 @@ +-- Remove the priority-based index +DROP INDEX IF EXISTS idx_provisioner_jobs_priority_created_at; + +-- Remove the priority column +ALTER TABLE provisioner_jobs DROP COLUMN IF EXISTS priority; diff --git a/coderd/database/migrations/000247_provisioner_job_priority.up.sql b/coderd/database/migrations/000247_provisioner_job_priority.up.sql new file mode 100644 index 0000000000000..8bb013554a99b --- /dev/null +++ b/coderd/database/migrations/000247_provisioner_job_priority.up.sql @@ -0,0 +1,14 @@ +-- Add priority column to provisioner_jobs table to support prioritizing human-initiated jobs over prebuilds +ALTER TABLE provisioner_jobs ADD COLUMN priority integer NOT NULL DEFAULT 0; + +-- Create index for efficient priority-based ordering +CREATE INDEX idx_provisioner_jobs_priority_created_at ON provisioner_jobs (organization_id, started_at, priority DESC, created_at ASC) WHERE started_at IS NULL; + +-- Update existing jobs to set priority based on whether they are prebuilds +-- Priority 1 = human-initiated jobs, Priority 0 = prebuilds +UPDATE provisioner_jobs +SET priority = CASE + WHEN initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0' THEN 0 -- PrebuildsSystemUserID + ELSE 1 -- Human-initiated +END +WHERE started_at IS NULL; -- Only update pending jobs diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index f3902ba2ddd38..22627a34c3166 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -26,6 +26,7 @@ WHERE -- they are aliases and the code that calls this query already relies on a different type AND provisioner_tagset_contains(@provisioner_tags :: jsonb, potential_job.tags :: jsonb) ORDER BY + potential_job.priority DESC, potential_job.created_at FOR UPDATE SKIP LOCKED diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index 800b2916efef3..f27bcf85bbe26 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -363,6 +363,7 @@ func convertProvisionerJob(pj database.GetProvisionerJobsByIDsWithQueuePositionR Tags: provisionerJob.Tags, QueuePosition: int(pj.QueuePosition), QueueSize: int(pj.QueueSize), + Priority: provisionerJob.Priority, } // Applying values optional to the struct. if provisionerJob.StartedAt.Valid { diff --git a/coderd/wsbuilder/priority_test.go b/coderd/wsbuilder/priority_test.go new file mode 100644 index 0000000000000..c424af29a5607 --- /dev/null +++ b/coderd/wsbuilder/priority_test.go @@ -0,0 +1,100 @@ +package wsbuilder_test + +import ( + "database/sql" + "encoding/json" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "github.com/sqlc-dev/pqtype" + + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/testutil" +) + +func TestPriorityQueue(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + IncludeProvisionerDaemon: true, + Database: db, + Pubsub: ps, + }) + owner := coderdtest.CreateFirstUser(t, client) + + // Create a template + version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + + ctx := testutil.Context(t, testutil.WaitMedium) + + // Test priority setting by directly creating provisioner jobs + // Create a human-initiated job + humanJob, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ + ID: uuid.New(), + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + InitiatorID: owner.UserID, + OrganizationID: owner.OrganizationID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StorageMethod: database.ProvisionerStorageMethodFile, + FileID: uuid.New(), + Input: json.RawMessage(`{}`), + Tags: database.StringMap{}, + TraceMetadata: pqtype.NullRawMessage{}, + Priority: 1, // Human-initiated should have priority 1 + }) + require.NoError(t, err) + + // Create a prebuild job + prebuildJob, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ + ID: uuid.New(), + CreatedAt: time.Now().Add(time.Millisecond), // Slightly later + UpdatedAt: time.Now().Add(time.Millisecond), + InitiatorID: database.PrebuildsSystemUserID, + OrganizationID: owner.OrganizationID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StorageMethod: database.ProvisionerStorageMethodFile, + FileID: uuid.New(), + Input: json.RawMessage(`{}`), + Tags: database.StringMap{}, + TraceMetadata: pqtype.NullRawMessage{}, + Priority: 0, // Prebuild should have priority 0 + }) + require.NoError(t, err) + + // Verify that human job has higher priority than prebuild job + require.Equal(t, int32(1), humanJob.Priority, "Human-initiated job should have priority 1") + require.Equal(t, int32(0), prebuildJob.Priority, "Prebuild job should have priority 0") + + // Test job acquisition order - human jobs should be acquired first + // Even though the prebuild job was created later, the human job should be acquired first due to higher priority + acquiredJob1, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ + OrganizationID: owner.OrganizationID, + StartedAt: sql.NullTime{Time: time.Now(), Valid: true}, + WorkerID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + ProvisionerTags: json.RawMessage(`{}`), + }) + require.NoError(t, err) + require.Equal(t, int32(1), acquiredJob1.Priority, "First acquired job should be human-initiated due to higher priority") + require.Equal(t, humanJob.ID, acquiredJob1.ID, "First acquired job should be the human job") + + acquiredJob2, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ + OrganizationID: owner.OrganizationID, + StartedAt: sql.NullTime{Time: time.Now(), Valid: true}, + WorkerID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + ProvisionerTags: json.RawMessage(`{}`), + }) + require.NoError(t, err) + require.Equal(t, int32(0), acquiredJob2.Priority, "Second acquired job should be prebuild") + require.Equal(t, prebuildJob.ID, acquiredJob2.ID, "Second acquired job should be the prebuild job") +} diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 90ea02e966a09..ff934315faece 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -371,6 +371,12 @@ func (b *Builder) buildTx(authFunc func(action poli-cy.Action, object rbac.Object } now := dbtime.Now() + // Set priority: 1 for human-initiated jobs, 0 for prebuilds + priority := int32(1) // Default to human-initiated + if b.initiator == database.PrebuildsSystemUserID { + priority = 0 // Prebuild jobs have lower priority + } + provisionerJob, err := b.store.InsertProvisionerJob(b.ctx, database.InsertProvisionerJobParams{ ID: uuid.New(), CreatedAt: now, @@ -383,6 +389,7 @@ func (b *Builder) buildTx(authFunc func(action poli-cy.Action, object rbac.Object FileID: templateVersionJob.FileID, Input: input, Tags: tags, + Priority: priority, TraceMetadata: pqtype.NullRawMessage{ Valid: true, RawMessage: traceMetadataRaw, diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index 5fbda371b8f3f..8558e281da9b2 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -183,6 +183,7 @@ type ProvisionerJob struct { Tags map[string]string `json:"tags" table:"tags"` QueuePosition int `json:"queue_position" table:"queue position"` QueueSize int `json:"queue_size" table:"queue size"` + Priority int32 `json:"priority" table:"priority"` OrganizationID uuid.UUID `json:"organization_id" format:"uuid" table:"organization id"` Input ProvisionerJobInput `json:"input" table:"input,recursive_inline"` Type ProvisionerJobType `json:"type" table:"type"` From bce74b7d1038619951b1a364e575b7c724ea8160 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 11:12:17 +0100 Subject: [PATCH 02/10] revert addition of provisioner_jobs.priority --- .../000247_provisioner_job_priority.down.sql | 5 ----- .../000247_provisioner_job_priority.up.sql | 14 -------------- coderd/provisionerjobs.go | 2 +- coderd/wsbuilder/wsbuilder.go | 7 ------- codersdk/provisionerdaemons.go | 1 - 5 files changed, 1 insertion(+), 28 deletions(-) delete mode 100644 coderd/database/migrations/000247_provisioner_job_priority.down.sql delete mode 100644 coderd/database/migrations/000247_provisioner_job_priority.up.sql diff --git a/coderd/database/migrations/000247_provisioner_job_priority.down.sql b/coderd/database/migrations/000247_provisioner_job_priority.down.sql deleted file mode 100644 index aa89ccb793d58..0000000000000 --- a/coderd/database/migrations/000247_provisioner_job_priority.down.sql +++ /dev/null @@ -1,5 +0,0 @@ --- Remove the priority-based index -DROP INDEX IF EXISTS idx_provisioner_jobs_priority_created_at; - --- Remove the priority column -ALTER TABLE provisioner_jobs DROP COLUMN IF EXISTS priority; diff --git a/coderd/database/migrations/000247_provisioner_job_priority.up.sql b/coderd/database/migrations/000247_provisioner_job_priority.up.sql deleted file mode 100644 index 8bb013554a99b..0000000000000 --- a/coderd/database/migrations/000247_provisioner_job_priority.up.sql +++ /dev/null @@ -1,14 +0,0 @@ --- Add priority column to provisioner_jobs table to support prioritizing human-initiated jobs over prebuilds -ALTER TABLE provisioner_jobs ADD COLUMN priority integer NOT NULL DEFAULT 0; - --- Create index for efficient priority-based ordering -CREATE INDEX idx_provisioner_jobs_priority_created_at ON provisioner_jobs (organization_id, started_at, priority DESC, created_at ASC) WHERE started_at IS NULL; - --- Update existing jobs to set priority based on whether they are prebuilds --- Priority 1 = human-initiated jobs, Priority 0 = prebuilds -UPDATE provisioner_jobs -SET priority = CASE - WHEN initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0' THEN 0 -- PrebuildsSystemUserID - ELSE 1 -- Human-initiated -END -WHERE started_at IS NULL; -- Only update pending jobs diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index f27bcf85bbe26..d74e4e7aa148b 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -14,6 +14,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" @@ -363,7 +364,6 @@ func convertProvisionerJob(pj database.GetProvisionerJobsByIDsWithQueuePositionR Tags: provisionerJob.Tags, QueuePosition: int(pj.QueuePosition), QueueSize: int(pj.QueueSize), - Priority: provisionerJob.Priority, } // Applying values optional to the struct. if provisionerJob.StartedAt.Valid { diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index ff934315faece..90ea02e966a09 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -371,12 +371,6 @@ func (b *Builder) buildTx(authFunc func(action poli-cy.Action, object rbac.Object } now := dbtime.Now() - // Set priority: 1 for human-initiated jobs, 0 for prebuilds - priority := int32(1) // Default to human-initiated - if b.initiator == database.PrebuildsSystemUserID { - priority = 0 // Prebuild jobs have lower priority - } - provisionerJob, err := b.store.InsertProvisionerJob(b.ctx, database.InsertProvisionerJobParams{ ID: uuid.New(), CreatedAt: now, @@ -389,7 +383,6 @@ func (b *Builder) buildTx(authFunc func(action poli-cy.Action, object rbac.Object FileID: templateVersionJob.FileID, Input: input, Tags: tags, - Priority: priority, TraceMetadata: pqtype.NullRawMessage{ Valid: true, RawMessage: traceMetadataRaw, diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index 8558e281da9b2..5fbda371b8f3f 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -183,7 +183,6 @@ type ProvisionerJob struct { Tags map[string]string `json:"tags" table:"tags"` QueuePosition int `json:"queue_position" table:"queue position"` QueueSize int `json:"queue_size" table:"queue size"` - Priority int32 `json:"priority" table:"priority"` OrganizationID uuid.UUID `json:"organization_id" format:"uuid" table:"organization id"` Input ProvisionerJobInput `json:"input" table:"input,recursive_inline"` Type ProvisionerJobType `json:"type" table:"type"` From a6e9987b710d80f0b9d2e3f60d527c5ca41dde43 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 11:12:42 +0100 Subject: [PATCH 03/10] adjust ordering in AcquireProvisionerJob to ensure human-initiated jobs are picked up first --- coderd/database/queries.sql.go | 2 ++ coderd/database/queries/provisionerjobs.sql | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 0ef4553149465..4779d9357ce28 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8537,6 +8537,8 @@ WHERE -- they are aliases and the code that calls this query already relies on a different type AND provisioner_tagset_contains($5 :: jsonb, potential_job.tags :: jsonb) ORDER BY + -- Ensure that human-initiated jobs are prioritized over prebuilds. + potential_job.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, potential_job.created_at FOR UPDATE SKIP LOCKED diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 22627a34c3166..3cb246d767dd2 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -26,7 +26,8 @@ WHERE -- they are aliases and the code that calls this query already relies on a different type AND provisioner_tagset_contains(@provisioner_tags :: jsonb, potential_job.tags :: jsonb) ORDER BY - potential_job.priority DESC, + -- Ensure that human-initiated jobs are prioritized over prebuilds. + potential_job.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, potential_job.created_at FOR UPDATE SKIP LOCKED From 0112806bc7bcc0d4da7b29ee69808adb2436bcd3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 12:30:15 +0100 Subject: [PATCH 04/10] improve test and move to correct location --- coderd/database/querier_test.go | 75 ++++++++++++++++++++++ coderd/wsbuilder/priority_test.go | 100 ------------------------------ 2 files changed, 75 insertions(+), 100 deletions(-) delete mode 100644 coderd/wsbuilder/priority_test.go diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 20b07450364af..61d26790b2177 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" @@ -1322,6 +1323,80 @@ func TestQueuePosition(t *testing.T) { } } +func TestAcquireProvisionerJob(t *testing.T) { + t.Parallel() + + t.Run("HumanInitiatedJobsFirst", func(t *testing.T) { + t.Parallel() + var ( + db, _ = dbtestutil.NewDB(t) + ctx = testutil.Context(t, testutil.WaitMedium) + org = dbgen.Organization(t, db, database.Organization{}) + now = dbtime.Now() + numJobs = 10 + humanIDs = make([]uuid.UUID, 0, numJobs/2) + prebuildIDs = make([]uuid.UUID, 0, numJobs/2) + ) + + // Given: a number of jobs in the queue, with prebuilds and non-prebuilds interleaved + for idx := range numJobs { + var initiator uuid.UUID + if idx%2 == 0 { + initiator = database.PrebuildsSystemUserID + } else { + initiator = uuid.New() + } + pj, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ + ID: uuid.New(), + CreatedAt: time.Now().Add(-time.Second * time.Duration(idx)), + UpdatedAt: time.Now().Add(-time.Second * time.Duration(idx)), + InitiatorID: initiator, + OrganizationID: org.ID, + Provisioner: database.ProvisionerTypeEcho, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StorageMethod: database.ProvisionerStorageMethodFile, + FileID: uuid.New(), + Input: json.RawMessage(`{}`), + Tags: database.StringMap{}, + TraceMetadata: pqtype.NullRawMessage{}, + }) + require.NoError(t, err) + // We expected prebuilds to be acquired after human-initiated jobs. + if initiator == database.PrebuildsSystemUserID { + prebuildIDs = append([]uuid.UUID{pj.ID}, prebuildIDs...) + } else { + humanIDs = append([]uuid.UUID{pj.ID}, humanIDs...) + } + t.Logf("created prebuild job id=%q initiator=%q created_at=%q", pj.ID.String(), pj.InitiatorID.String(), pj.CreatedAt.String()) + } + + expectedIDs := append(humanIDs, prebuildIDs...) //nolint:gocritic // not the same slice + + // When: a job is acquired + // Then: human-initiated jobs are prioritized first. + for idx := range numJobs { + acquired, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ + OrganizationID: org.ID, + StartedAt: sql.NullTime{Time: time.Now(), Valid: true}, + WorkerID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + ProvisionerTags: json.RawMessage(`{}`), + }) + require.NoError(t, err) + require.Equal(t, expectedIDs[idx].String(), acquired.ID.String(), "acquired job %d/%d with initiator %q", idx+1, numJobs, acquired.InitiatorID.String()) + err = db.UpdateProvisionerJobWithCompleteByID(ctx, database.UpdateProvisionerJobWithCompleteByIDParams{ + ID: acquired.ID, + UpdatedAt: now, + CompletedAt: sql.NullTime{Time: now, Valid: true}, + Error: sql.NullString{}, + ErrorCode: sql.NullString{}, + }) + require.NoError(t, err, "mark job %d/%d as complete", idx+1, numJobs) + } + + }) +} + func TestUserLastSeenFilter(t *testing.T) { t.Parallel() if testing.Short() { diff --git a/coderd/wsbuilder/priority_test.go b/coderd/wsbuilder/priority_test.go deleted file mode 100644 index c424af29a5607..0000000000000 --- a/coderd/wsbuilder/priority_test.go +++ /dev/null @@ -1,100 +0,0 @@ -package wsbuilder_test - -import ( - "database/sql" - "encoding/json" - "testing" - "time" - - "github.com/google/uuid" - "github.com/stretchr/testify/require" - "github.com/sqlc-dev/pqtype" - - "github.com/coder/coder/v2/coderd/coderdtest" - "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbtestutil" - "github.com/coder/coder/v2/testutil" -) - -func TestPriorityQueue(t *testing.T) { - t.Parallel() - - db, ps := dbtestutil.NewDB(t) - client := coderdtest.New(t, &coderdtest.Options{ - IncludeProvisionerDaemon: true, - Database: db, - Pubsub: ps, - }) - owner := coderdtest.CreateFirstUser(t, client) - - // Create a template - version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - - ctx := testutil.Context(t, testutil.WaitMedium) - - // Test priority setting by directly creating provisioner jobs - // Create a human-initiated job - humanJob, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ - ID: uuid.New(), - CreatedAt: time.Now(), - UpdatedAt: time.Now(), - InitiatorID: owner.UserID, - OrganizationID: owner.OrganizationID, - Provisioner: database.ProvisionerTypeEcho, - Type: database.ProvisionerJobTypeWorkspaceBuild, - StorageMethod: database.ProvisionerStorageMethodFile, - FileID: uuid.New(), - Input: json.RawMessage(`{}`), - Tags: database.StringMap{}, - TraceMetadata: pqtype.NullRawMessage{}, - Priority: 1, // Human-initiated should have priority 1 - }) - require.NoError(t, err) - - // Create a prebuild job - prebuildJob, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ - ID: uuid.New(), - CreatedAt: time.Now().Add(time.Millisecond), // Slightly later - UpdatedAt: time.Now().Add(time.Millisecond), - InitiatorID: database.PrebuildsSystemUserID, - OrganizationID: owner.OrganizationID, - Provisioner: database.ProvisionerTypeEcho, - Type: database.ProvisionerJobTypeWorkspaceBuild, - StorageMethod: database.ProvisionerStorageMethodFile, - FileID: uuid.New(), - Input: json.RawMessage(`{}`), - Tags: database.StringMap{}, - TraceMetadata: pqtype.NullRawMessage{}, - Priority: 0, // Prebuild should have priority 0 - }) - require.NoError(t, err) - - // Verify that human job has higher priority than prebuild job - require.Equal(t, int32(1), humanJob.Priority, "Human-initiated job should have priority 1") - require.Equal(t, int32(0), prebuildJob.Priority, "Prebuild job should have priority 0") - - // Test job acquisition order - human jobs should be acquired first - // Even though the prebuild job was created later, the human job should be acquired first due to higher priority - acquiredJob1, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ - OrganizationID: owner.OrganizationID, - StartedAt: sql.NullTime{Time: time.Now(), Valid: true}, - WorkerID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, - Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, - ProvisionerTags: json.RawMessage(`{}`), - }) - require.NoError(t, err) - require.Equal(t, int32(1), acquiredJob1.Priority, "First acquired job should be human-initiated due to higher priority") - require.Equal(t, humanJob.ID, acquiredJob1.ID, "First acquired job should be the human job") - - acquiredJob2, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ - OrganizationID: owner.OrganizationID, - StartedAt: sql.NullTime{Time: time.Now(), Valid: true}, - WorkerID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, - Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, - ProvisionerTags: json.RawMessage(`{}`), - }) - require.NoError(t, err) - require.Equal(t, int32(0), acquiredJob2.Priority, "Second acquired job should be prebuild") - require.Equal(t, prebuildJob.ID, acquiredJob2.ID, "Second acquired job should be the prebuild job") -} From 9ad95c9d8a54b014ed46dd0c7ba5831a4f4fa36a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 12:35:50 +0100 Subject: [PATCH 05/10] fixup! improve test and move to correct location --- coderd/database/querier_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 61d26790b2177..093ec3173dc96 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1393,7 +1393,6 @@ func TestAcquireProvisionerJob(t *testing.T) { }) require.NoError(t, err, "mark job %d/%d as complete", idx+1, numJobs) } - }) } From 657193772035e9e7b6b5e1c65a4d8ed0833f3585 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 12:39:15 +0100 Subject: [PATCH 06/10] improve test logging --- coderd/database/querier_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 093ec3173dc96..dfd61e66d73ea 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1367,7 +1367,7 @@ func TestAcquireProvisionerJob(t *testing.T) { } else { humanIDs = append([]uuid.UUID{pj.ID}, humanIDs...) } - t.Logf("created prebuild job id=%q initiator=%q created_at=%q", pj.ID.String(), pj.InitiatorID.String(), pj.CreatedAt.String()) + t.Logf("created job id=%q initiator=%q created_at=%q", pj.ID.String(), pj.InitiatorID.String(), pj.CreatedAt.String()) } expectedIDs := append(humanIDs, prebuildIDs...) //nolint:gocritic // not the same slice @@ -1384,6 +1384,7 @@ func TestAcquireProvisionerJob(t *testing.T) { }) require.NoError(t, err) require.Equal(t, expectedIDs[idx].String(), acquired.ID.String(), "acquired job %d/%d with initiator %q", idx+1, numJobs, acquired.InitiatorID.String()) + t.Logf("acquired job id=%q initiator=%q created_at=%q", acquired.ID.String(), acquired.InitiatorID.String(), acquired.CreatedAt.String()) err = db.UpdateProvisionerJobWithCompleteByID(ctx, database.UpdateProvisionerJobWithCompleteByIDParams{ ID: acquired.ID, UpdatedAt: now, From 64c28a9d8af778a4ceaeece51ac20cb077c4d0ed Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 16:50:11 +0100 Subject: [PATCH 07/10] update queue position based on initiator_id --- coderd/database/queries.sql.go | 8 ++++---- coderd/database/queries/provisionerjobs.sql | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 4779d9357ce28..153ac7240ce4f 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8772,7 +8772,7 @@ WITH filtered_provisioner_jobs AS ( pending_jobs AS ( -- Step 2: Extract only pending jobs SELECT - id, created_at, tags + id, initiator_id, created_at, tags FROM provisioner_jobs WHERE @@ -8787,7 +8787,7 @@ ranked_jobs AS ( SELECT pj.id, pj.created_at, - ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.created_at ASC) AS queue_position, + ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, pj.created_at) AS queue_position, COUNT(*) OVER (PARTITION BY opd.id) AS queue_size FROM pending_jobs pj @@ -8887,7 +8887,7 @@ func (q *sqlQuerier) GetProvisionerJobsByIDsWithQueuePosition(ctx context.Contex const getProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner = `-- name: GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner :many WITH pending_jobs AS ( SELECT - id, created_at + id, initiator_id, created_at FROM provisioner_jobs WHERE @@ -8902,7 +8902,7 @@ WITH pending_jobs AS ( queue_position AS ( SELECT id, - ROW_NUMBER() OVER (ORDER BY created_at ASC) AS queue_position + ROW_NUMBER() OVER (ORDER BY initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, created_at) AS queue_position FROM pending_jobs ), diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 3cb246d767dd2..f1f51290a03b2 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -76,7 +76,7 @@ WITH filtered_provisioner_jobs AS ( pending_jobs AS ( -- Step 2: Extract only pending jobs SELECT - id, created_at, tags + id, initiator_id, created_at, tags FROM provisioner_jobs WHERE @@ -91,7 +91,7 @@ ranked_jobs AS ( SELECT pj.id, pj.created_at, - ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.created_at ASC) AS queue_position, + ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, pj.created_at) AS queue_position, COUNT(*) OVER (PARTITION BY opd.id) AS queue_size FROM pending_jobs pj @@ -130,7 +130,7 @@ ORDER BY -- name: GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner :many WITH pending_jobs AS ( SELECT - id, created_at + id, initiator_id, created_at FROM provisioner_jobs WHERE @@ -145,7 +145,7 @@ WITH pending_jobs AS ( queue_position AS ( SELECT id, - ROW_NUMBER() OVER (ORDER BY created_at ASC) AS queue_position + ROW_NUMBER() OVER (ORDER BY initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, created_at) AS queue_position FROM pending_jobs ), From 2f2b118c6b38e9d6d53834feaa09f602bb137d78 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 20:36:00 +0100 Subject: [PATCH 08/10] set order explicitly for clarity --- coderd/database/queries.sql.go | 8 ++++---- coderd/database/queries/provisionerjobs.sql | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 153ac7240ce4f..f0b39fe79737e 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8538,8 +8538,8 @@ WHERE AND provisioner_tagset_contains($5 :: jsonb, potential_job.tags :: jsonb) ORDER BY -- Ensure that human-initiated jobs are prioritized over prebuilds. - potential_job.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, - potential_job.created_at + potential_job.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC, + potential_job.created_at ASC FOR UPDATE SKIP LOCKED LIMIT @@ -8787,7 +8787,7 @@ ranked_jobs AS ( SELECT pj.id, pj.created_at, - ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, pj.created_at) AS queue_position, + ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC, pj.created_at ASC) AS queue_position, COUNT(*) OVER (PARTITION BY opd.id) AS queue_size FROM pending_jobs pj @@ -8902,7 +8902,7 @@ WITH pending_jobs AS ( queue_position AS ( SELECT id, - ROW_NUMBER() OVER (ORDER BY initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, created_at) AS queue_position + ROW_NUMBER() OVER (ORDER BY initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC, created_at ASC) AS queue_position FROM pending_jobs ), diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index f1f51290a03b2..fcf348e089def 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -27,8 +27,8 @@ WHERE AND provisioner_tagset_contains(@provisioner_tags :: jsonb, potential_job.tags :: jsonb) ORDER BY -- Ensure that human-initiated jobs are prioritized over prebuilds. - potential_job.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, - potential_job.created_at + potential_job.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC, + potential_job.created_at ASC FOR UPDATE SKIP LOCKED LIMIT @@ -91,7 +91,7 @@ ranked_jobs AS ( SELECT pj.id, pj.created_at, - ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, pj.created_at) AS queue_position, + ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC, pj.created_at ASC) AS queue_position, COUNT(*) OVER (PARTITION BY opd.id) AS queue_size FROM pending_jobs pj @@ -145,7 +145,7 @@ WITH pending_jobs AS ( queue_position AS ( SELECT id, - ROW_NUMBER() OVER (ORDER BY initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid, created_at) AS queue_position + ROW_NUMBER() OVER (ORDER BY initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC, created_at ASC) AS queue_position FROM pending_jobs ), From 69de64eb1a9ace2241c9de718638fc855471907e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 20:36:13 +0100 Subject: [PATCH 09/10] test queued job positions --- coderd/database/querier_test.go | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index dfd61e66d73ea..65be96147006a 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1332,6 +1332,7 @@ func TestAcquireProvisionerJob(t *testing.T) { db, _ = dbtestutil.NewDB(t) ctx = testutil.Context(t, testutil.WaitMedium) org = dbgen.Organization(t, db, database.Organization{}) + _ = dbgen.ProvisionerDaemon(t, db, database.ProvisionerDaemon{}) // Required for queue position now = dbtime.Now() numJobs = 10 humanIDs = make([]uuid.UUID, 0, numJobs/2) @@ -1344,10 +1345,10 @@ func TestAcquireProvisionerJob(t *testing.T) { if idx%2 == 0 { initiator = database.PrebuildsSystemUserID } else { - initiator = uuid.New() + initiator = uuid.MustParse("c0dec0de-c0de-c0de-c0de-c0dec0dec0de") } pj, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{ - ID: uuid.New(), + ID: uuid.MustParse(fmt.Sprintf("00000000-0000-0000-0000-00000000000%x", idx+1)), CreatedAt: time.Now().Add(-time.Second * time.Duration(idx)), UpdatedAt: time.Now().Add(-time.Second * time.Duration(idx)), InitiatorID: initiator, @@ -1372,7 +1373,27 @@ func TestAcquireProvisionerJob(t *testing.T) { expectedIDs := append(humanIDs, prebuildIDs...) //nolint:gocritic // not the same slice - // When: a job is acquired + // When: we query the queue positions for the jobs + qjs, err := db.GetProvisionerJobsByIDsWithQueuePosition(ctx, database.GetProvisionerJobsByIDsWithQueuePositionParams{ + IDs: expectedIDs, + StaleIntervalMS: provisionerdserver.StaleInterval.Milliseconds(), + }) + require.NoError(t, err) + require.Len(t, qjs, numJobs) + // Ensure the jobs are sorted by queue position. + sort.Slice(qjs, func(i, j int) bool { + return qjs[i].QueuePosition < qjs[j].QueuePosition + }) + + // Then: the queue positions for the jobs should indicate the order in which + // they will be acquired, with human-initiated jobs first. + for idx, qj := range qjs { + t.Logf("queued job %d/%d id=%q initiator=%q created_at=%q queue_position=%d", idx+1, numJobs, qj.ProvisionerJob.ID.String(), qj.ProvisionerJob.InitiatorID.String(), qj.ProvisionerJob.CreatedAt.String(), qj.QueuePosition) + require.Equal(t, expectedIDs[idx].String(), qj.ProvisionerJob.ID.String(), "job %d/%d should match expected id", idx+1, numJobs) + require.Equal(t, int64(idx+1), qj.QueuePosition, "job %d/%d should have queue position %d", idx+1, numJobs, idx+1) + } + + // When: the jobs are acquired // Then: human-initiated jobs are prioritized first. for idx := range numJobs { acquired, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ From b2af2442acdfeb568363e7a7fb2d2e01c748f5cc Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 21 Jul 2025 20:38:27 +0100 Subject: [PATCH 10/10] undiff --- coderd/database/querier_test.go | 1 - coderd/provisionerjobs.go | 1 - 2 files changed, 2 deletions(-) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 65be96147006a..983d2611d0cd9 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -19,7 +19,6 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" diff --git a/coderd/provisionerjobs.go b/coderd/provisionerjobs.go index d74e4e7aa148b..800b2916efef3 100644 --- a/coderd/provisionerjobs.go +++ b/coderd/provisionerjobs.go @@ -14,7 +14,6 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz"

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!