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

cmd/contour: hook SIGHUP to gracefully shut down contour serve #1364

Closed
davecheney opened this issue Aug 21, 2019 · 6 comments · Fixed by #1382
Closed

cmd/contour: hook SIGHUP to gracefully shut down contour serve #1364

davecheney opened this issue Aug 21, 2019 · 6 comments · Fixed by #1382
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@davecheney
Copy link
Contributor

contour serve runs until one of the members of serve's workgroup exits, shutting down the rest of the group.

For testing we sometimes manually hack in poison pill style jobs which terminate the group after a period of time #1361. It would be useful to be able to a graceful server shutdown rather than ^C the process. I propose we hook up SIGHUP to terminate the group.

@davecheney davecheney added kind/feature Categorizes issue or PR as related to a new feature. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Aug 21, 2019
@davecheney davecheney added this to the 1.0.0-beta.1 milestone Aug 21, 2019
@alexbrand
Copy link
Contributor

+1. This would also allow graceful shutdown by the Kubelet when terminating contour pods.

@alexbrand
Copy link
Contributor

I'd love to pick this up. I've got a couple of questions:

  • Kubernetes sends a SIGTERM and not a SIGHUP when terminating a pod. I was thinking we could hook up SIGTERM instead of SIGHUP. Thoughts?

  • Is a new goroutine in the Workgroup the right place for implementing this? I am thinking g.Add a new goroutine in serve.go that setups the signal handler and blocks on the notification channel. Once the signal is handled, the goroutine logs and returns nil.

@davecheney
Copy link
Contributor Author

@youngnick any objections to SIGTERM?

@youngnick
Copy link
Member

No objections to SIGTERM, particularly since we're shutting the program down as the response anyway. Probably better not to overload SIGHUP (which has the expected behaviour of leaving the program running) anyway.

Good idea.

@davecheney
Copy link
Contributor Author

@alexbrand all yours next week once we start the next cycle.

@davecheney
Copy link
Contributor Author

Is a new goroutine in the Workgroup the right place for implementing this? I am thinking g.Add a new goroutine in serve.go that setups the signal handler and blocks on the notification channel. Once the signal is handled, the goroutine logs and returns nil.

Yup. That's a good plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants