Skip to content

feat: add provisioner job metadata #16454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 6, 2025
Merged

feat: add provisioner job metadata #16454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4228a6a
feat: add provisioner job metadata
mafredri Feb 5, 2025
696819f
hi there dbmeme
mafredri Feb 5, 2025
0ff1339
move
mafredri Feb 5, 2025
2b5afee
safety rails
mafredri Feb 5, 2025
316b0a8
add tpl import job check
mafredri Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/testdata/coder_provisioner_jobs_list_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ OPTIONS:
-O, --org string, $CODER_ORGANIZATION
Select which organization (uuid or name) to use.

-c, --column [id|created at|started at|completed at|canceled at|error|error code|status|worker id|file id|tags|queue position|queue size|organization id|template version id|workspace build id|type|available workers|organization|queue] (default: created at,id,organization,status,type,queue,tags)
-c, --column [id|created at|started at|completed at|canceled at|error|error code|status|worker id|file id|tags|queue position|queue size|organization id|template version id|workspace build id|type|available workers|template version name|template id|template name|template display name|workspace id|workspace name|organization|queue] (default: created at,id,organization,status,type,queue,tags)
Columns to display in table output.

-l, --limit int, $CODER_PROVISIONER_JOB_LIST_LIMIT (default: 50)
Expand Down
14 changes: 14 additions & 0 deletions cli/testdata/coder_provisioner_jobs_list_--output_json.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
"template_version_id": "============[version ID]============"
},
"type": "template_version_import",
"metadata": {
"template_version_name": "===========[version name]===========",
"template_id": "===========[template ID]============",
"template_name": "test-template",
"template_display_name": ""
},
"organization_name": "Coder"
},
{
Expand All @@ -39,6 +45,14 @@
"workspace_build_id": "========[workspace build ID]========"
},
"type": "workspace_build",
"metadata": {
"template_version_name": "===========[version name]===========",
"template_id": "===========[template ID]============",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which party will consume the metadata later? I mean, is it human or machine?

If human, perhaps we can remove template_id and workspace_id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either machine or human. For now, this is needed in the UI to show and link relevant workspace/template. I'll defer to @BrunoQuaresma as to what's needed, but I don't see any harm in keeping the IDs in? If I was an admin debugging provisioner jobs, I'd imagine it could come in handy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree with leaving this information available in the JSON output. This is my general experience with other CLI tooling as well (e.g. kubectl or docker) -- if you need all the info, dump the JSON or YAML and read it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I raised this comment is my concern about the response size. IMHO we could always add these properties later, but if you find these could be useful for debugging then leave them as is. I understand that admins don't use them for troubleshooting but rather default to template/template version/workspace names, and IDs seem to be internal references.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response size is a valid concern @mtojek, especially considering we didn't limit the response of this endpoint previously. In the regular response size (say 50 entries), these UUIDs are quite harmless and the endpoint isn't going to be very actively used. I haven't personally needed to debug provisioner jobs so I went for more rather than less, but happy to go either way here (assuming @BrunoQuaresma doesn't need the IDs).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW @BrunoQuaresma is out today, and since it is not a huge issue I don't have anything against merging it as is 👍

"template_name": "test-template",
"template_display_name": "",
"workspace_id": "===========[workspace ID]===========",
"workspace_name": "test-workspace"
},
"organization_name": "Coder"
}
]
28 changes: 28 additions & 0 deletions coderd/apidoc/docs.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions coderd/apidoc/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 39 additions & 0 deletions coderd/database/dbmem/dbmem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4117,6 +4117,45 @@ func (q *FakeQuerier) GetProvisionerJobsByOrganizationAndStatusWithQueuePosition
QueuePosition: rowQP.QueuePosition,
QueueSize: rowQP.QueueSize,
}

// Start add metadata.
var input codersdk.ProvisionerJobInput
err := json.Unmarshal([]byte(job.Input), &input)
if err != nil {
return nil, err
}
templateVersionID := input.TemplateVersionID
if input.WorkspaceBuildID != nil {
workspaceBuild, err := q.getWorkspaceBuildByIDNoLock(ctx, *input.WorkspaceBuildID)
if err != nil {
return nil, err
}
workspace, err := q.getWorkspaceByIDNoLock(ctx, workspaceBuild.WorkspaceID)
if err != nil {
return nil, err
}
row.WorkspaceID = uuid.NullUUID{UUID: workspace.ID, Valid: true}
row.WorkspaceName = workspace.Name
if templateVersionID == nil {
templateVersionID = &workspaceBuild.TemplateVersionID
}
}
if templateVersionID != nil {
templateVersion, err := q.getTemplateVersionByIDNoLock(ctx, *templateVersionID)
if err != nil {
return nil, err
}
row.TemplateVersionName = templateVersion.Name
template, err := q.getTemplateByIDNoLock(ctx, templateVersion.TemplateID.UUID)
if err != nil {
return nil, err
}
row.TemplateID = uuid.NullUUID{UUID: template.ID, Valid: true}
row.TemplateName = template.Name
row.TemplateDisplayName = template.DisplayName
}
// End add metadata.

