Skip to content

Commit d845c5f

Browse files
dannykoppingdannykopping
committed
chore: minor touch-ups
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
1 parent 2aa3509 commit d845c5f

File tree

3 files changed

+44
-28
lines changed

3 files changed

+44
-28
lines changed

‎coderd/provisionerdserver/provisionerdserver.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1841,7 +1841,7 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database.
18411841
}
18421842

18431843
func (s *server) notifyPrebuiltWorkspaceResourceReplacement(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild, claimantID uuid.UUID, replacements []*sdkproto.ResourceReplacement) {
1844-
if claimantID == uuid.Nil {
1844+
if claimantID == uuid.Nil || len(replacements) == 0 {
18451845
// This is not a prebuild claim.
18461846
return
18471847
}
@@ -1878,8 +1878,9 @@ func (s *server) notifyPrebuiltWorkspaceResourceReplacement(ctx context.Context,
18781878
// Associate this notification with all the related entities.
18791879
workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID,
18801880
); err != nil {
1881-
s.Logger.Warn(ctx, "failed to notify of prebuilt workspace resource replacement", slog.Error(err))
1882-
break
1881+
s.Logger.Warn(ctx, "failed to notify of prebuilt workspace resource replacement", slog.Error(err),
1882+
slog.F("user_id", templateAdmin.ID.String()), slog.F("user_email", templateAdmin.Email))
1883+
continue
18831884
}
18841885
}
18851886
}

