Skip to content

feat: cancel stuck pending jobs #17803

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 24 commits into from
May 20, 2025
+773 −291
Merged

feat: cancel stuck pending jobs #17803

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0f51f35
added queries for fetching NotStartedProvisionerJobs
ibetitsmike Apr 1, 2025
2f3d606
added detector handling of not started jobs
ibetitsmike May 9, 2025
4b252eb
filling out started_at when unhanging not started jobs
ibetitsmike May 9, 2025
ca49519
WIP
ibetitsmike May 13, 2025
af994c2
refactored to reaper & added tests
ibetitsmike May 13, 2025
3815727
Revert "filling out started_at when unhanging not started jobs"
ibetitsmike May 13, 2025
b65f620
created new ORM update to avoid forcing setting StartedAt on every Co…
ibetitsmike May 13, 2025
3c7c323
added missing dbauthz tests
ibetitsmike May 13, 2025
35df01f
added checks for StartedAt value in the updated jobs
ibetitsmike May 13, 2025
8aa1ee2
refactor from reaper to jobreaper
ibetitsmike May 14, 2025
4385933
WIP
ibetitsmike May 14, 2025
96fee51
WIP
ibetitsmike May 14, 2025
d8db119
WIP
ibetitsmike May 15, 2025
5120fb1
WIP
ibetitsmike May 15, 2025
8d4fa5a
fixed sql comments
ibetitsmike May 15, 2025
18b809c
taking a step back with RBAC
ibetitsmike May 16, 2025
0fe1404
WIP
ibetitsmike May 16, 2025
77be34e
WIP
ibetitsmike May 16, 2025
4351529
WIP
ibetitsmike May 16, 2025
c03bfa3
fixed InOrg check for provisionerjob resource
ibetitsmike May 19, 2025
a15bd1c
PR review; naming in the comments, added comments for SQL, less verbo…
ibetitsmike May 19, 2025
5b9348f
fixes to tests after lint remove rand
ibetitsmike May 19, 2025
91d2d32
readded rand to fix gen failing in CI
ibetitsmike May 19, 2025
767cb77
adjusted TODOs
ibetitsmike May 20, 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
Prev Previous commit
Next Next commit
taking a step back with RBAC
  • Loading branch information
@ibetitsmike
ibetitsmike committed May 16, 2025
commit 18b809c6438850ad0d808074b6d0f2098d7bee0c
77 changes: 41 additions & 36 deletions coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,9 +1088,9 @@ func (q *querier) AcquireNotificationMessages(ctx context.Context, arg database.
}

func (q *querier) AcquireProvisionerJob(ctx context.Context, arg database.AcquireProvisionerJobParams) (database.ProvisionerJob, error) {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
return database.ProvisionerJob{}, err
}
// if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
// return database.ProvisionerJob{}, err
// }
return q.db.AcquireProvisionerJob(ctx, arg)
}

Expand Down Expand Up @@ -2309,30 +2309,31 @@ func (q *querier) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uui
}

func (q *querier) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]database.ProvisionerJob, error) {
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
return nil, err
}
// if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
// return nil, err
// }
return q.db.GetProvisionerJobsByIDs(ctx, ids)
}

func (q *querier) GetProvisionerJobsByIDsWithQueuePosition(ctx context.Context, ids []uuid.UUID) ([]database.GetProvisionerJobsByIDsWithQueuePositionRow, error) {
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
return nil, err
}
// if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
// return nil, err
// }
// policy.ActionRead, rbac.ResourceProvisionerJobs.InOrg(org.ID)
return q.db.GetProvisionerJobsByIDsWithQueuePosition(ctx, ids)
}

func (q *querier) GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner(ctx context.Context, arg database.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerParams) ([]database.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisionerRow, error) {
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
return nil, err
}
// if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
// return nil, err
// }
return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner)(ctx, arg)
}

func (q *querier) GetProvisionerJobsCreatedAfter(ctx context.Context, createdAt time.Time) ([]database.ProvisionerJob, error) {
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
return nil, err
}
// if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceProvisionerJobs); err != nil {
// return nil, err
// }
return q.db.GetProvisionerJobsCreatedAfter(ctx, createdAt)
}

