We can't know whether a replacement (i.e. drift of terraform state leading to a resource needing to be deleted/recreated) will take place apriori; we can only detect it atplan time, because the provider decides whether a resource must be replaced and it cannot be inferred through static analysis of the template.

This is likely to be the most common gotcha with using prebuilds, since it requires a slight template modification to use prebuilds effectively, so let's head this off before it's an issue for customers.

Drift details will now be logged in the workspace build logs:

Plus a notification will be sent to template admins when this situation arises:

A new metric -coderd_prebuilt_workspaces_resource_replacements_total - will also increment each time a workspace encounters replacements.

We only trackthat a resource replacement occurred, not how many. Just one is enough to ruin a prebuild, but we can't know apriori which replacement would cause this.
For example, say we have 2 replacements: adocker_container and anull_resource; we don't know which one might
cause an issue (or indeed if either would), so we just track the replacement.

github-actionsbot assigneddannykopping

Apr 25, 2025

dannykopping commented

Apr 25, 2025

View reviewed changes

enterprise/coderd/coderd.goShow resolvedHide resolved

coderd/database/migrations/000318_resource_replacements_notification.up.sql OutdatedShow resolvedHide resolved

SasSwart mentioned this pull request

May 2, 2025

feat: reinitialize agents when a prebuilt workspace is claimed#17475

Merged

github-actionsbot added the staleThis issue is like stale bread. label

May 6, 2025

dannykopping force-pushed thedk/logreplacements branch 3 times, most recently from4859410 to634082bCompare

May 7, 2025 21:06

github-actionsbot removed the staleThis issue is like stale bread. label

May 8, 2025

dannykopping force-pushed thedk/logreplacements branch from0322146 to13168f4Compare

May 8, 2025 12:38

dannykopping commented

May 8, 2025

View reviewed changes

coderd/provisionerdserver/provisionerdserver.go OutdatedShow resolvedHide resolved

