Movatterモバイル変換

SasSwart changed the title~~feat: add cli commands to pause and resume prebuilt workspace reconci…~~feat: provide a mechanism to pause prebuilt workspace reconciliation

renumber migrations

0b4d669

SasSwart changed the title~~feat: provide a mechanism to pause prebuilt workspace reconciliation~~feat: allow users to pause prebuilt workspace reconciliation

SasSwart requested review fromjohnstcn andssncferreira

July 1, 2025 19:02

johnstcn reviewed

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

LGTM so far, just missing the final piece of plumbing

SasSwart added2 commits

July 1, 2025 19:42

make gen

caa9224

get prebuilds to honor the pause setting and expose a metric to show …

140d820

…whether prebuilds are currently paused

SasSwart requested a review fromjohnstcn

July 2, 2025 08:26

SasSwart marked this pull request as ready for review

July 2, 2025 08:29

johnstcn approved these changes

Copy link

Member

johnstcn commentedJul 2, 2025•
edited
Loading

~~Might be good to also avoid the termprebuilds in the CLI command unless there is prior art~~

EDIT: prior art exists withcodersdk.PrebuildsConfig and in numerous other places in the codebase. I don't have any better naming suggestion, so leave it ascoder prebuilds

dannykopping reviewed

enterprise/coderd/prebuilds/metricscollector.go

		MetricEligibleGauge=namespace+"eligible"
		MetricPresetHardLimitedGauge=namespace+"preset_hard_limited"
		MetricLastUpdatedGauge=namespace+"metrics_last_updated"
		MetricReconciliationPausedGauge=namespace+"reconciliation_paused"

Copy link

Contributor

Choose a reason for hiding this comment

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

Paused is difficult to reason about; I'd suggest inverting and make it_running so that a good value is 1 and a bad value is 0.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

paused in this case has a technical meaning relating to a specific setting. If we rename this torunning then I think we need to change the setting to be{"reconciliation_running": true}, which doesn't communicate the intent of the API clearly.

I'd like to keep it how it currently is, but as compensatory measures I will:

update the metric documentation to be clear about its meaning
add a second metric to represent a general "check engine light" for prebuilds

dannykopping approved these changes

enterprise/coderd/prebuilds.go

Copy link

Contributor

dannykopping left a comment

Choose a reason for hiding this comment

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