Expand Down Expand Up @@ -3528,23 +3529,27 @@ func (q *querier) InsertPresetParameters(ctx context.Context, arg database.Inser
}

func (q *querier) InsertProvisionerJob(ctx context.Context, arg database.InsertProvisionerJobParams) (database.ProvisionerJob, error) {
if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceProvisionerJobs); err != nil {
return database.ProvisionerJob{}, err
}
// TODO: Remove this once we have a proper rbac check for provisioner jobs.
// Currently ProvisionerJobs are not associated with a user, so we can't
// check for a user's permissions. We'd need to check for the associated workspace
// and verify ownership through that.
Copy link
Member

Choose a reason for hiding this comment

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

The comment should probably mention all the ways a job could be made and what the permission check should look like. I think the following is correct:

  • template_version_import jobs require either template_version:create on the org (if creating a new template) or template:update and template_version:create on the template (if pushing a new version)
  • template_version_dry_run jobs require permissions to workspace:create in the org as well as template_version:read on the specific template version
  • workspace_build jobs require workspace:update on the specific workspace as well as template_version:read on the specific template version

I just don't want the comment to only say that workspace ownership needs to be checked, and for some poor soul to start working on this thinking it'll be simple when in reality it will probably be difficult.

Likely should have a ticket created to track this too. You might want to add a comment here in the code with the ticket number after you create it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left a simple description with link to the issue to not duplicate information.

// if err := q.authorizeContext(ctx, policy.ActionCreate, rbac.ResourceProvisionerJobs); err != nil {
// return database.ProvisionerJob{}, err
// }
return q.db.InsertProvisionerJob(ctx, arg)
}

func (q *querier) InsertProvisionerJobLogs(ctx context.Context, arg database.InsertProvisionerJobLogsParams) ([]database.ProvisionerJobLog, error) {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
return nil, err
}
// if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
// return nil, err
// }
return q.db.InsertProvisionerJobLogs(ctx, arg)
}

func (q *querier) InsertProvisionerJobTimings(ctx context.Context, arg database.InsertProvisionerJobTimingsParams) ([]database.ProvisionerJobTiming, error) {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
return nil, err
}
// if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
// return nil, err
// }
return q.db.InsertProvisionerJobTimings(ctx, arg)
}

Expand Down Expand Up @@ -4168,16 +4173,16 @@ func (q *querier) UpdateProvisionerDaemonLastSeenAt(ctx context.Context, arg dat
}

func (q *querier) UpdateProvisionerJobByID(ctx context.Context, arg database.UpdateProvisionerJobByIDParams) error {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
return err
}
// if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
// return err
// }
return q.db.UpdateProvisionerJobByID(ctx, arg)
}

func (q *querier) UpdateProvisionerJobWithCancelByID(ctx context.Context, arg database.UpdateProvisionerJobWithCancelByIDParams) error {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
return err
}
// if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
// return err
// }

job, err := q.db.GetProvisionerJobByID(ctx, arg.ID)
if err != nil {
Expand Down Expand Up @@ -4246,16 +4251,16 @@ func (q *querier) UpdateProvisionerJobWithCancelByID(ctx context.Context, arg da
}

func (q *querier) UpdateProvisionerJobWithCompleteByID(ctx context.Context, arg database.UpdateProvisionerJobWithCompleteByIDParams) error {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
return err
}
// if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
// return err
// }
return q.db.UpdateProvisionerJobWithCompleteByID(ctx, arg)
}

func (q *querier) UpdateProvisionerJobWithCompleteWithStartedAtByID(ctx context.Context, arg database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams) error {
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
return err
}
// if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceProvisionerJobs); err != nil {
// return err
// }
return q.db.UpdateProvisionerJobWithCompleteWithStartedAtByID(ctx, arg)
}

