Skip to content

Commit c4b69bb

Browse files
johnstcnblink-so[bot]kylecarbs
authored
fix: prioritise human-initiated builds over prebuilds (#18933)
Continues from #18882 - Reverts extraneous changes - Adds explicit `ORDER BY initiator_id = $PREBUILDS_USER_ID` to `AcquireProvisionerJob` - Improves test added for above PR --------- Co-authored-by: blink-so[bot] <211532188+blink-so[bot]@users.noreply.github.com> Co-authored-by: kylecarbs <7122116+kylecarbs@users.noreply.github.com>
1 parent e98dce7 commit c4b69bb

File tree

3 files changed

+109
-10
lines changed

3 files changed

+109
-10
lines changed

‎coderd/database/querier_test.go

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1322,6 +1322,101 @@ func TestQueuePosition(t *testing.T) {
13221322
}
13231323
}
13241324

1325+
func TestAcquireProvisionerJob(t *testing.T) {
1326+
t.Parallel()
1327+
1328+
t.Run("HumanInitiatedJobsFirst", func(t *testing.T) {
1329+
t.Parallel()
1330+
var (
1331+
db, _ = dbtestutil.NewDB(t)
1332+
ctx = testutil.Context(t, testutil.WaitMedium)
1333+
org = dbgen.Organization(t, db, database.Organization{})
1334+
_ = dbgen.ProvisionerDaemon(t, db, database.ProvisionerDaemon{}) // Required for queue position
1335+
now = dbtime.Now()
1336+
numJobs = 10
1337+
humanIDs = make([]uuid.UUID, 0, numJobs/2)
1338+
prebuildIDs = make([]uuid.UUID, 0, numJobs/2)
1339+
)
1340+
1341+
// Given: a number of jobs in the queue, with prebuilds and non-prebuilds interleaved
1342+
for idx := range numJobs {
1343+
var initiator uuid.UUID
1344+
if idx%2 == 0 {
1345+
initiator = database.PrebuildsSystemUserID
1346+
} else {
1347+
initiator = uuid.MustParse("c0dec0de-c0de-c0de-c0de-c0dec0dec0de")
1348+
}
1349+
pj, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{
1350+
ID: uuid.MustParse(fmt.Sprintf("00000000-0000-0000-0000-00000000000%x", idx+1)),
1351+
CreatedAt: time.Now().Add(-time.Second * time.Duration(idx)),
1352+
UpdatedAt: time.Now().Add(-time.Second * time.Duration(idx)),
1353+
InitiatorID: initiator,
1354+
OrganizationID: org.ID,
1355+
Provisioner: database.ProvisionerTypeEcho,
1356+
Type: database.ProvisionerJobTypeWorkspaceBuild,
1357+
StorageMethod: database.ProvisionerStorageMethodFile,
1358+
FileID: uuid.New(),
1359+
Input: json.RawMessage(`{}`),
1360+
Tags: database.StringMap{},
1361+
TraceMetadata: pqtype.NullRawMessage{},
1362+
})
1363+
require.NoError(t, err)
1364+
// We expected prebuilds to be acquired after human-initiated jobs.
1365+
if initiator == database.PrebuildsSystemUserID {
1366+
prebuildIDs = append([]uuid.UUID{pj.ID}, prebuildIDs...)
1367+
} else {
1368+
humanIDs = append([]uuid.UUID{pj.ID}, humanIDs...)
1369+
}
1370+
t.Logf("created job id=%q initiator=%q created_at=%q", pj.ID.String(), pj.InitiatorID.String(), pj.CreatedAt.String())
1371+
}
1372+
1373+
expectedIDs := append(humanIDs, prebuildIDs...) //nolint:gocritic // not the same slice
1374+
1375+
// When: we query the queue positions for the jobs
1376+
qjs, err := db.GetProvisionerJobsByIDsWithQueuePosition(ctx, database.GetProvisionerJobsByIDsWithQueuePositionParams{
1377+
IDs: expectedIDs,
1378+
StaleIntervalMS: provisionerdserver.StaleInterval.Milliseconds(),
1379+
})
1380+
require.NoError(t, err)
1381+
require.Len(t, qjs, numJobs)
1382+
// Ensure the jobs are sorted by queue position.
1383+
sort.Slice(qjs, func(i, j int) bool {
1384+
return qjs[i].QueuePosition < qjs[j].QueuePosition
1385+
})
1386+
1387+
// Then: the queue positions for the jobs should indicate the order in which
1388+
// they will be acquired, with human-initiated jobs first.
1389+
for idx, qj := range qjs {
1390+
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)
1391+
require.Equal(t, expectedIDs[idx].String(), qj.ProvisionerJob.ID.String(), "job %d/%d should match expected id", idx+1, numJobs)
1392+
require.Equal(t, int64(idx+1), qj.QueuePosition, "job %d/%d should have queue position %d", idx+1, numJobs, idx+1)
1393+
}
1394+
1395+
// When: the jobs are acquired
1396+
// Then: human-initiated jobs are prioritized first.
1397+
for idx := range numJobs {
1398+
acquired, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{
1399+
OrganizationID: org.ID,
1400+
StartedAt: sql.NullTime{Time: time.Now(), Valid: true},
1401+
WorkerID: uuid.NullUUID{UUID: uuid.New(), Valid: true},
1402+
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
1403+
ProvisionerTags: json.RawMessage(`{}`),
1404+
})
1405+
require.NoError(t, err)
1406+
require.Equal(t, expectedIDs[idx].String(), acquired.ID.String(), "acquired job %d/%d with initiator %q", idx+1, numJobs, acquired.InitiatorID.String())
1407+
t.Logf("acquired job id=%q initiator=%q created_at=%q", acquired.ID.String(), acquired.InitiatorID.String(), acquired.CreatedAt.String())
1408+
err = db.UpdateProvisionerJobWithCompleteByID(ctx, database.UpdateProvisionerJobWithCompleteByIDParams{
1409+
ID: acquired.ID,
1410+
UpdatedAt: now,
1411+
CompletedAt: sql.NullTime{Time: now, Valid: true},
1412+
Error: sql.NullString{},
1413+
ErrorCode: sql.NullString{},
1414+
})
1415+
require.NoError(t, err, "mark job %d/%d as complete", idx+1, numJobs)
1416+
}
1417+
})
1418+
}
1419+
13251420
func TestUserLastSeenFilter(t *testing.T) {
13261421
t.Parallel()
13271422
if testing.Short() {

‎coderd/database/queries.sql.go

Lines changed: 7 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/provisionerjobs.sql

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ WHERE
2626
-- they are aliases and the code that calls this query already relies on a different type
2727
AND provisioner_tagset_contains(@provisioner_tags :: jsonb, potential_job.tags :: jsonb)
2828
ORDER BY
29-
potential_job.created_at
29+
-- Ensure that human-initiated jobs are prioritized over prebuilds.
30+
potential_job.initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC,
31+
potential_job.created_at ASC
3032
FOR UPDATE
3133
SKIP LOCKED
3234
LIMIT
@@ -74,7 +76,7 @@ WITH filtered_provisioner_jobs AS (
7476
pending_jobs AS (
7577
-- Step 2: Extract only pending jobs
7678
SELECT
77-
id, created_at, tags
79+
id, initiator_id, created_at, tags
7880
FROM
7981
provisioner_jobs
8082
WHERE
@@ -89,7 +91,7 @@ ranked_jobs AS (
8991
SELECT
9092
pj.id,
9193
pj.created_at,
92-
ROW_NUMBER() OVER (PARTITION BY opd.id ORDER BY pj.created_at ASC) AS queue_position,
94+
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,
9395
COUNT(*) OVER (PARTITION BY opd.id) AS queue_size
9496
FROM
9597
pending_jobs pj
@@ -128,7 +130,7 @@ ORDER BY
128130
-- name: GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner :many
129131
WITH pending_jobs AS (
130132
SELECT
131-
id, created_at
133+
id, initiator_id, created_at
132134
FROM
133135
provisioner_jobs
134136
WHERE
@@ -143,7 +145,7 @@ WITH pending_jobs AS (
143145
queue_position AS (
144146
SELECT
145147
id,
146-
ROW_NUMBER() OVER (ORDER BY created_at ASC) AS queue_position
148+
ROW_NUMBER() OVER (ORDER BY initiator_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid ASC, created_at ASC) AS queue_position
147149
FROM
148150
pending_jobs
149151
),

0 commit comments

Comments
 (0)