‎provisioner/terraform/executor.go

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -300,10 +300,14 @@ func (e *executor) plan(ctx, killCtx context.Context, env, vars []string, logr l
300300
graphTimings := newTimingAggregator(database.ProvisionerJobTimingStageGraph)
301301
graphTimings.ingest(createGraphTimingsEvent(timingGraphStart))
302302

303-
state, plan, replacements, err := e.planResources(ctx, killCtx, planfilePath)
303+
state, plan, err := e.planResources(ctx, killCtx, planfilePath)
304304
if err != nil {
305305
graphTimings.ingest(createGraphTimingsEvent(timingGraphErrored))
306-
return nil, err
306+
return nil, xerrors.Errorf("plan resources: %w", err)
307+
}
308+
planJSON, err := json.Marshal(plan)
309+
if err != nil {
310+
return nil, xerrors.Errorf("marshal plan: %w", err)
307311
}
308312

309313
graphTimings.ingest(createGraphTimingsEvent(timingGraphComplete))
@@ -312,17 +316,22 @@ func (e *executor) plan(ctx, killCtx context.Context, env, vars []string, logr l
312316
// the point of prebuilding if the expensive resource is replaced once claimed!
313317
var (
314318
isPrebuildClaimAttempt = !destroy && metadata.GetPrebuildClaimForUserId() != ""
315-
reps []*proto.ResourceReplacement
319+
resReps []*proto.ResourceReplacement
316320
)
317-
if count := len(replacements); count > 0 && isPrebuildClaimAttempt {
318-
// TODO(dannyk): we should log drift always (not just during prebuild claim attempts); we're validating that this output
319-
// will not be overwhelming for end-users, but it'll certainly be super valuable for template admins
320-
// to diagnose this resource replacement issue, at least.
321-
e.logDrift(ctx, killCtx, planfilePath, logr)
322-
323-
reps = make([]*proto.ResourceReplacement, 0, len(replacements))
324-
for n, p := range replacements {
325-
reps = append(reps, &proto.ResourceReplacement{
321+
if repsFromPlan := findResourceReplacements(plan); len(repsFromPlan) > 0 {
322+
if isPrebuildClaimAttempt {
323+
// TODO(dannyk): we should log drift always (not just during prebuild claim attempts); we're validating that this output
324+
// will not be overwhelming for end-users, but it'll certainly be super valuable for template admins
325+
// to diagnose this resource replacement issue, at least.
326+
// Once prebuilds moves out of beta, consider deleting this condition.
327+
328+
// Lock held before calling (see top of method).
329+
e.logDrift(ctx, killCtx, planfilePath, logr)
330+
}
331+
332+
resReps = make([]*proto.ResourceReplacement, 0, len(repsFromPlan))
333+
for n, p := range repsFromPlan {
334+
resReps = append(resReps, &proto.ResourceReplacement{
326335
Resource: n,
327336
Paths: p,
328337
})
@@ -335,8 +344,8 @@ func (e *executor) plan(ctx, killCtx context.Context, env, vars []string, logr l
335344
ExternalAuthProviders: state.ExternalAuthProviders,
336345
Timings: append(e.timings.aggregate(), graphTimings.aggregate()...),
337346
Presets: state.Presets,
338-
Plan: plan,
339-
ResourceReplacements: reps,
347+
Plan: planJSON,
348+
ResourceReplacements: resReps,
340349
}, nil
341350
}
342351

@@ -358,18 +367,18 @@ func onlyDataResources(sm tfjson.StateModule) tfjson.StateModule {
358367
}
359368

360369
// planResources must only be called while the lock is held.
361-
func (e *executor) planResources(ctx, killCtx context.Context, planfilePath string) (*State, json.RawMessage, resourceReplacements, error) {
370+
func (e *executor) planResources(ctx, killCtx context.Context, planfilePath string) (*State, *tfjson.Plan, error) {
362371
ctx, span := e.server.startTrace(ctx, tracing.FuncName())
363372
defer span.End()
364373

365374
plan, err := e.parsePlan(ctx, killCtx, planfilePath)
366375
if err != nil {
367-
return nil, nil, nil, xerrors.Errorf("show terraform plan file: %w", err)
376+
return nil, nil, xerrors.Errorf("show terraform plan file: %w", err)
368377
}
369378

370379
rawGraph, err := e.graph(ctx, killCtx)
371380
if err != nil {
372-
return nil, nil, nil, xerrors.Errorf("graph: %w", err)
381+
return nil, nil, xerrors.Errorf("graph: %w", err)
373382
}
374383
modules := []*tfjson.StateModule{}
375384
if plan.PriorState != nil {
@@ -387,15 +396,10 @@ func (e *executor) planResources(ctx, killCtx context.Context, planfilePath stri
387396

388397
state, err := ConvertState(ctx, modules, rawGraph, e.server.logger)
389398
if err != nil {
390-
return nil, nil, nil, err
391-
}
392-
393-
planJSON, err := json.Marshal(plan)
394-
if err != nil {
395-
return nil, nil, nil, err
399+
return nil, nil, err
396400
}
397401

398-
return state, planJSON, findResourceReplacements(plan), nil
402+
return state, plan, nil
399403
}
400404

401405
// parsePlan must only be called while the lock is held.

‎provisioner/terraform/resource_replacements.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,18 @@ func findResourceReplacements(plan *tfjson.Plan) resourceReplacements {
4242
continue
4343
}
4444

45-
// Replacing our resources, no problem!
45+
// Replacing our resources: could be a problem - but we ignore since they're "virtual" resources. If any of these
46+
// resources' attributes are referenced by non-coder resources, those will show up as transitive changes there.
47+
// i.e. if the coder_agent.id attribute is used in docker_container.env
48+
//
49+
// Replacing our resources is not strictly a problem in and of itself.
50+
//
51+
// NOTE:
52+
// We may need to special-case coder_agent in the future. Currently, coder_agent is replaced on every build
53+
// because it only supports Create but not Update: https://github.com/coder/terraform-provider-coder/blob/5648efb/provider/agent.go#L28
54+
// When we can modify an agent's attributes, some of which may be immutable (like "arch") and some may not (like "env"),
55+
// then we'll have to handle this specifically.
56+
// This will only become relevant once we support multiple agents: https://github.com/coder/coder/issues/17388
4657
if strings.Index(ch.Type, "coder_") == 0 {
4758
continue
4859
}

0 commit comments

Comments
 (0)