We need two separate locks because we'll have awider reconciliation loop which must hold a global lock, and an inner lock which is used to protect the piece where wedetermine the state. The latter can be called outside of a reconciliation loop (as in the metrics collection).

coderd/database/migrations/000302_prebuilds.up.sql OutdatedShow resolvedHide resolved

coderd/database/querier_test.go OutdatedShow resolvedHide resolved

dannykopping reviewed

Mar 13, 2025

View reviewed changes

coderd/database/migrations/000303_preset_prebuilds.up.sql OutdatedShow resolvedHide resolved

dannykopping reviewed

Mar 13, 2025

View reviewed changes

coderd/database/dbauthz/dbauthz.go OutdatedShow resolvedHide resolved

evgeniy-scherbina reviewed

Mar 13, 2025

View reviewed changes

coderd/database/migrations/000302_prebuilds.up.sql OutdatedShow resolvedHide resolved

dannykopping mentioned this pull request

Mar 13, 2025

chore: add prebuilds system user#16916

Merged

dannykopping force-pushed theprebuilds-db branch frome1b1e36 to81ba8d2Compare

March 13, 2025 21:01

dannykopping changed the base branch frommain toprebuilds-system-user

March 13, 2025 21:01

dannykopping force-pushed theprebuilds-db branch 2 times, most recently from2024110 to390a1fdCompare

March 13, 2025 21:22

Copy link

Contributor

evgeniy-scherbina commentedMar 14, 2025

Do we want to add unit-tests on DB level in this PR to make sure VIEWs and SQL queries are correct?

Copy link

Contributor

dannykopping commentedMar 14, 2025

Do we want to add unit-tests on DB level in this PR to make sure VIEWs and SQL queries are correct?

Feels like overkill to me, since we have tests which rely on the views indirectly, but if you feel it'd add value then go ahead 👍

SasSwart force-pushed theprebuilds-db branch 3 times, most recently from70ecdcc to7e062e2Compare

March 17, 2025 08:14

SasSwartand others added7 commits

March 17, 2025 12:11

add prebuilds system user database changes and associated changes

300e80f

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

optionally prevent system users from counting to user count

b788237

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

appease the linter

8122595

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

add unit test for system user behaviour

bfb7c28

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

reverting RBAC changes; not relevant here

6639167

appeasing linterSigned-off-by: Danny Kopping <dannykopping@gmail.com>

removing unnecessary changes

769ae1d

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

exclude system user db tests from non-linux OSs

e7e9c27

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

dannykopping force-pushed theprebuilds-system-user branch fromc92fb1b toe7e9c27Compare

March 17, 2025 12:12

SasSwart added3 commits

March 17, 2025 12:19

Rename prebuild system user reference

3936047

ensure that users.IsSystem is not nullable

8bdcafb

feat: add migrations and queries to support prebuilds

412d198

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

SasSwart mentioned this pull request

Mar 17, 2025

Reintegrate Workspace Prebuilds feature branch intomaincoder/internal#516

Closed

12 tasks

SasSwart mentioned this pull request

Mar 28, 2025

Recognise and persist prebuild definitions on template importcoder/internal#507

Closed

evgeniy-scherbina added14 commits

March 31, 2025 09:13

refactor: clarify comment for SQL query

5150a5c

refactor: fix indentations

bff34ea

refactor: rename helper func in test package

ef462b6

refactor: database level tests

dc45165

refactor: database level tests

9c8a352

refactor: helper funcs in db-level tests

eb80919

refactor: minor improvement in SQL query

0b2bbee

refactor: rename SQL queries

3a97bf6

refactor: rename SQL queries

2eeb884

refactor: rename fields in SQL query

73f99e8

fix: improve rbac policies

113e12b

fix: minor fix in filtered_builds CTE

217cae0

fix: formatting

b0bf220

refactor: minor refactoring

ff72a00

spikecurtis reviewed

Apr 2, 2025

View reviewed changes

coderd/database/dbauthz/dbauthz.go OutdatedShow resolvedHide resolved

evgeniy-scherbina added9 commits

April 2, 2025 10:53

fix: handle presets with the same tv.id and name

a007d4a

fix: redefine RBAC permissions for prebuilds

ea9c53b

Merge remote-tracking branch 'origin/main' into prebuilds-db

66d44ed

fix: fix migration numbers

29e121f

test: fix dbmem tests

f64754f

test: fix dbmem tests

7e02397

fix: linter

55d9827

fix: linter

866454b

fix: linter

4fa959a

Emyrk approved these changes

Apr 3, 2025

View reviewed changes

Copy link

Member

Emyrk left a comment

Choose a reason for hiding this comment

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

👍 on the RBAC checks.@spikecurtis no ResourceSystem is now used.

Resource<Type>.All() is used in almost all of these because there is no filter for orgs or a user. The reconciliation loop queries across all orgs.

The queries should succeed or fail as an entire set, rather then be filtered. Based on how they are used. In the future, if we add more views for prebuilds, we can update these queries to filter and apply the appropriate checks. Similar to how we filter workspaces & templates today.

coderd/database/dbauthz/dbauthz.go

Comment on lines +1148 to +1160

		workspaceObject := rbac.ResourceWorkspace.WithOwner(arg.NewUserID.String()).InOrg(preset.OrganizationID)
		err = q.authorizeContext(ctx, policy.ActionCreate, workspaceObject.RBACObject())
		if err != nil {
		return empty, err
		}

		tpl, err := q.GetTemplateByID(ctx, preset.TemplateID.UUID)
		if err != nil {
		return empty, xerrors.Errorf("verify template by id: %w", err)
		}
		if err := q.authorizeContext(ctx, policy.ActionUse, tpl); err != nil {
		return empty, xerrors.Errorf("use template for workspace: %w", err)
		}

Copy link

Member

EmyrkApr 3, 2025

Choose a reason for hiding this comment

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

Just leave a comment this should matchInsertWorkspace

coderd/database/dbauthz/dbauthz.go

Comment on lines +1186 to +1191

		func (q *querier) CountInProgressPrebuilds(ctx context.Context) ([]database.CountInProgressPrebuildsRow, error) {
		if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceWorkspace.All()); err != nil {
		return nil, err
		}
		return q.db.CountInProgressPrebuilds(ctx)
		}

Copy link

Member

EmyrkApr 3, 2025

Choose a reason for hiding this comment

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

This query spans all organizations, and prebuilds are just workspaces 👍

coderd/database/dbauthz/dbauthz.goShow resolvedHide resolved

coderd/database/dbauthz/dbauthz.go

Comment on lines +2200 to +2206

		func (q *querier) GetPresetsBackoff(ctx context.Context, lookback time.Time) ([]database.GetPresetsBackoffRow, error) {
		// GetPresetsBackoff returns a list of template version presets along with metadata such as the number of failed prebuilds.
		if err := q.authorizeContext(ctx, policy.ActionViewInsights, rbac.ResourceTemplate.All()); err != nil {
		return nil, err
		}
		return q.db.GetPresetsBackoff(ctx, lookback)
		}

Copy link

Member

EmyrkApr 3, 2025

Choose a reason for hiding this comment

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

@spikecurtis we decided to just lump in the preset metrics with template insights. Since template insights are metrics related to a template, prebuild metrics metadata is essentially the same imo.