- Notifications
You must be signed in to change notification settings - Fork923
feat(coderd): add matched provisioner daemons information to more places#15688
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| 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 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: extracted todb2sdk.
| // 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) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: I wanted to avoid sprinklingdbauthz.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 toSystemRestricted if folks prefer.
| @@ -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"}) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: we may need to notify or log about this, but deferring for later.
| 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| 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) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
zero because includeProvisionerDaemon is not set
| 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) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
includeProvisionerDaemon is set here!
| @@ -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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: we should probably disable this linter for unit tests
| _, err = c.Client.CreateWorkspace(ctx, owner.OrganizationID, codersdk.Me, codersdk.CreateWorkspaceRequest{ | ||
| _, err = c.Client.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: noticed this was usage of a deprecated function, drive-by fix.
| 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)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: linter complained about a possible nil ptr reference in pre-existing code.
| require.NoError(t, closeDaemon.Close()) | ||
| ctx := testutil.Context(t, testutil.WaitLong) | ||
| // Given: no provisioner daemons exist. | ||
| _, err := db.ExecContext(ctx, `DELETE FROM provisioner_daemons;`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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 { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we only ever expect a single job to be returned byGetProvisionerJobsByIDsWithQueuePosition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In this instance, we should get either 0 or 1:
jobs, err := api.Database.GetProvisionerJobsByIDsWithQueuePosition(ctx, []uuid.UUID{templateVersion.JobID})
dannykoppingDec 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My point here is really that if we're only ever expecting a single job, we should consider changing the semantics of theGetProvisionerJobsByIDsWithQueuePosition to be a:one not a:many.
non-blocking suggestion, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess it'spossible though that a single template version could have multiple provisioner jobs associated if something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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>
2b57dcc intomainUh oh!
There was an error while loading.Please reload this page.
…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 commit2b57dcc)
…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 commit2b57dcc)
…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 commit2b57dcc)
Uh oh!
There was an error while loading.Please reload this page.
checkProvisionersintodb2sdk.MatchedProvisionerswsbuilderto also return matched provisioners