Skip to content

Enable signal handlers per default for LocalStack inside a docker container#6000

Merged
dfangl merged 2 commits intomasterfrom
docker-signal-handling
May 3, 2022
Merged

Enable signal handlers per default for LocalStack inside a docker container#6000
dfangl merged 2 commits intomasterfrom
docker-signal-handling

Conversation

@dfangl
Copy link
Member

@dfangl dfangl commented May 3, 2022

Currently, passing signals from the LS docker container init process (= docker-entrypoint.sh) to supervisord (which will in turn pass it to the LocalStack process itself), is disabled per default, unless you set SET_TERM_HANDLER=1.
This results in long shutdown times for docker-compose, as well as leftover containers started by LS (like lambda containers etc.), since docker-compose will kill containers after 10s if they do not end themselves in that timeframe.
This behavior is not seen in LS CLI, since we set the term handler variable explicitely there.

This PR inverts this behavior, signal handling is enabled per default, unless DISABLE_TERM_HANDLER=1 is set, for users who, for any reason, would like to use the "old" behavior.

The SET_TERM_HANDLER variable setting in CLI is still in there for now, in order to prevent bricking instances where a new CLI meets an older docker image (should be removed after a considerate amount of time).

This PR results in way faster stop times in docker-compose (<2s instead of >10s), as well as a possible cleanup of resources, and possibilities for persistence.

@dfangl dfangl temporarily deployed to localstack-ext-tests May 3, 2022 13:45 Inactive
@github-actions
Copy link

github-actions bot commented May 3, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@dfangl
Copy link
Member Author

dfangl commented May 3, 2022

I have read the CLA Document and I hereby sign the CLA

@dfangl dfangl requested a review from thrau May 3, 2022 13:56
@github-actions
Copy link

github-actions bot commented May 3, 2022

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 0m 33s ⏱️ + 3m 5s
   982 tests ±0     942 ✔️ ±0  40 💤 ±0  0 ±0 
1 206 runs  ±0  1 138 ✔️ ±0  68 💤 ±0  0 ±0 

Results for commit d7da98a. ± Comparison against base commit 0f8d040.

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+docs,awesome! 💯

@dfangl dfangl merged commit 38b35e7 into master May 3, 2022
@dfangl dfangl deleted the docker-signal-handling branch May 3, 2022 17:10
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants