feat(coderd): add matched provisioner daemons information to more places #15688

johnstcn · 2024-11-29T22:04:26Z

Refactors checkProvisioners into db2sdk.MatchedProvisioners
Adds a separate RBAC subject just for reading provisioner daemons for this use-case
Adds matched provisioners information to more templateversion endpoints
Updates existing unit tests for template version endpoints
Updates wsbuilder to also return matched provisioners
Updates existing unit tests for workspaces API endpoints
Updates CLI to show a warning when creating a workspace for which no provisoners are available
Adds API endpoint for matched provisioners of template dry-run job
Local smoke-testing (stop and start external provisioner daemon while testing tagged templates/workspaces)

…al template-related endpoints

johnstcn · 2024-11-29T22:05:18Z

coderd/templateversions.go

-func checkProvisioners(ctx context.Context, store database.Store, orgID uuid.UUID, wantTags map[string]string) (codersdk.MatchedProvisioners, error) {
-	// Check for eligible provisioners. This allows us to return a warning to the user if they
-	// submit a job for which no provisioner is available.
-	eligibleProvisioners, err := store.GetProvisionerDaemonsByOrganization(ctx, database.GetProvisionerDaemonsByOrganizationParams{
-		OrganizationID: orgID,
-		WantTags:       wantTags,
-	})
-	if err != nil {
-		// Log the error but do not return any warnings. This is purely advisory and we should not block.
-		return codersdk.MatchedProvisioners{}, xerrors.Errorf("provisioner daemons by organization: %w", err)
-	}
-
-	staleInterval := time.Now().Add(-provisionerdserver.StaleInterval)
-	mostRecentlySeen := codersdk.NullTime{}
-	var matched codersdk.MatchedProvisioners
-	for _, provisioner := range eligibleProvisioners {
-		if !provisioner.LastSeenAt.Valid {
-			continue
-		}
-		matched.Count++
-		if provisioner.LastSeenAt.Time.After(staleInterval) {
-			matched.Available++
-		}
-		if provisioner.LastSeenAt.Time.After(mostRecentlySeen.Time) {
-			matched.MostRecentlySeen.Valid = true
-			matched.MostRecentlySeen.Time = provisioner.LastSeenAt.Time
-		}
-	}
-	return matched, nil
-}


self-review: extracted to db2sdk.

johnstcn · 2024-11-29T22:06:33Z

coderd/database/dbauthz/dbauthz.go

+// AsSystemReadProvisionerDaemons returns a context with an actor that has permissions
+// to read provisioner daemons.
+func AsSystemReadProvisionerDaemons(ctx context.Context) context.Context {
+	return context.WithValue(ctx, authContextKey{}, subjectSystemReadProvisionerDaemons)
+}
+


self-review: I wanted to avoid sprinkling dbauthz.AsSystemRestricted everywhere, so I made a separate RBAC role for when we just wish to read provisioner daemons. I can remove this and switch back to SystemRestricted if folks prefer.

johnstcn · 2024-11-29T22:07:30Z

coderd/autobuild/lifecycle_executor.go

@@ -245,7 +245,7 @@ func (e *Executor) runOnce(t time.Time) Stats {
 							}
 						}

