Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canceling Terraform Command #301

Closed
wants to merge 3 commits into from
Closed

Conversation

lornasong
Copy link
Member

@lornasong lornasong commented May 9, 2022

This PR:

  • Fixes an issue where canceling context was not killing the Terraform command as expected
  • Adds a workaround for a secondary Golang issue where canceling a command when Stdout and Stderr are set can cause hanging.
    • Note: I was a little surprised that that the hanging issue doesn't impact all versions of TF i.e. some versions of TF pass but others fail

Commits (see message for more details, sample logs, and progression of working through issue)

  1. Add timeout TestContext_sleepTimeoutExpired to check if the Terraform Apply is killed within a reasonable timeframe of the cancel (test fails because terraform apply is not killed)
  2. Add fix so that Terraform Apply is killed. This introduces the hanging issue (test fails because of hanging)
  3. Add workaround for hanging (test passes)

…timeframe

Add a timeout to the test to ensure that the terraform apply cancels within
a reasonable time of the 5s timeout.

Currently, this test is not canceling the terraform apply as expected. In the
logs you can see that the test takes 1 min rather than ~5s:
```
    --- PASS: TestContext_sleepTimeoutExpired/sleep-0.12.31 (62.13s)
```

```
=== RUN   TestContext_sleepTimeoutExpired/sleep-0.12.31
    util_test.go:113: [INFO] running Terraform command: /var/folders/6y/gy9gggt14379c_k39vwb50lc0000gn/T/terraform_1378921380/terraform apply -no-color -auto-approve -input=false -lock=true -parallelism=10 -refresh=true
    util_test.go:103: CLI Output:
	    // truncated ...
        time_sleep.sleep: Creating...
        time_sleep.sleep: Still creating... [10s elapsed]
        time_sleep.sleep: Still creating... [20s elapsed]
        time_sleep.sleep: Still creating... [30s elapsed]
        time_sleep.sleep: Still creating... [41s elapsed]
        time_sleep.sleep: Still creating... [51s elapsed]
        time_sleep.sleep: Creation complete after 1m0s [id=2022-05-06T17:40:20Z]

        Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
```
Processes were not being killed because cmd.ProcessState was nil. With this
change, processes will be able to make the request to Kill().

Added a temporary log to printout cmd.ProcessState to demonstrate. Will be
removed in next commit.

Note: this will cause hanging `TestContext_sleepTimeoutExpired` due to a known
Golang issue with killing a command when Stdout or Stderr are set to anything
besides `nil` or `*os.File`. This is because the Kill does not notify the
stdout/stderr subprocesses to stop. `cmd.Wait` (called by `cmd.Run`) waits
indefinitely for those subprocesses to stop.
`TestContext_sleepTimeoutExpired` can occasionally hang when killing a command
that has Stdout or Stderr set to anything besides `nil` or `*os.File`.
golang/go#23019

Use workaround to read from StdoutPipe and StderrPipe rather than setting
Stdout / Stderr
@lornasong lornasong marked this pull request as ready for review May 10, 2022 19:37
@radeksimko radeksimko requested review from radeksimko and removed request for radeksimko May 11, 2022 09:32
@lornasong lornasong closed this May 17, 2022
@lornasong
Copy link
Member Author

Closed since it is a sub-set of changes in #299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant