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

Handle SIGTERM when running commands #827

Merged
merged 1 commit into from
Apr 1, 2015

Conversation

mfischer-zd
Copy link
Contributor

Make Consul treat SIGTERM like it does SIGINT when running commands.
This is especially important when running Consul as a daemon, since
Unix process managers send SIGTERM to restart or terminate a process.

This change is untested on Windows.

Fixes #797

@ryanbreen
Copy link
Contributor

Cool! Does this need testing on Windows? I didn't think they had SIGTERM over there.

@mfischer-zd
Copy link
Contributor Author

Yes, please. I'm hoping that the new line is a harmless no-op on that platform, but I can't say as I don't run Windows.

@ryanbreen
Copy link
Contributor

Ahh, ok, the question is more "does Windows explode as a result of this patch?" than "is SIGTERM handled properly on Windows?" IIRC, the latter would be impossible to test since Windows doesn't have SIGTERM, but someone with more Windows experience would know for sure.

@highlyunavailable
Copy link
Contributor

It builds on Windows, at least. Usually there's just build failures if something isn't implemented.

I'm running the tests now.

@highlyunavailable
Copy link
Contributor

Also FYI Notify is a variadic function, so signal.Notify(signalCh, os.Interrupt, syscall.SIGTERM) should be valid (see example).

@mfischer-zd
Copy link
Contributor Author

Mmm, good point @highlyunavailable. If the tests pass, I'll compact the code and update the PR.

@highlyunavailable
Copy link
Contributor

From the look of things this won't break on Windows, there just is no instance in which there will be the syscall.SIGTERM event.

The tests failed all sorts of ways but it doesn't look like anything you did. Mostly Raft couldn't start: agent_test.go:72: err: Failed to start Consul server: Failed to start Raft: There is not enough space on the disk. (20 GB free, though, so maybe this is something to do with memory mapped files)

@mfischer-zd
Copy link
Contributor Author

Those tests are failing on the master branch too, so let's see what happens when those are fixed.

Make Consul treat SIGTERM like it does SIGINT when running commands.
This is especially important when running Consul as a daemon, since
Unix process managers send SIGTERM to restart or terminate a process.

This change is untested on Windows.

Fixes hashicorp#797
@mfischer-zd
Copy link
Contributor Author

Updated commit to use an additional argument to signal.Notify() instead of making another call to it.

@ryanuber
Copy link
Member

ryanuber commented Apr 1, 2015

Thanks, @mfischer-zd. We do already have some handling for SIGTERM here. The behavior of SIGTERM is configurable. If what you're after is a graceful leave, you can set the leave_on_terminate config option, which should cause Consul to invoke the same code path as SIGINT.

@mfischer-zd
Copy link
Contributor Author

@ryanuber, this patch corrects behavior of consul lock and other tools - it's not related to the behavior of Consul in agent mode. See #797 for further discussion.

@ryanuber
Copy link
Member

ryanuber commented Apr 1, 2015

@mfischer-zd ah that makes sense then, merging this in. The test failures are definitely unrelated to this change. I'll address those separately.

ryanuber added a commit that referenced this pull request Apr 1, 2015
Handle SIGTERM when running commands
@ryanuber ryanuber merged commit c0178a8 into hashicorp:master Apr 1, 2015
duckhan pushed a commit to duckhan/consul that referenced this pull request Oct 24, 2021
* Add changelog entry for ingress gw change
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.

consul lock should pass TERM signals to its children
4 participants