provisionerd/proto/provisionerd.proto Outdated

		@@ -75,6 +75,7 @@ message CompletedJob {
		repeated provisioner.Resource resources = 2;
		repeated provisioner.Timing timings = 3;
		repeated provisioner.Module modules = 4;
		repeated provisioner.ResourceReplacement resourceReplacements = 5;

Copy link

ContributorAuthor

dannykoppingMay 6, 2025

Choose a reason for hiding this comment

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

I believe this is backwards-compatible sincerepeated fields are effectivelyoptional if nil length.

enterprise/coderd/prebuilds/reconcile.go

		// nolint:gocritic // Necessary to query all the required data.
		ctx = dbauthz.AsSystemRestricted(ctx)
		// Since this may be called in a fire-and-forget fashion, we need to give up at some point.
		trackCtx, trackCancel := context.WithTimeout(ctx, time.Minute)

Copy link

ContributorAuthor

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

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

This is a best-effort attempt to warn operators of this situation; it's ok if it times out, we'll get a log to trace this with.

Copy link

Member

mafredriMay 12, 2025

Choose a reason for hiding this comment

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

I see this partially covers my earlier comment. I still think it'd be good to take shutdowns into consideration for these and define what that behavior should be (rather than undefined). Right now these routines will be rudely interrupted during shutdown rather than exiting cleanly. Likewise these can be left running even if a CompleteJob is interrupted.

Copy link

ContributorAuthor

dannykoppingMay 13, 2025

Choose a reason for hiding this comment

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

Right now these routines will be rudely interrupted during shutdown rather than exiting cleanly.

Good call! I've now passed in theprovisionerdserver's lifecycle context.

Likewise these can be left running even if a CompleteJob is interrupted.

I believe that's already the case?

provisioner/terraform/executor.go

		@@ -258,7 +258,7 @@ func getStateFilePath(workdir string) string {
		}

		// revive:disable-next-line:flag-parameter
		func (e executor) plan(ctx, killCtx context.Context, env, vars []string, logr logSink,destroy bool) (proto.PlanComplete, error) {
		func (e executor) plan(ctx, killCtx context.Context, env, vars []string, logr logSink,metadata proto.Metadata) (*proto.PlanComplete, error) {

Copy link

ContributorAuthor

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

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

This is the meat of the feature; everything else is just plumbing between system and user eyeball.

provisioner/terraform/executor.go

		level := proto.LogLevel_INFO

		// Terraform indicates that a resource will be deleted and recreated by showing the change along with this substring.
		if bytes.Contains(line, []byte("# forces replacement")) {

Copy link

ContributorAuthor

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

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

A bit flimsy; open to other ideas.
In any case, this is just sugar. The fact that the plan, with all its drift details, are shown will be sufficient. Highlighting the lines is just a courtesy to the user.

coderd/prebuilds/api.go OutdatedShow resolvedHide resolved

coderd/database/migrations/000320_resource_replacements_notification.up.sql OutdatedShow resolvedHide resolved

enterprise/coderd/prebuilds/metricscollector.go

		@@ -13,43 +16,65 @@ import (
		"github.com/coder/coder/v2/coderd/prebuilds"
		)

		const (

Copy link

ContributorAuthor

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

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

Muddies the purpose of the PR a bit, but it was a worthwhile driveby refactoring given that we're adding a new metric (MetricResourceReplacementsCount) and we need to check for its value in a test.

dannykopping requested review frommafredri,SasSwart andevgeniy-scherbina

May 8, 2025 12:39

dannykopping changed the title~~WIP! feat: log resource replacements when claiming a prebuilt workspace~~feat: track resource replacements when claiming a prebuilt workspace

May 8, 2025

dannykopping added14 commits

May 8, 2025 14:48

feat: log resource replacements

b32923a

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

feat: show terraform state drift diff in build logs

0b0830f

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

feat: only highlight lines which mention replacement

256395a

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

feat: notify template admins when prebuild claim results in resource …

61ef61a

…replacement(s)Signed-off-by: Danny Kopping <dannykopping@gmail.com>

chore: appease linter

a66559f

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

chore: fix notifications test

222892b

also a test that was broken from an earlier fixSigned-off-by: Danny Kopping <dannykopping@gmail.com>

fix: don't panic

f34e011

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

fix: renaming type

5168c01

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

chore: updating migration numbers

41e5e0c

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

chore: minor touch-ups

b29e8fa

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

feat: add resource replacements metric

b31ed5e

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

feat: add resource replacement notification

adf98d2

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

make lint; make fmt

f24aef0

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

chore: adding tests

70f9a53

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

dannykopping force-pushed thedk/logreplacements branch from13168f4 to70f9a53Compare

May 8, 2025 12:48

dannykopping marked this pull request as ready for review

May 8, 2025 12:59

mafredri reviewed

May 12, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

No major issues uncovered, my main concern is the goroutine management in CompleteJob, other than that mostly minor nits and suggestions.

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

coderd/database/migrations/000320_resource_replacements_notification.up.sql OutdatedShow resolvedHide resolved

coderd/notifications/enqueuer.go OutdatedShow resolvedHide resolved

coderd/provisionerdserver/provisionerdserver.go OutdatedShow resolvedHide resolved

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

provisioner/terraform/executor.goShow resolvedHide resolved

provisioner/terraform/executor.go OutdatedShow resolvedHide resolved

provisioner/terraform/executor.goShow resolvedHide resolved

provisioner/terraform/resource_replacements.go OutdatedShow resolvedHide resolved

dannykopping added a commit that referenced this pull request

May 12, 2025

feat: addis_prebuild_claimto distinguish post-claim provisioning (#…

af2941b

…17757)Used in combination withcoder/terraform-provider-coder#396This is required by both#17475 and#17571Operators may need to conditionalize their templates to perform certainoperations once a prebuilt workspace has been claimed. This value will**only** be set once a claim takes place and a subsequent `terraformapply` occurs. Any `terraform apply` runs thereafter will beindistinguishable from a normal run on a workspace.---------Signed-off-by: Danny Kopping <dannykopping@gmail.com>

dannykopping added9 commits

May 12, 2025 18:05

Merge branch 'main' of github.com:/coder/coder into dk/logreplacement

5209aae

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

Merge branch 'main' of github.com:/coder/coder into dk/logreplacements

39ce658

Merge branch 'main' of github.com:/coder/coder into dk/logreplacements

ac5655f

chore: note provisioner API change

82c3f58

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

chore: fixups

7577a90

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

chore: adding note about immutable resources

a893b79

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

chore: review feedback

d9c906a

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

Merge branch 'main' of github.com:/coder/coder into dk/logreplacements

471198a

chore: merge conflicts

7d694e6

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

github-actionsbot reviewed

May 13, 2025

View reviewed changes

docs/admin/templates/extending-templates/prebuilt-workspaces.mdShow resolvedHide resolved

chore: fix 'is not iterable' bullshit

6b7a8b7

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

mafredri reviewed

May 13, 2025

View reviewed changes

Copy link

Member

mafredri 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.

A few more comments but my main concern is around using theatomic.Pointer for the notifier, I don't really see a reason for it in which case I think the code should be simplified.

The other remaining concern is around goroutine/shutdown which could lead to test flakes (leak detection).

coderd/provisionerdserver/provisionerdserver.goShow resolvedHide resolved

docs/admin/templates/extending-templates/prebuilt-workspaces.mdShow resolvedHide resolved

enterprise/coderd/coderd.go OutdatedShow resolvedHide resolved

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

dannykopping added5 commits

May 14, 2025 09:17

Merge branch 'main' of github.com:/coder/coder into dk/logreplacements

5df2cb3

chore: rename migrations

6d1c3ea

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

chore: set notifications manager before enterprise server initializes…

5f62702

… APIavoid all this pointer jugglingSigned-off-by: Danny Kopping <dannykopping@gmail.com>

chore: completing refactor since#17792was since merged

f74d799

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

chore: remove unnecessary atomicity since map is protected by mutex a…

971f65c

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

dannykopping requested a review fromspikecurtis

May 14, 2025 09:39

mafredri approved these changes

May 14, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

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

Nice work!

chore: appeasing linter's Very Important Suggestion

bc362b0

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

spikecurtis approved these changes

May 14, 2025

View reviewed changes

Copy link

Contributor

spikecurtis left a comment

Choose a reason for hiding this comment

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

My review limited to the protobuf / version changes.

dannykoppingenabled auto-merge (squash)

May 14, 2025 09:51

dannykoppingdisabled auto-merge

May 14, 2025 09:53

dannykopping added2 commits

May 14, 2025 14:21

Merge branch 'main' of github.com:/coder/coder into dk/logreplacements

4fbd356

chore: remove old replacement logging

b9eb8be

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

dannykopping merged commit6e96778 intomain

May 14, 2025

39 checks passed

dannykopping deleted the dk/logreplacements branch

May 14, 2025 12:52

github-actionsbot locked and limited conversation to collaborators

May 14, 2025

Labels

None yet

4 participants

Movatterモバイル変換

feat: track resource replacements when claiming a prebuilt workspace#17571

feat: track resource replacements when claiming a prebuilt workspace#17571

Uh oh!

Conversation

dannykopping commentedApr 25, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dannykoppingMay 6, 2025

Choose a reason for hiding this comment

Uh oh!

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

Uh oh!

mafredriMay 12, 2025

Choose a reason for hiding this comment

Uh oh!

dannykoppingMay 13, 2025

Choose a reason for hiding this comment

Uh oh!

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

Uh oh!

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dannykoppingMay 8, 2025

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

spikecurtis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dannykopping commentedApr 25, 2025•
edited
Loading

mafredri left a comment•
edited
Loading