feat: Add graceful exits to provisionerd #372

kylecarbs · 2022-02-27T20:45:46Z

Terraform (or other provisioners) may need to clean up state, or
cancel actions before exit. This adds the ability to gracefully
exit provisionerd.

We'll eventually make this operate on a signal to allow for usage in a Kubernetes environment with graceful shutdowns.

The added Shutdown() function in provisionerd has the expected usage:

daemon := provisionerd.New(...)

ctx, cancelFunc := context.WithTimeout(context.Background(), 30*time.Second)
defer cancelFunc()
err := daemon.Shutdown(ctx)
if !errors.Is(err, context.DeadlineExceeded) {
  return err
}
err = daemon.Close()

This was detecting branches, but not our "main" branch before. Hopefully this fixes it!

This enables a consistent API for project import and provisioned resources.

This is a much cleaner abstraction. Explicitly declaring the user parameters for each provisioner makes for significantly simpler testing.

Terraform (or other provisioners) may need to cleanup state, or cancel actions before exit. This adds the ability to gracefully exit provisionerd.

codecov · 2022-02-27T20:49:11Z

Codecov Report

Merging #372 (d171e8a) into main (e5c9555) will decrease coverage by 0.17%.
The diff coverage is 66.12%.

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
- Coverage   68.19%   68.02%   -0.18%     
==========================================
  Files         148      148              
  Lines        8169     8271     +102     
  Branches       72       72              
==========================================
+ Hits         5571     5626      +55     
- Misses       2041     2085      +44     
- Partials      557      560       +3

Flag	Coverage Δ
unittest-go-macos-latest	`66.29% <62.79%> (-0.02%)`	⬇️
unittest-go-ubuntu-latest	`67.24% <62.84%> (-0.41%)`	⬇️
unittest-js	`66.10% <ø> (ø)`

