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.

Sign up for GitHub

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

Jump to bottom

Shutdown Subcommands with SIGTERM and SIGINT #389

Merged
merged 10 commits into from
Nov 14, 2020
Merged

Shutdown Subcommands with SIGTERM and SIGINT #389

merged 10 commits into from
Nov 14, 2020

Conversation

lawliet89
Copy link
Contributor

@lawliet89 lawliet89 commented Nov 11, 2020 •
edited
Loading

Changes proposed in this PR:

  • Kubernetes sends SIGTERM to shut down containers.
  • Other tools like Argo also adopts the same behaviour to shut containers down
  • Connect Lifecycle Sidecar should also listen to SIGTERM for shutting down

How I've tested this PR:

  • Pending review

How I expect reviewers to test this PR:

  • Start a connect injected pod and check logs for SIGTERM when pod is deleted

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@lawliet89
Shutdown Connect Lifecycle Sidecar with SIGTERM
30ea4a8 
- Kubernetes sends SIGTERM to shut down containers.
- Other tools like Argo also adopts the same behaviour to shut
container down
- Connect Lifecycle Sidecar should also listen to SIGTERM for shutting
down
thisisnotashwin
thisisnotashwin reviewed Nov 11, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Thanks so much for the PR!! This looks good. I just have one suggestion.

subcommand/lifecycle-sidecar/command.go Outdated Show resolved Hide resolved
thisisnotashwin
thisisnotashwin reviewed Nov 11, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

I think we perform similar signal handling in the following files as well:
• webhook-cert-manager/command.go
• inject-connect/command.go
• sync-catalog/command.go

Could you update them as well as a part of this PR so that we are consistent in our approach to signal handling across the repo. Thanks @lawliet89

thisisnotashwin
thisisnotashwin reviewed Nov 11, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

One last thing, could you include unit tests similar to what we have here:

consul-k8s/subcommand/inject-connect/command_test.go

Line 211 in 2b86fdd

func TestRun_CommandExitsCleanlyAfterSigInt(t *testing.T) {
just to verify this behavior

@lawliet89
Add tests
855da2b 
- Fix logging and add test for lifecycle-sidecar
@lawliet89 lawliet89 changed the title Shutdown Connect Lifecycle Sidecar with SIGTERM Shutdown Subcommands with SIGTERM and SIGINT Nov 12, 2020
@lawliet89
Copy link
Contributor Author

@ashwin-venkatesh I have added SIGTERM handling to the subcommands you listed and have added tests for all of them.

thisisnotashwin
thisisnotashwin reviewed Nov 12, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for adding the tests but they will need some assertions on them to ensure the signal handling works as expected.

subcommand/lifecycle-sidecar/command_test.go Outdated Show resolved Hide resolved
subcommand/sync-catalog/command_test.go Outdated Show resolved Hide resolved
subcommand/webhook-cert-manager/command_test.go Outdated Show resolved Hide resolved
@lawliet89 lawliet89 force-pushed the sigterm branch 5 times, most recently from 510415f to b0e13a5 Compare November 13, 2020 01:50
@lawliet89
Add check for exit
c54699e 
- 10s for sync catalog since it needs more time
@lawliet89
Copy link
Contributor Author

@ashwin-venkatesh I have added the assertions you suggested. I gave sync catalog more time because it doesn't seem to be able to exit within 1s.

thisisnotashwin
thisisnotashwin reviewed Nov 13, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Ok I have the last few suggestions but I think we will be in a position to merge this once those changes are made! Thanks @lawliet89

subcommand/inject-connect/command_test.go Outdated Show resolved Hide resolved
subcommand/lifecycle-sidecar/command_test.go Show resolved Hide resolved
subcommand/sync-catalog/command_test.go Outdated Show resolved Hide resolved
subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
thisisnotashwin
thisisnotashwin approved these changes Nov 14, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

🥳 Excellent!!

@thisisnotashwin thisisnotashwin merged commit 239a2db into hashicorp:master Nov 14, 2020
@thisisnotashwin
Copy link
Contributor

Thanks @lawliet89 !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lawliet89 @thisisnotashwin