Repository navigation
fix(provisioner/terraform): interrupt terraform again before killing it - #31685
Conversation
When a canceled Terraform command is still running after ExitTimeout, the provisioner sent SIGKILL to Terraform. The kill skips the plugin cleanup of Terraform, so provider plugins kept running. A leftover provider holds its binary open, and a later "terraform init" that replaces that binary fails with "text file busy". Terraform exits at once on a second interrupt and stops its provider plugins before it exits. Now the provisioner sends a second interrupt when ExitTimeout expires, and kills Terraform only if it is still running 10 seconds later (or after ExitTimeout, if that is shorter). Windows is unchanged because it does not support interrupts. Refs #14726
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mafredri
left a comment
There was a problem hiding this comment.
One correctness concern (inline), plus a few nits. The fix itself works: with real terraform 1.16.2 and hashicorp/external 2.3.5 stuck in a data source at the default 3m ExitTimeout, this PR exits with exit status 1 after "Two interrupts received" and the provider is gone. On main, the run ends with signal: killed and the provider is left running. ExitsOnSecondInterrupt also fails on main and passes here, so it does test the change. Didn't look at Windows beyond reading the code; nothing changes there.
Nits:
- The
forceExitTimeoutdoc says terraform exits within seconds of a second interrupt. That's only true forplanandapply(RunOperation).inithandles only the first interrupt, andshow/graphdon't handle interrupts at all, so those still get killed, just 10s later than before. ServeOptions.ExitTimeoutrepeats "10 seconds" as a literal instead of pointing atforceExitTimeout.- The
HungJobExitTimeoutcomment injobreaperstill describes 3m as the limit. The real total is now 3m10s, which is still underHungJobDuration. - A timed-out cancel now reports
terraform plan: exit status 1instead ofsignal: killed, which reads like an ordinary terraform failure. No code matches on the old string. Perhaps wrap the error whenkillCtx.Err() != nil?
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Relates to #14726
Relates to https://linear.app/codercom/issue/ENG-745
Relates to https://linear.app/codercom/issue/CORE-807
I tracked a run of
text file busyfailures on dogfood down to a single provisioner pod. Acoder-katabuild hung creating akubernetes_pod_v1and was cancelled. We sent terraform a SIGINT, waited out the 3 minuteExitTimeout, and then SIGKILLed it. The kill skips terraform's plugin cleanup, and providers ignore SIGINT, so the idleterraform-provider-coderit had started just kept running.coder-katahas no lock file, so eachterraform initreinstalls the provider into the shared plugin cache, and everycoder-katabuild on that pod afterwards failed with:To fix, we'll interrupt terraform a second time instead of killing it straight away. On a second interrupt, terraform cancels the operation and returns within 5 seconds (meta.go), which lets its deferred
plugin.CleanupClients()stop the providers (main.go). If terraform is still running 10 seconds later, we kill it like before.This is the same
cmd.Cancelandcmd.WaitDelayapproach terraform-exec uses to stop terraform (cmd.go), the only difference being that we've already sent the first interrupt by the timekillCtxis done. I checked this against a real terraformplanstuck in a data source: it kept running after the first SIGINT, and exited about 50ms after the second, with the provider gone.Terraform does warn that data loss may have occurred on a second interrupt, but that's still better than the SIGKILL we send today. This won't help if terraform panics or gets OOM killed (hashicorp/terraform#39399), and Windows is unchanged, since it can't be interrupted.
We already capture a stack trace from the provider when
terraform inithitstext file busy, but that broke when we started runninginitwith-json, since terraform then reports the error on stdout instead of stderr. I'm fixing that in #31688.