Expand Down
20 changes: 10 additions & 10 deletions coderd/database/dbauthz/dbauthz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3892,7 +3892,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
}))
s.Run("GetProvisionerJobsCreatedAfter", s.Subtest(func(db database.Store, check *expects) {
_ = dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{CreatedAt: time.Now().Add(-time.Hour)})
check.Args(time.Now()).Asserts(rbac.ResourceProvisionerJobs, policy.ActionRead)
check.Args(time.Now()).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionRead */ )
}))
s.Run("GetTemplateVersionsByIDs", s.Subtest(func(db database.Store, check *expects) {
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
Expand Down Expand Up @@ -3978,7 +3978,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
a := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
b := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
check.Args([]uuid.UUID{a.ID, b.ID}).
Asserts(rbac.ResourceProvisionerJobs, policy.ActionRead).
Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionRead */ ).
Returns(slice.New(a, b))
}))
s.Run("InsertWorkspaceAgent", s.Subtest(func(db database.Store, check *expects) {
Expand Down Expand Up @@ -4022,26 +4022,26 @@ func (s *MethodTestSuite) TestSystemFunctions() {
OrganizationID: j.OrganizationID,
Types: []database.ProvisionerType{j.Provisioner},
ProvisionerTags: must(json.Marshal(j.Tags)),
}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionUpdate)
}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionUpdate */ )
}))
s.Run("UpdateProvisionerJobWithCompleteByID", s.Subtest(func(db database.Store, check *expects) {
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
check.Args(database.UpdateProvisionerJobWithCompleteByIDParams{
ID: j.ID,
}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionUpdate)
}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionUpdate */ )
}))
s.Run("UpdateProvisionerJobWithCompleteWithStartedAtByID", s.Subtest(func(db database.Store, check *expects) {
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
check.Args(database.UpdateProvisionerJobWithCompleteWithStartedAtByIDParams{
ID: j.ID,
}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionUpdate)
}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionUpdate */ )
}))
s.Run("UpdateProvisionerJobByID", s.Subtest(func(db database.Store, check *expects) {
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
check.Args(database.UpdateProvisionerJobByIDParams{
ID: j.ID,
UpdatedAt: time.Now(),
}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionUpdate)
}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionUpdate */ )
}))
s.Run("InsertProvisionerJob", s.Subtest(func(db database.Store, check *expects) {
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
Expand All @@ -4051,19 +4051,19 @@ func (s *MethodTestSuite) TestSystemFunctions() {
StorageMethod: database.ProvisionerStorageMethodFile,
Type: database.ProvisionerJobTypeWorkspaceBuild,
Input: json.RawMessage("{}"),
}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionCreate)
}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionCreate */ )
}))
s.Run("InsertProvisionerJobLogs", s.Subtest(func(db database.Store, check *expects) {
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
check.Args(database.InsertProvisionerJobLogsParams{
JobID: j.ID,
}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionUpdate)
}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionUpdate */ )
}))
s.Run("InsertProvisionerJobTimings", s.Subtest(func(db database.Store, check *expects) {
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
check.Args(database.InsertProvisionerJobTimingsParams{
JobID: j.ID,
}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionUpdate)
}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionUpdate */ )
}))
s.Run("UpsertProvisionerDaemon", s.Subtest(func(db database.Store, check *expects) {
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
Expand Down Expand Up @@ -4279,7 +4279,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
check.Args([]uuid.UUID{uuid.New()}).Asserts(rbac.ResourceSystem, policy.ActionRead)
}))
s.Run("GetProvisionerJobsByIDsWithQueuePosition", s.Subtest(func(db database.Store, check *expects) {
check.Args([]uuid.UUID{}).Asserts(rbac.ResourceProvisionerJobs, policy.ActionRead)
check.Args([]uuid.UUID{}).Asserts( /* rbac.ResourceProvisionerJobs, policy.ActionRead */ )
}))
s.Run("GetReplicaByID", s.Subtest(func(db database.Store, check *expects) {
check.Args(uuid.New()).Asserts(rbac.ResourceSystem, policy.ActionRead).Errors(sql.ErrNoRows)
Expand Down
4 changes: 2 additions & 2 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.

1 change: 1 addition & 0 deletions site/src/api/rbacresourcesGenerated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ export const RBACResourceActions: Partial<
update: "update a provisioner daemon",
},
provisioner_jobs: {
create: "create provisioner jobs",
read: "read provisioner jobs",
update: "update provisioner jobs",
},
Expand Down
Loading