This will be pretty handy, LGTM!

		return
		}

		ifbytes.Equal(settingsJSON, []byte(currentSettingsJSON)) {

Copy link

Contributor

dannykoppingJul 2, 2025

Choose a reason for hiding this comment

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

Nit: not a problem now but when we add more settings, map ordering may cause this check to fail.

Copy link

ContributorAuthor

SasSwartJul 2, 2025

Choose a reason for hiding this comment

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

good catch.

enterprise/coderd/prebuilds/metricscollector.go OutdatedShow resolvedHide resolved

enterprise/coderd/prebuilds/reconcile.goShow resolvedHide resolved

enterprise/coderd/prebuilds/reconcile_test.go Outdated

		require.Len(t,workspaces,2,"should have created 2 prebuilds")

		// Now pause prebuilds reconciliation
		err=db.UpsertPrebuildsSettings(ctx,`{"reconciliation_paused": true}`)

Copy link

Contributor

dannykoppingJul 2, 2025

Choose a reason for hiding this comment

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

I'd suggest going via the API here to avoid duplicating this business logic.

Copy link

ContributorAuthor

SasSwartJul 2, 2025

Choose a reason for hiding this comment

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

good point

ssncferreira reviewed

coderd/database/dbauthz/dbauthz.go

Copy link

Contributor

ssncferreira left a comment•
edited
Loading

Choose a reason for hiding this comment

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

LGTM 🎉 Thanks for creating this.
There is just a question about rbac: with this change we require the permissionrbac.ResourceDeploymentConfig, so that means that only the owner of the resouce or a user with roleauditorRole can upsert this setting. Should we extend it to Admins?

coderd/audit/request.goShow resolvedHide resolved

		}

		func (q*querier)UpsertPrebuildsSettings(ctx context.Context,valuestring)error {
		iferr:=q.authorizeContext(ctx,policy.ActionUpdate,rbac.ResourceDeploymentConfig);err!=nil {

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

Just to clarify here on the rbac side, this means that only a user with this permission can update the setting. Fromhttps://github.com/coder/coder/blob/main/coderd/rbac/roles.go currently, only theauditorRole has this permission. Is that intended?

Copy link

Member

johnstcnJul 2, 2025

Choose a reason for hiding this comment

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

Auditor only has read though?

coder/coderd/rbac/roles.go

Line 327 in8a69f6a

ResourceDeploymentConfig.Type: {policy.ActionRead},

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

Yes, good catch! 👀 So it means that only the owner of theResourceDeploymentConfig can update the settings, right? Should we maybe extend it to the Admins? 🤔

Copy link

Member

johnstcnJul 2, 2025•
edited
Loading

Choose a reason for hiding this comment

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

I don't think there exists any other suitable role that should have permission to edit site-wide deployment config? Our choices are site-wide template admin or site-wide user admin, neither of which feel right to me. Site-wide auditor should only have read permissions, and all of the org-scoped admins are out of site-wide scope.

This might be something to explore in a separate PR.

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

Yes, this could definitely be addressed in a separate PR. You might be right that adding the permission to admin roles isn’t the best approach. My main concern was that currently only users with theOwner role can update this setting. In our dogfood environment, this works fine since many users have theOwner role, but this is not the typical case.

On second thought, limiting this permission to theOwner role does make sense, since it’s an important command and users should be fully aware of the implications when using it.

coderd/database/types.go


		typePrebuildsSettingsstruct {
		ID uuid.UUID`db:"id" json:"id"`
		ReconciliationPausedbool`db:"reconciliation_paused" json:"reconciliation_paused"`

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

Do you think it would also be useful to add a timestamp?

enterprise/coderd/prebuilds.go


		ifbytes.Equal(settingsJSON, []byte(currentSettingsJSON)) {
		// See: https://www.rfc-editor.org/rfc/rfc7232#section-4.1
		httpapi.Write(ctx,rw,http.StatusNotModified,nil)

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

nit: according to the RFC the 304 Not Modified is normally a response to a GET request.
Maybe a 204 No Content makes more sense in this case, wdyt?https://datatracker.ietf.org/doc/html/rfc7231#section-6.3.5

enterprise/cli/prebuilds.go

		returnxerrors.Errorf("unable to pause prebuilds: %w",err)
		}

		_,_=fmt.Fprintln(inv.Stderr,"Prebuilds are now paused.")

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

Why not?

Suggested change

	_,_=fmt.Fprintln(inv.Stderr,"Prebuilds are now paused.")
	_,_=fmt.Fprintln(inv.Stdout,"Prebuilds are now paused.")

Copy link

ContributorAuthor

SasSwartJul 2, 2025

Choose a reason for hiding this comment

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

My reasoning was that this command doesn't actually produce any output of its own.
It reports its own status, but that's more logging than output.

enterprise/cli/prebuilds_test.go

		assert.Contains(t,buf.String(),"Prebuilds are now paused.")

		// Verify the settings were actually updated
		//nolint:gocritic // Only owners can change deployment settings

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

Why the//nolint:gocritic? 🤔

Copy link

ContributorAuthor

SasSwartJul 2, 2025

Choose a reason for hiding this comment

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

There's a rule to check that we test with non-owner roles as they're usually more appropriate. In this case, owner is the appropriate role.

enterprise/coderd/prebuilds/reconcile_test.go

		err=db.UpsertPrebuildsSettings(ctx,`{"reconciliation_paused": false}`)
		require.NoError(t,err)

		// Run reconciliation again - it should now recreate the prebuilds

Copy link

Contributor

ssncferreiraJul 2, 2025

Choose a reason for hiding this comment

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

Nice test 🤩

deduplicate prebuilds pause business logic

cdd55fe

SasSwartenabled auto-merge (squash)

July 2, 2025 14:55

SasSwart merged commit01163ea intomain

34 checks passed

SasSwart deleted the jjs/internal-368 branch

July 2, 2025 15:05

github-actionsbot locked and limited conversation to collaborators