mirror of
https://github.com/coder/coder.git
synced 2026-09-01 14:53:15 +08:00
feat(coderd): add queue-position-free provisioner job query (#28367)
Follow-up to #28302 ([GRU-82](https://linear.app/codercom/issue/GRU-82), [RFC](https://app.notion.com/p/coderhq/Lite-Workspace-API-Requests-3aad579be5928060b5e7cc65c539717f)). That PR added related-data selection but noted that selecting `latest_build.job` still ran the expensive `GetProvisionerJobsByIDsWithQueuePosition` query, which computes queue position and size with window functions over pending jobs and provisioner daemons and is consistently the top resource consumer in scale tests. This PR adds a cheaper `GetProvisionerJobsByIDs` query that fetches jobs by ID without the queue-position computation, and selects between the two based on whether `latest_build.job.queue_position` is requested. When the queue position is not selected, `QueuePosition` and `QueueSize` are left zero and the rest of the response is unchanged. Both paths go through a single `provisionerJobsByIDs` API method that takes the `jobRelated` selection, switches on it, and reshapes the cheaper query's rows into `GetProvisionerJobsByIDsWithQueuePositionRow` so downstream conversion is uniform. The new query has the same dbauthz properties as the queue-position variant (system-level pass-through pending the proper provisioner-job RBAC check tracked in #16160), with a matching authorization test. No behavior change for existing callers: they select `queue_position` (via `allLatestBuildRelated`), so they continue to use the queue-position query. --- *Generated by Coder Agents on behalf of @spikecurtis.*
This commit is contained in:
@@ -4631,6 +4631,12 @@ func (q *querier) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uui
|
||||
return q.db.GetProvisionerJobTimingsByJobID(ctx, jobID)
|
||||
}
|
||||
|
||||
func (q *querier) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.ProvisionerJob, error) {
|
||||
// TODO: Remove this once we have a proper rbac check for provisioner jobs.
|
||||
// Details in https://github.com/coder/coder/issues/16160
|
||||
return q.db.GetProvisionerJobsByIDs(ctx, ids)
|
||||
}
|
||||
|
||||
func (q *querier) GetProvisionerJobsByIDsWithQueuePosition(ctx context.Context, ids database.GetProvisionerJobsByIDsWithQueuePositionParams) ([]database.GetProvisionerJobsByIDsWithQueuePositionRow, error) {
|
||||
// TODO: Remove this once we have a proper rbac check for provisioner jobs.
|
||||
// Details in https://github.com/coder/coder/issues/16160
|
||||
|
||||
@@ -5608,6 +5608,11 @@ func (s *MethodTestSuite) TestSystemFunctions() {
|
||||
dbm.EXPECT().GetWorkspaceAgentLogSourcesByAgentIDs(gomock.Any(), ids).Return([]database.WorkspaceAgentLogSource{}, nil).AnyTimes()
|
||||
check.Args(ids).Asserts(rbac.ResourceSystem, policy.ActionRead)
|
||||
}))
|
||||
s.Run("GetProvisionerJobsByIDs", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) {
|
||||
ids := []uuid.UUID{}
|
||||
dbm.EXPECT().GetProvisionerJobsByIDs(gomock.Any(), ids).Return([]database.ProvisionerJob{}, nil).AnyTimes()
|
||||
check.Args(ids).Asserts()
|
||||
}))
|
||||
s.Run("GetProvisionerJobsByIDsWithQueuePosition", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) {
|
||||
arg := database.GetProvisionerJobsByIDsWithQueuePositionParams{}
|
||||
dbm.EXPECT().GetProvisionerJobsByIDsWithQueuePosition(gomock.Any(), arg).Return([]database.GetProvisionerJobsByIDsWithQueuePositionRow{}, nil).AnyTimes()
|
||||
|
||||
+8
@@ -2792,6 +2792,14 @@ func (m queryMetricsStore) GetProvisionerJobTimingsByJobID(ctx context.Context,
|
||||
return r0, r1
|
||||
}
|
||||
|
||||
func (m queryMetricsStore) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.ProvisionerJob, error) {
|
||||
start := time.Now()
|
||||
r0, r1 := m.s.GetProvisionerJobsByIDs(ctx, ids)
|
||||
m.queryLatencies.WithLabelValues("GetProvisionerJobsByIDs").Observe(time.Since(start).Seconds())
|
||||
m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetProvisionerJobsByIDs").Inc()
|
||||
return r0, r1
|
||||
}
|
||||
|
||||
func (m queryMetricsStore) GetProvisionerJobsByIDsWithQueuePosition(ctx context.Context, arg database.GetProvisionerJobsByIDsWithQueuePositionParams) ([]database.GetProvisionerJobsByIDsWithQueuePositionRow, error) {
|
||||
start := time.Now()
|
||||
r0, r1 := m.s.GetProvisionerJobsByIDsWithQueuePosition(ctx, arg)
|
||||
|
||||
Generated
+15
@@ -5234,6 +5234,21 @@ func (mr *MockStoreMockRecorder) GetProvisionerJobTimingsByJobID(ctx, jobID any)
|
||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetProvisionerJobTimingsByJobID", reflect.TypeOf((*MockStore)(nil).GetProvisionerJobTimingsByJobID), ctx, jobID)
|
||||
}
|
||||
|
||||
// GetProvisionerJobsByIDs mocks base method.
|
||||
func (m *MockStore) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.ProvisionerJob, error) {
|
||||
m.ctrl.T.Helper()
|
||||
ret := m.ctrl.Call(m, "GetProvisionerJobsByIDs", ctx, ids)
|
||||
ret0, _ := ret[0].([]database.ProvisionerJob)
|
||||
ret1, _ := ret[1].(error)
|
||||
return ret0, ret1
|
||||
}
|
||||
|
||||
// GetProvisionerJobsByIDs indicates an expected call of GetProvisionerJobsByIDs.
|
||||
func (mr *MockStoreMockRecorder) GetProvisionerJobsByIDs(ctx, ids any) *gomock.Call {
|
||||
mr.mock.ctrl.T.Helper()
|
||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetProvisionerJobsByIDs", reflect.TypeOf((*MockStore)(nil).GetProvisionerJobsByIDs), ctx, ids)
|
||||
}
|
||||
|
||||
// GetProvisionerJobsByIDsWithQueuePosition mocks base method.
|
||||
func (m *MockStore) GetProvisionerJobsByIDsWithQueuePosition(ctx context.Context, arg database.GetProvisionerJobsByIDsWithQueuePositionParams) ([]database.GetProvisionerJobsByIDsWithQueuePositionRow, error) {
|
||||
m.ctrl.T.Helper()
|
||||
|
||||
Generated
+5
@@ -771,6 +771,11 @@ type sqlcQuerier interface {
|
||||
// Blocks until the row is available for update.
|
||||
GetProvisionerJobByIDWithLock(ctx context.Context, id uuid.UUID) (ProvisionerJob, error)
|
||||
GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uuid.UUID) ([]ProvisionerJobTiming, error)
|
||||
// Fetches provisioner jobs by their IDs without computing queue position or
|
||||
// queue size. Callers that do not need the queue position should prefer this
|
||||
// over GetProvisionerJobsByIDsWithQueuePosition, whose window functions over
|
||||
// pending jobs and provisioner daemons are comparatively expensive.
|
||||
GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]ProvisionerJob, error)
|
||||
GetProvisionerJobsByIDsWithQueuePosition(ctx context.Context, arg GetProvisionerJobsByIDsWithQueuePositionParams) ([]GetProvisionerJobsByIDsWithQueuePositionRow, error)
|
||||
GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner(ctx context.Context, arg GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerParams) ([]GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerRow, error)
|
||||
GetProvisionerJobsCreatedAfter(ctx context.Context, createdAt time.Time) ([]ProvisionerJob, error)
|
||||
|
||||
Generated
+60
@@ -23663,6 +23663,66 @@ func (q *sqlQuerier) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const getProvisionerJobsByIDs = `-- name: GetProvisionerJobsByIDs :many
|
||||
SELECT
|
||||
id, created_at, updated_at, started_at, canceled_at, completed_at, error, organization_id, initiator_id, provisioner, storage_method, type, input, worker_id, file_id, tags, error_code, trace_metadata, job_status, logs_length, logs_overflowed
|
||||
FROM
|
||||
provisioner_jobs
|
||||
WHERE
|
||||
id = ANY($1 :: uuid [ ])
|
||||
ORDER BY
|
||||
created_at
|
||||
`
|
||||
|
||||
// Fetches provisioner jobs by their IDs without computing queue position or
|
||||
// queue size. Callers that do not need the queue position should prefer this
|
||||
// over GetProvisionerJobsByIDsWithQueuePosition, whose window functions over
|
||||
// pending jobs and provisioner daemons are comparatively expensive.
|
||||
func (q *sqlQuerier) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]ProvisionerJob, error) {
|
||||
rows, err := q.db.QueryContext(ctx, getProvisionerJobsByIDs, pq.Array(ids))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer rows.Close()
|
||||
var items []ProvisionerJob
|
||||
for rows.Next() {
|
||||
var i ProvisionerJob
|
||||
if err := rows.Scan(
|
||||
&i.ID,
|
||||
&i.CreatedAt,
|
||||
&i.UpdatedAt,
|
||||
&i.StartedAt,
|
||||
&i.CanceledAt,
|
||||
&i.CompletedAt,
|
||||
&i.Error,
|
||||
&i.OrganizationID,
|
||||
&i.InitiatorID,
|
||||
&i.Provisioner,
|
||||
&i.StorageMethod,
|
||||
&i.Type,
|
||||
&i.Input,
|
||||
&i.WorkerID,
|
||||
&i.FileID,
|
||||
&i.Tags,
|
||||
&i.ErrorCode,
|
||||
&i.TraceMetadata,
|
||||
&i.JobStatus,
|
||||
&i.LogsLength,
|
||||
&i.LogsOverflowed,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
items = append(items, i)
|
||||
}
|
||||
if err := rows.Close(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return items, nil
|
||||
}
|
||||
|
||||
const getProvisionerJobsByIDsWithQueuePosition = `-- name: GetProvisionerJobsByIDsWithQueuePosition :many
|
||||
WITH filtered_provisioner_jobs AS (
|
||||
-- Step 1: Filter provisioner_jobs
|
||||
|
||||
@@ -67,6 +67,20 @@ WHERE
|
||||
id = $1
|
||||
FOR UPDATE;
|
||||
|
||||
-- name: GetProvisionerJobsByIDs :many
|
||||
-- Fetches provisioner jobs by their IDs without computing queue position or
|
||||
-- queue size. Callers that do not need the queue position should prefer this
|
||||
-- over GetProvisionerJobsByIDsWithQueuePosition, whose window functions over
|
||||
-- pending jobs and provisioner daemons are comparatively expensive.
|
||||
SELECT
|
||||
*
|
||||
FROM
|
||||
provisioner_jobs
|
||||
WHERE
|
||||
id = ANY(@ids :: uuid [ ])
|
||||
ORDER BY
|
||||
created_at;
|
||||
|
||||
-- name: GetProvisionerJobsByIDsWithQueuePosition :many
|
||||
WITH filtered_provisioner_jobs AS (
|
||||
-- Step 1: Filter provisioner_jobs
|
||||
|
||||
@@ -1110,6 +1110,36 @@ type workspaceBuildsData struct {
|
||||
provisionerDaemons []database.GetEligibleProvisionerDaemonsByProvisionerJobIDsRow
|
||||
}
|
||||
|
||||
// provisionerJobsByIDs fetches provisioner jobs by ID, shaped as
|
||||
// GetProvisionerJobsByIDsWithQueuePositionRow so callers can treat the result
|
||||
// uniformly. When the selection requests the queue position it uses
|
||||
// GetProvisionerJobsByIDsWithQueuePosition, whose queue position and size are
|
||||
// computed with window functions over pending jobs and provisioner daemons and
|
||||
// are comparatively expensive. Otherwise, it uses the cheaper
|
||||
// GetProvisionerJobsByIDs and leaves QueuePosition and QueueSize zero.
|
||||
func (api *API) provisionerJobsByIDs(ctx context.Context, jobIDs []uuid.UUID, cfg jobRelated) ([]database.GetProvisionerJobsByIDsWithQueuePositionRow, error) {
|
||||
if cfg.QueuePosition {
|
||||
return api.Database.GetProvisionerJobsByIDsWithQueuePosition(ctx, database.GetProvisionerJobsByIDsWithQueuePositionParams{
|
||||
IDs: jobIDs,
|
||||
StaleIntervalMS: provisionerdserver.StaleInterval.Milliseconds(),
|
||||
})
|
||||
}
|
||||
|
||||
provisionerJobs, err := api.Database.GetProvisionerJobsByIDs(ctx, jobIDs)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
jobs := make([]database.GetProvisionerJobsByIDsWithQueuePositionRow, 0, len(provisionerJobs))
|
||||
for _, job := range provisionerJobs {
|
||||
jobs = append(jobs, database.GetProvisionerJobsByIDsWithQueuePositionRow{
|
||||
ID: job.ID,
|
||||
CreatedAt: job.CreatedAt,
|
||||
ProvisionerJob: job,
|
||||
})
|
||||
}
|
||||
return jobs, nil
|
||||
}
|
||||
|
||||
func (api *API) workspaceBuildsData(ctx context.Context, workspaceBuilds []database.WorkspaceBuild, cfg latestBuildRelated) (workspaceBuildsData, error) {
|
||||
jobIDs := make([]uuid.UUID, 0, len(workspaceBuilds))
|
||||
for _, build := range workspaceBuilds {
|
||||
@@ -1122,10 +1152,7 @@ func (api *API) workspaceBuildsData(ctx context.Context, workspaceBuilds []datab
|
||||
)
|
||||
if cfg.Job != nil {
|
||||
var err error
|
||||
jobs, err = api.Database.GetProvisionerJobsByIDsWithQueuePosition(ctx, database.GetProvisionerJobsByIDsWithQueuePositionParams{
|
||||
IDs: jobIDs,
|
||||
StaleIntervalMS: provisionerdserver.StaleInterval.Milliseconds(),
|
||||
})
|
||||
jobs, err = api.provisionerJobsByIDs(ctx, jobIDs, *cfg.Job)
|
||||
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
return workspaceBuildsData{}, xerrors.Errorf("get provisioner jobs: %w", err)
|
||||
}
|
||||
|
||||
@@ -63,6 +63,12 @@ func TestWorkspaceBuildsDataQueryGating(t *testing.T) {
|
||||
app := database.WorkspaceApp{ID: uuid.New(), AgentID: agent.ID}
|
||||
|
||||
expectJob := func(db *dbmock.MockStore) {
|
||||
db.EXPECT().GetProvisionerJobsByIDs(gomock.Any(), gomock.Any()).
|
||||
Return([]database.ProvisionerJob{}, nil)
|
||||
db.EXPECT().GetEligibleProvisionerDaemonsByProvisionerJobIDs(gomock.Any(), gomock.Any()).
|
||||
Return([]database.GetEligibleProvisionerDaemonsByProvisionerJobIDsRow{}, nil)
|
||||
}
|
||||
expectJobWithQueuePosition := func(db *dbmock.MockStore) {
|
||||
db.EXPECT().GetProvisionerJobsByIDsWithQueuePosition(gomock.Any(), gomock.Any()).
|
||||
Return([]database.GetProvisionerJobsByIDsWithQueuePositionRow{}, nil)
|
||||
db.EXPECT().GetEligibleProvisionerDaemonsByProvisionerJobIDs(gomock.Any(), gomock.Any()).
|
||||
@@ -100,6 +106,11 @@ func TestWorkspaceBuildsDataQueryGating(t *testing.T) {
|
||||
cfg: latestBuildRelated{Job: &jobRelated{}},
|
||||
setup: expectJob,
|
||||
},
|
||||
{
|
||||
name: "JobWithQueuePosition",
|
||||
cfg: latestBuildRelated{Job: &jobRelated{QueuePosition: true}},
|
||||
setup: expectJobWithQueuePosition,
|
||||
},
|
||||
{
|
||||
name: "TemplateVersion",
|
||||
cfg: latestBuildRelated{TemplateVersion: true},
|
||||
@@ -171,7 +182,7 @@ func TestWorkspaceBuildsDataQueryGating(t *testing.T) {
|
||||
name: "All",
|
||||
cfg: allLatestBuildRelated(),
|
||||
setup: func(db *dbmock.MockStore) {
|
||||
expectJob(db)
|
||||
expectJobWithQueuePosition(db)
|
||||
expectTemplateVersion(db)
|
||||
expectResources(db)
|
||||
expectAgents(db)
|
||||
|
||||
Reference in New Issue
Block a user