fix: Atlantis ungraceful termination

[meringu]

what

By default, dumb init forward SIGTERM signals to all child processes. This causes Atlantis to shutdown ungracefully when running a Terraform command. The process tree looks as follows:

dumb-init -> atlantis -> sh -> terraform -> provider(s).

When the container runtime stops the container while Atlantis is running a Terraform command (E.g Kubernetes node scale down), dumb-init will forward the SIGTERM to all these processes in reverse order. Both Terraform and Atlantis will trap the signal and try to exit gracefully, however the provider will crash, resulting in Atlantis commenting the following on a PR:

Stopping operation...

Interrupt received.
Please wait for Terraform to exit or data loss may occur.
Gracefully shutting down...

╷
│ Error: execution halted
│ 
│ 
╵
╷
│ Error: execution halted
│ 
│ 
╵
╷
│ Error: Plugin did not respond
│ 
│   with time_sleep.wait_30_seconds,
│   on main.tf line 1, in resource "time_sleep" "wait_30_seconds":
│    1: resource "time_sleep" "wait_30_seconds" {
│ 
│ The plugin encountered an error, and failed to respond to the
│ plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may
│ contain more details.
╵

This is consistently reproducible by using the time_sleep resource (as above) and stopping the container during an apply.

The impact of this behaviour can be partial applies that need manual clean up, particularly in the case that the provider has created an asset, but has not finished waiting for it to converge. It will exist, but not be tracked in the statefile. Some crashes, like resource updates can be resolved just by planning and applying again.

why

There are a few options here, we've set the DUMB_INIT_SETSID=0 environment variable to all of our Atlantis instances, which has resolved this issue for us. Atlantis will finish the Terraform command before finishing scale down. We could also set the --single-child flag in the entrypoint script (might be cleaner, but was not as easy to test as adding this env var in my environment). More details on how dumb init works here: https://github.com/Yelp/dumb-init?tab=readme-ov-file#what-dumb-init-does. Another option would be to remove it entirely, and just use #!/bin/sh as the entrypoint.

tests

Tested within Kubernetes. I reproduced it by deleting a pod during an apply. Adding this env var then trying again results in Atlantis finishing the command then scaling down.

references

None, I debugged this yesterday. I have some strace output if anyone is interested. It shows that PID 1 (dumb-init)
is sending SIGTERM to all the child PIDs (Atlantis, Terraform, provider etc...)

[jamengual]

Could you add something to the docs about this change?

[meringu]

I can do @jamengual. I couldn't see any specific docs about Docker, do you have any recommendations where this would best fit? Or would a code comment suffice?

I was thinking, I might quickly test with --single-child. As it is specific to the entrypoint script, it might be nice to contain this within there, rather than proliferate the env var to all child processes.

[jamengual]

we could try that in the entry point and maybe add a comment there is all works

[meringu]

I've ended up moving dumb-init from the shebang to the exec. Setting --single-child in the shebang is not viable as the container runtime will use sh to start the container process, which results in #!/usr/bin/dumb-init --single-child /bin/sh being interpreted as /usr/bin/dumb-init '--single-child /bin/sh'.

The side effect is that if any /docker-entrypoint.d/ scripts fork a process into the background, they won't be cleaned up by dumb init, but this would have been the effect of setting --single-child anyway I believe.

I've tested this to make sure the SIGTERM is caught correctly in my environment to no longer kill the plugin in the middle of an apply, the auto prefixing of the atlantis binary works, and execution of /docker-entrypoint.d/ scripts still function as I expect.

[meringu]

I'm not entirely sure if there is any value in keeping dumb-init with this change. I believe one would need to overwrite the entrypoint script to take advantage of other features like the signal rewriting. It doesn't hurt leaving it in however.

[chenrui333]

Thanks @meringu!