-						nextBuild, job, err = builder.Build(e.ctx, tx, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"})
+						nextBuild, job, _, err = builder.Build(e.ctx, tx, nil, audit.WorkspaceBuildBaggage{IP: "127.0.0.1"})


self-review: we may need to notify or log about this, but deferring for later.

johnstcn · 2024-11-29T22:08:23Z

coderd/templateversions.go

+	var matchedProvisioners *codersdk.MatchedProvisioners
+	if jobs[0].ProvisionerJob.JobStatus == database.ProvisionerJobStatusPending {
+		// nolint: gocritic // The user hitting this endpoint may not have
+		// permission to read provisioner daemons, but we want to show them
+		// information about the provisioner daemons that are available.
+		provisioners, err := api.Database.GetProvisionerDaemonsByOrganization(dbauthz.AsSystemReadProvisionerDaemons(ctx), database.GetProvisionerDaemonsByOrganizationParams{
+			OrganizationID: jobs[0].ProvisionerJob.OrganizationID,
+			WantTags:       jobs[0].ProvisionerJob.Tags,
+		})
+		if err != nil {
+			api.Logger.Error(ctx, "failed to fetch provisioners for job id", slog.F("job_id", jobs[0].ProvisionerJob.ID), slog.Error(err))
+		} else {
+			matchedProvisioners = ptr.Ref(db2sdk.MatchedProvisioners(provisioners, dbtime.Now(), provisionerdserver.StaleInterval))
+		}
+	}
+
+	httpapi.Write(ctx, rw, http.StatusOK, convertTemplateVersion(templateVersion, convertProvisionerJob(jobs[0]), matchedProvisioners, nil))


self-review: This addresses an issue I noticed in the frontend where it quickly "flashes" with the tag warning message but then gets overwritten when the FE re-requests the template version. Adding it to other template-related endpoints for posterity.

johnstcn · 2024-11-29T22:09:06Z

coderd/templateversions_test.go

+		if assert.Equal(t, tv.Job.Status, codersdk.ProvisionerJobPending) {
+			assert.NotNil(t, tv.MatchedProvisioners)
+			assert.Zero(t, tv.MatchedProvisioners.Available)
+			assert.Zero(t, tv.MatchedProvisioners.Count)
+			assert.False(t, tv.MatchedProvisioners.MostRecentlySeen.Valid)
+		}


zero because includeProvisionerDaemon is not set

johnstcn · 2024-11-29T22:09:20Z

coderd/templateversions_test.go

+		if assert.Equal(t, version.Job.Status, codersdk.ProvisionerJobPending) {
+			assert.NotNil(t, version.MatchedProvisioners)
+			assert.Equal(t, version.MatchedProvisioners.Available, 1)
+			assert.Equal(t, version.MatchedProvisioners.Count, 1)
+			assert.True(t, version.MatchedProvisioners.MostRecentlySeen.Valid)
+		}


includeProvisionerDaemon is set here!

johnstcn · 2024-11-29T22:10:04Z

coderd/wsbuilder/wsbuilder_test.go

@@ -94,7 +95,8 @@ func TestBuilder_NoOptions(t *testing.T) {

 	ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID}
 	uut := wsbuilder.New(ws, database.WorkspaceTransitionStart)
-	_, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{})
+	// nolint: dogsled


self-review: we should probably disable this linter for unit tests

johnstcn · 2024-12-01T16:58:38Z

enterprise/coderd/workspacebuilds_test.go

-				_, err = c.Client.CreateWorkspace(ctx, owner.OrganizationID, codersdk.Me, codersdk.CreateWorkspaceRequest{
+				_, err = c.Client.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{


self-review: noticed this was usage of a deprecated function, drive-by fix.

johnstcn · 2024-12-01T17:06:06Z

coderd/workspacebuilds.go

-	err = provisionerjobs.PostJob(api.Pubsub, *provisionerJob)
-	if err != nil {
-		// Client probably doesn't care about this error, so just log it.
-		api.Logger.Error(ctx, "failed to post provisioner job to pubsub", slog.Error(err))
+
+	if provisionerJob != nil {
+		if err := provisionerjobs.PostJob(api.Pubsub, *provisionerJob); err != nil {
+			// Client probably doesn't care about this error, so just log it.
+			api.Logger.Error(ctx, "failed to post provisioner job to pubsub", slog.Error(err))
+		}


self-review: linter complained about a possible nil ptr reference in pre-existing code.

johnstcn · 2024-12-01T17:07:17Z

coderd/workspacebuilds_test.go

+		require.NoError(t, closeDaemon.Close())
+		ctx := testutil.Context(t, testutil.WaitLong)
+		// Given: no provisioner daemons exist.
+		_, err := db.ExecContext(ctx, `DELETE FROM provisioner_daemons;`)


self-review: this is kinda yucky but I don't agree with the idea of creating database queries for things that will only ever get used intests.

…of dbauthz.AsSystemRestricted

dannykopping

coderd/database/dbauthz/dbauthz.go

dannykopping · 2024-12-02T09:13:26Z

coderd/templateversions.go

@@ -60,6 +62,22 @@ func (api *API) templateVersion(rw http.ResponseWriter, r *http.Request) {
 		return
 	}

+	var matchedProvisioners *codersdk.MatchedProvisioners
+	if jobs[0].ProvisionerJob.JobStatus == database.ProvisionerJobStatusPending {


Do we only ever expect a single job to be returned by GetProvisionerJobsByIDsWithQueuePosition?

In this instance, we should get either 0 or 1:

jobs, err := api.Database.GetProvisionerJobsByIDsWithQueuePosition(ctx, []uuid.UUID{templateVersion.JobID})

My point here is really that if we're only ever expecting a single job, we should consider changing the semantics of the GetProvisionerJobsByIDsWithQueuePosition to be a :one not a :many.
non-blocking suggestion, of course.

I guess it's possible though that a single template version could have multiple provisioner jobs associated if something went wrong.

I think we use the :many version of this query in a few places, and not just the :one.

Co-authored-by: Danny Kopping <danny@coder.com>

…on dry run, hook up to cli

…ces (#15688) - Refactors `checkProvisioners` into `db2sdk.MatchedProvisioners` - Adds a separate RBAC subject just for reading provisioner daemons - Adds matched provisioners information to additional endpoints relating to workspace builds and templates -Updates existing unit tests for above endpoints -Adds API endpoint for matched provisioners of template dry-run job -Updates CLI to show warning when creating/starting/stopping/deleting workspaces for which no provisoners are available --------- Co-authored-by: Danny Kopping <danny@coder.com> (cherry picked from commit 2b57dcc)

johnstcn added 2 commits November 29, 2024 21:33

feat(coderd/wsbuilder): return provisioners available at time of insert

ba944ab

feat(coderd): add matched provisioner daemons information to addition…

4e51f20

…al template-related endpoints

github-actions bot assigned johnstcn Nov 29, 2024

johnstcn commented Nov 29, 2024

View reviewed changes

johnstcn changed the title ~~Cj/more matching provisioners~~ feat(coderd): add matched provisioner daemons information to more places Nov 29, 2024

johnstcn requested a review from SasSwart November 29, 2024 22:14

johnstcn added 4 commits December 1, 2024 14:42

fix linter complaint re nil ptr deref

47036e8

add test coverage for matched provisioners with workspace build creation

16be03b

skip for non-postgres

4304a06

add tests for workspace creation

9ef68dd

johnstcn commented Dec 1, 2024

View reviewed changes

revert fe changes in this pr

38788d5

johnstcn commented Dec 1, 2024

View reviewed changes

johnstcn added 4 commits December 1, 2024 17:08

coderd/wsbuilder: use dbauthz.AsSystemReadProvisionerDaemons instead …

1c95ffe

…of dbauthz.AsSystemRestricted

refactor: extract WarnMatchedProvisioners to cliutil

98521be

fixup! refactor: extract WarnMatchedProvisioners to cliutil

e1423f5

feat(cli): add provisioner warning to create/start/stop commands

517a505

johnstcn marked this pull request as ready for review December 2, 2024 08:59

johnstcn requested a review from dannykopping December 2, 2024 09:00

dannykopping approved these changes Dec 2, 2024

View reviewed changes

Apply suggestions from code review

c4295ef

Co-authored-by: Danny Kopping <danny@coder.com>

johnstcn requested a review from DanielleMaywood December 2, 2024 09:50

johnstcn added 4 commits December 2, 2024 16:49

feat(coderd): add endpoint for matched provisioners of template versi…

c5fb83b

…on dry run, hook up to cli

feat(cli): delete: warn on no matched provisioners

3bd62fd

address linter complaint

2f625bc

add test for cli/delete

848338e

johnstcn mentioned this pull request Dec 2, 2024

Improve UX when there is no corresponding provisioner for a workspace/template #15048

Closed

johnstcn merged commit 2b57dcc into main Dec 2, 2024
31 checks passed

johnstcn deleted the cj/more-matching-provisioners branch December 2, 2024 20:54

johnstcn added a commit that referenced this pull request Dec 2, 2024

chore(cli): fix test flake introduced by #15688

8f46268

johnstcn added a commit that referenced this pull request Dec 2, 2024

chore(cli): fix test flake introduced by #15688 (#15724)

0c541c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(coderd): add matched provisioner daemons information to more places #15688

feat(coderd): add matched provisioner daemons information to more places #15688

johnstcn commented Nov 29, 2024 •

edited

Loading

johnstcn Nov 29, 2024

johnstcn Nov 29, 2024

johnstcn Nov 29, 2024

johnstcn Nov 29, 2024

johnstcn Nov 29, 2024

johnstcn Nov 29, 2024

johnstcn Nov 29, 2024

johnstcn Dec 1, 2024

johnstcn Dec 1, 2024 •

edited

Loading

johnstcn Dec 1, 2024

dannykopping left a comment

dannykopping Dec 2, 2024

johnstcn Dec 2, 2024

dannykopping Dec 2, 2024 •

edited

Loading

dannykopping Dec 2, 2024

johnstcn Dec 2, 2024

		_, err = c.Client.CreateWorkspace(ctx, owner.OrganizationID, codersdk.Me, codersdk.CreateWorkspaceRequest{
		_, err = c.Client.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{

feat(coderd): add matched provisioner daemons information to more places #15688

feat(coderd): add matched provisioner daemons information to more places #15688

Conversation

johnstcn commented Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn commented Nov 29, 2024 •

edited

Loading

johnstcn Dec 1, 2024 •

edited

Loading

dannykopping Dec 2, 2024 •

edited

Loading