Skip to content

fix(coderd/database): avoid clobbering workspace build state #9826

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 8 commits into from
Sep 22, 2023
Merged

fix(coderd/database): avoid clobbering workspace build state #9826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
daece62
RED: update tests in enterprise/coderd/schedule.go to replicate issue
johnstcn Sep 22, 2023
2dd595d
fixup! RED: update tests in enterprise/coderd/schedule.go to replicat…
johnstcn Sep 22, 2023
8c53881
RED: add new database queries
johnstcn Sep 22, 2023
d34b5a4
fixup! RED: add new database queries
johnstcn Sep 22, 2023
f6d839f
GREEN: update to use new queries
johnstcn Sep 22, 2023
a6ef20c
Remove UpdateWorkspaceBuildByID query
johnstcn Sep 22, 2023
6b37452
modify GetActiveWorkspaceBuildsByTemplateID to not return incomplete …
johnstcn Sep 22, 2023
c166635
no context
johnstcn Sep 22, 2023
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
11 changes: 5 additions & 6 deletions coderd/activitybump_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,11 @@ func TestWorkspaceActivityBump(t *testing.T) {
dbBuild, err := db.GetWorkspaceBuildByID(ctx, workspace.LatestBuild.ID)
require.NoError(t, err)

err = db.UpdateWorkspaceBuildByID(ctx, database.UpdateWorkspaceBuildByIDParams{
ID: workspace.LatestBuild.ID,
UpdatedAt: dbtime.Now(),
ProvisionerState: dbBuild.ProvisionerState,
Deadline: dbBuild.Deadline,
MaxDeadline: dbtime.Now().Add(maxTTL),
err = db.UpdateWorkspaceBuildDeadlineByID(ctx, database.UpdateWorkspaceBuildDeadlineByIDParams{
ID: workspace.LatestBuild.ID,
UpdatedAt: dbtime.Now(),
Deadline: dbBuild.Deadline,
MaxDeadline: dbtime.Now().Add(maxTTL),
})
require.NoError(t, err)
}
Expand Down
19 changes: 13 additions & 6 deletions coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2675,7 +2675,15 @@ func (q *querier) UpdateWorkspaceAutostart(ctx context.Context, arg database.Upd
return update(q.log, q.auth, fetch, q.db.UpdateWorkspaceAutostart)(ctx, arg)
}

func (q *querier) UpdateWorkspaceBuildByID(ctx context.Context, arg database.UpdateWorkspaceBuildByIDParams) error {
// UpdateWorkspaceBuildCostByID is used by the provisioning system to update the cost of a workspace build.
func (q *querier) UpdateWorkspaceBuildCostByID(ctx context.Context, arg database.UpdateWorkspaceBuildCostByIDParams) error {
if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil {
return err
}
return q.db.UpdateWorkspaceBuildCostByID(ctx, arg)
}

func (q *querier) UpdateWorkspaceBuildDeadlineByID(ctx context.Context, arg database.UpdateWorkspaceBuildDeadlineByIDParams) error {
build, err := q.db.GetWorkspaceBuildByID(ctx, arg.ID)
if err != nil {
return err
Expand All @@ -2685,20 +2693,19 @@ func (q *querier) UpdateWorkspaceBuildByID(ctx context.Context, arg database.Upd
if err != nil {
return err
}

err = q.authorizeContext(ctx, rbac.ActionUpdate, workspace.RBACObject())
if err != nil {
return err
}

return q.db.UpdateWorkspaceBuildByID(ctx, arg)
return q.db.UpdateWorkspaceBuildDeadlineByID(ctx, arg)
}

// UpdateWorkspaceBuildCostByID is used by the provisioning system to update the cost of a workspace build.
func (q *querier) UpdateWorkspaceBuildCostByID(ctx context.Context, arg database.UpdateWorkspaceBuildCostByIDParams) error {
func (q *querier) UpdateWorkspaceBuildProvisionerStateByID(ctx context.Context, arg database.UpdateWorkspaceBuildProvisionerStateByIDParams) error {
if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil {
return err
}
return q.db.UpdateWorkspaceBuildCostByID(ctx, arg)
return q.db.UpdateWorkspaceBuildProvisionerStateByID(ctx, arg)
}

// Deprecated: Use SoftDeleteWorkspaceByID
Expand Down
19 changes: 13 additions & 6 deletions coderd/database/dbauthz/dbauthz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1232,14 +1232,13 @@ func (s *MethodTestSuite) TestWorkspace() {
ID: ws.ID,
}).Asserts(ws, rbac.ActionUpdate).Returns()
}))
s.Run("UpdateWorkspaceBuildByID", s.Subtest(func(db database.Store, check *expects) {
s.Run("UpdateWorkspaceBuildDeadlineByID", s.Subtest(func(db database.Store, check *expects) {
ws := dbgen.Workspace(s.T(), db, database.Workspace{})
build := dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: uuid.New()})
check.Args(database.UpdateWorkspaceBuildByIDParams{
ID: build.ID,
UpdatedAt: build.UpdatedAt,
Deadline: build.Deadline,
ProvisionerState: []byte{},
check.Args(database.UpdateWorkspaceBuildDeadlineByIDParams{
ID: build.ID,
UpdatedAt: build.UpdatedAt,
Deadline: build.Deadline,
Copy link
Member

@mafredri mafredri Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a test, but in theory this clobbers max deadline? Should we do some testing about that / prevent it?

Copy link
Member Author

@johnstcn johnstcn Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In theory, yes - but I want to keep the scope of this limited to provisioner state.
We can think about #9822 separately.

}).Asserts(ws, rbac.ActionUpdate)
}))
s.Run("SoftDeleteWorkspaceByID", s.Subtest(func(db database.Store, check *expects) {
Expand Down Expand Up @@ -1378,6 +1377,14 @@ func (s *MethodTestSuite) TestSystemFunctions() {
DailyCost: 10,
}).Asserts(rbac.ResourceSystem, rbac.ActionUpdate)
}))
s.Run("UpdateWorkspaceBuildProvisionerStateByID", s.Subtest(func(db database.Store, check *expects) {
ws := dbgen.Workspace(s.T(), db, database.Workspace{})
build := dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: uuid.New()})
check.Args(database.UpdateWorkspaceBuildProvisionerStateByIDParams{
ID: build.ID,
ProvisionerState: []byte("testing"),
}).Asserts(rbac.ResourceSystem, rbac.ActionUpdate)
}))
s.Run("UpsertLastUpdateCheck", s.Subtest(func(db database.Store, check *expects) {
check.Args("value").Asserts(rbac.ResourceSystem, rbac.ActionUpdate)
}))
Expand Down
45 changes: 34 additions & 11 deletions coderd/database/dbfake/dbfake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5854,7 +5854,7 @@ func (q *FakeQuerier) UpdateWorkspaceAutostart(_ context.Context, arg database.U
return sql.ErrNoRows
}

func (q *FakeQuerier) UpdateWorkspaceBuildByID(_ context.Context, arg database.UpdateWorkspaceBuildByIDParams) error {
func (q *FakeQuerier) UpdateWorkspaceBuildCostByID(_ context.Context, arg database.UpdateWorkspaceBuildCostByIDParams) error {
if err := validateDatabaseType(arg); err != nil {
return err
}
Expand All @@ -5866,32 +5866,55 @@ func (q *FakeQuerier) UpdateWorkspaceBuildByID(_ context.Context, arg database.U
if workspaceBuild.ID != arg.ID {
continue
}
workspaceBuild.UpdatedAt = arg.UpdatedAt
workspaceBuild.ProvisionerState = arg.ProvisionerState
workspaceBuild.Deadline = arg.Deadline
workspaceBuild.MaxDeadline = arg.MaxDeadline
workspaceBuild.DailyCost = arg.DailyCost
q.workspaceBuilds[index] = workspaceBuild
return nil
}
return sql.ErrNoRows
}

func (q *FakeQuerier) UpdateWorkspaceBuildCostByID(_ context.Context, arg database.UpdateWorkspaceBuildCostByIDParams) error {
if err := validateDatabaseType(arg); err != nil {
func (q *FakeQuerier) UpdateWorkspaceBuildDeadlineByID(_ context.Context, arg database.UpdateWorkspaceBuildDeadlineByIDParams) error {
err := validateDatabaseType(arg)
if err != nil {
return err
}

q.mutex.Lock()
defer q.mutex.Unlock()

for index, workspaceBuild := range q.workspaceBuilds {
if workspaceBuild.ID != arg.ID {
for idx, build := range q.workspaceBuilds {
if build.ID != arg.ID {
continue
}
workspaceBuild.DailyCost = arg.DailyCost
q.workspaceBuilds[index] = workspaceBuild
build.Deadline = arg.Deadline
build.MaxDeadline = arg.MaxDeadline
build.UpdatedAt = arg.UpdatedAt
q.workspaceBuilds[idx] = build
return nil
}

return sql.ErrNoRows
}

func (q *FakeQuerier) UpdateWorkspaceBuildProvisionerStateByID(_ context.Context, arg database.UpdateWorkspaceBuildProvisionerStateByIDParams) error {
err := validateDatabaseType(arg)
if err != nil {
return err
}

q.mutex.Lock()
defer q.mutex.Unlock()

for idx, build := range q.workspaceBuilds {
if build.ID != arg.ID {
continue
}
build.ProvisionerState = arg.ProvisionerState
build.UpdatedAt = arg.UpdatedAt
q.workspaceBuilds[idx] = build
return nil
}

return sql.ErrNoRows
}

Expand Down
21 changes: 14 additions & 7 deletions coderd/database/dbmetrics/dbmetrics.go
View file
Open in desktop

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

38 changes: 26 additions & 12 deletions coderd/database/dbmock/dbmock.go
View file
Open in desktop

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

3 changes: 2 additions & 1 deletion coderd/database/querier.go
View file
Open in desktop

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

71 changes: 46 additions & 25 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.

Loading