if row.QueuePosition > 0 {
var availableWorkers []database.ProvisionerDaemon
for _, daemon := range q.provisionerDaemons {
Expand Down
46 changes: 40 additions & 6 deletions coderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 24 additions & 2 deletions coderd/database/queries/provisionerjobs.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,21 +130,43 @@ SELECT
AND pj.organization_id = pd.organization_id
AND pj.provisioner = ANY(pd.provisioners)
AND provisioner_tagset_contains(pd.tags, pj.tags)
) AS available_workers
) AS available_workers,
-- Include template and workspace information.
COALESCE(tv.name, '') AS template_version_name,
t.id AS template_id,
COALESCE(t.name, '') AS template_name,
COALESCE(t.display_name, '') AS template_display_name,
w.id AS workspace_id,
COALESCE(w.name, '') AS workspace_name
FROM
provisioner_jobs pj
LEFT JOIN
queue_position qp ON qp.id = pj.id
LEFT JOIN
queue_size qs ON TRUE
LEFT JOIN
workspace_builds wb ON wb.id = CASE WHEN pj.input ? 'workspace_build_id' THEN (pj.input->>'workspace_build_id')::uuid END
LEFT JOIN
workspaces w ON wb.workspace_id = w.id
Comment on lines +149 to +150
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that wb.workspace_id will be NULL here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if the job is a template version import.

LEFT JOIN
-- We should always have a template version, either explicitly or implicitly via workspace build.
template_versions tv ON tv.id = CASE WHEN pj.input ? 'template_version_id' THEN (pj.input->>'template_version_id')::uuid ELSE wb.template_version_id END
Comment on lines +152 to +153
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the issue here that either input.template_version_id or wb.workspace_build_id can be null, so we're trying to account for both possibilities?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite. Both could be null in the future, all we need to do is add a new provisioner job type/input.

What we do get currently though, assuming correct input, is that there always exists a template version either directly (id) or indirectly (workspace build). This makes sure we join the template version (and thus, template) in both cases.

And in the case that neither exists in input, the template and template version info will be null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. It might be good to add some querier tests for these eventualities just to make sure we're not surprised in the future.

LEFT JOIN
templates t ON tv.template_id = t.id
WHERE
(sqlc.narg('organization_id')::uuid IS NULL OR pj.organization_id = @organization_id)
AND (COALESCE(array_length(@ids::uuid[], 1), 0) = 0 OR pj.id = ANY(@ids::uuid[]))
AND (COALESCE(array_length(@status::provisioner_job_status[], 1), 0) = 0 OR pj.job_status = ANY(@status::provisioner_job_status[]))
GROUP BY
pj.id,
qp.queue_position,
qs.count
qs.count,
tv.name,
t.id,
t.name,
t.display_name,
w.id,
w.name
ORDER BY
pj.created_at DESC
LIMIT
Expand Down
10 changes: 10 additions & 0 deletions coderd/provisionerjobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,16 @@ func convertProvisionerJobWithQueuePosition(pj database.GetProvisionerJobsByOrga
QueueSize: pj.QueueSize,
})
job.AvailableWorkers = pj.AvailableWorkers
job.Metadata = &codersdk.ProvisionerJobMetadata{
TemplateVersionName: pj.TemplateVersionName,
TemplateID: pj.TemplateID.UUID,
TemplateName: pj.TemplateName,
TemplateDisplayName: pj.TemplateDisplayName,
WorkspaceName: pj.WorkspaceName,
}
if pj.WorkspaceID.Valid {
job.Metadata.WorkspaceID = &pj.WorkspaceID.UUID
}
return job
}

Expand Down
45 changes: 39 additions & 6 deletions coderd/provisionerjobs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/coder/coder/v2/coderd/coderdtest"
Expand Down Expand Up @@ -72,13 +73,45 @@ func TestProvisionerJobs(t *testing.T) {

t.Run("Single", func(t *testing.T) {
t.Parallel()
t.Run("OK", func(t *testing.T) {
t.Run("Workspace", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitMedium)
// Note this calls the single job endpoint.
job2, err := templateAdminClient.OrganizationProvisionerJob(ctx, owner.OrganizationID, job.ID)
require.NoError(t, err)
require.Equal(t, job.ID, job2.ID)
t.Run("OK", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitMedium)
// Note this calls the single job endpoint.
job2, err := templateAdminClient.OrganizationProvisionerJob(ctx, owner.OrganizationID, job.ID)
require.NoError(t, err)
require.Equal(t, job.ID, job2.ID)

// Verify that job metadata is correct.
assert.Equal(t, job2.Metadata, &codersdk.ProvisionerJobMetadata{
TemplateVersionName: version.Name,
TemplateID: template.ID,
TemplateName: template.Name,
TemplateDisplayName: template.DisplayName,
WorkspaceID: &w.ID,
WorkspaceName: w.Name,
})
})
})
t.Run("Template Import", func(t *testing.T) {
t.Parallel()
t.Run("OK", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitMedium)
// Note this calls the single job endpoint.
job2, err := templateAdminClient.OrganizationProvisionerJob(ctx, owner.OrganizationID, version.Job.ID)
require.NoError(t, err)
require.Equal(t, version.Job.ID, job2.ID)

// Verify that job metadata is correct.
assert.Equal(t, job2.Metadata, &codersdk.ProvisionerJobMetadata{
TemplateVersionName: version.Name,
TemplateID: template.ID,
TemplateName: template.Name,
TemplateDisplayName: template.DisplayName,
})
})
})
t.Run("Missing", func(t *testing.T) {
t.Parallel()
Expand Down
Loading
Loading