Impacted Files	Coverage Δ
provisioner/echo/serve.go	`46.39% <0.00%> (-0.98%)`	⬇️
coderd/provisionerdaemons.go	`57.50% <22.72%> (-3.13%)`	⬇️
provisioner/terraform/serve.go	`63.63% <62.50%> (-5.12%)`	⬇️
provisionerd/provisionerd.go	`70.22% <71.00%> (+1.67%)`	⬆️
provisioner/terraform/provision.go	`74.08% <76.08%> (-0.57%)`	⬇️
coderd/provisionerjobs.go	`59.23% <100.00%> (-0.88%)`	⬇️
peer/channel.go	`82.45% <0.00%> (-1.76%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5c9555...d171e8a. Read the comment docs.

bryphe-coder · 2022-02-28T17:50:49Z

provisioner/terraform/provision.go

+// This couldn't use terraform-exec, because it doesn't support cancellation, and there didn't appear
+// to be a straight-forward way to add it.


Interesting! Thanks for the comment

bryphe-coder · 2022-02-28T17:52:05Z

provisioner/terraform/provision.go

-	terraform.SetStdout(writer)
-	t.logger.Debug(ctx, "running apply", slog.F("options", options))
-	err = terraform.Apply(ctx, options...)
+	t.logger.Debug(ctx, "running apply", slog.F("vars", len(vars)), slog.F("env", len(env)))


I imagine some of these env vars may contain sensitive data - is there anything we need to do to protect them? Does slog have the ability to obfuscate sensitive items?

I only printed the len here intentionally to obfuscate it!

bryphe-coder · 2022-02-28T17:53:21Z

provisioner/terraform/provision.go

+		// Terraform can fail and apply and still need to store it's state.
+		// In this case, we return Complete with an explicit error message.


Thanks for the description. Good call on catching this case where we would still need to pick up the statefile.

bryphe-coder · 2022-02-28T17:54:15Z

provisioner/terraform/provision.go

+			_ = cmd.Process.Signal(os.Kill)
+		}
+	}()
+	cmd.Stdout = stdout


Do we also care about Stderr here?

bryphe-coder

Good call on this! I just realized this was important when I started playing with the GCP provider... I'd hit Control+C to stop the terraform apply job and noted that it still updated the statefile - important for us to support that, too.

kylecarbs · 2022-02-28T18:24:03Z

Exactly! That and being able to gracefully shutdown provisionerd instances is pretty important. That way our CI can gracefully upgrade provisionerd instances, instead of purging them and messing up workspace state.

kylecarbs added 14 commits February 24, 2022 16:37

ci: Update DataDog GitHub branch to fallback to GITHUB_REF

48bcf2c

This was detecting branches, but not our "main" branch before. Hopefully this fixes it!

Add basic Terraform Provider

eb50cda

Rename post files to upload

773e38f

Add tests for resources

89ca1d7

Skip instance identity test

29b5740

Add tests for ensuring agent get's passed through properly

604e995

Fix linting errors

f8cab49

Merge branch 'main' into tfprovider

8f19f85

Add echo path

444109d

Fix agent authentication

108f1b5

fix: Convert all jobs to use a common resource and agent type

89b1887

This enables a consistent API for project import and provisioned resources.

Add "coder_workspace" data source

8de9e61

feat: Remove magical parameters from being injected

cb1c883

This is a much cleaner abstraction. Explicitly declaring the user parameters for each provisioner makes for significantly simpler testing.

feat: Add graceful exits to provisionerd

c55793c

Terraform (or other provisioners) may need to cleanup state, or cancel actions before exit. This adds the ability to gracefully exit provisionerd.

kylecarbs requested a review from bryphe-coder February 27, 2022 20:45

kylecarbs self-assigned this Feb 27, 2022

Fix cancel error check

dac812d

kylecarbs force-pushed the tfcancel branch from 99875b8 to dac812d Compare February 27, 2022 23:30

kylecarbs force-pushed the nomagicparams branch from cb1c883 to 9830266 Compare February 28, 2022 17:10

bryphe-coder reviewed Feb 28, 2022

View reviewed changes

bryphe-coder approved these changes Feb 28, 2022

View reviewed changes

Base automatically changed from nomagicparams to main February 28, 2022 18:26

Merge branch 'main' into tfcancel

d171e8a

kylecarbs enabled auto-merge (squash) February 28, 2022 18:32

kylecarbs merged commit 9d2803e into main Feb 28, 2022

kylecarbs deleted the tfcancel branch February 28, 2022 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add graceful exits to provisionerd #372

feat: Add graceful exits to provisionerd #372

kylecarbs commented Feb 27, 2022

codecov bot commented Feb 27, 2022 •

edited

bryphe-coder Feb 28, 2022

bryphe-coder Feb 28, 2022

kylecarbs Feb 28, 2022

bryphe-coder Feb 28, 2022

bryphe-coder Feb 28, 2022

bryphe-coder left a comment

kylecarbs commented Feb 28, 2022

		// This couldn't use terraform-exec, because it doesn't support cancellation, and there didn't appear
		// to be a straight-forward way to add it.

		// Terraform can fail and apply and still need to store it's state.
		// In this case, we return Complete with an explicit error message.

feat: Add graceful exits to provisionerd #372

feat: Add graceful exits to provisionerd #372

Conversation

kylecarbs commented Feb 27, 2022

codecov bot commented Feb 27, 2022 • edited

Codecov Report

bryphe-coder Feb 28, 2022

Choose a reason for hiding this comment

bryphe-coder Feb 28, 2022

Choose a reason for hiding this comment

kylecarbs Feb 28, 2022

Choose a reason for hiding this comment

bryphe-coder Feb 28, 2022

Choose a reason for hiding this comment

bryphe-coder Feb 28, 2022

Choose a reason for hiding this comment

bryphe-coder left a comment

Choose a reason for hiding this comment

kylecarbs commented Feb 28, 2022

codecov bot commented Feb 27, 2022 •

edited