Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

make Monitor shut down the process hard if it fails to shutdown grace… #1717

Merged
merged 4 commits into from
Dec 19, 2016

Conversation

mwitkow
Copy link
Contributor

@mwitkow mwitkow commented Dec 8, 2016

Should fix:
#1716

@jonboulle
Copy link
Contributor

Interesting catch. I wonder if we should just panic or something similar to dump a stack trace in this situation?

@mwitkow
Copy link
Contributor Author

mwitkow commented Dec 9, 2016

Hmm, panic would work, but I'm wondering why the tests are failing:

--- FAIL: TestNodeShutdown (9.70s)
	node_test.go:83: Unit hello.service not reported as inactive:

@dongsupark
Copy link
Contributor

@mwitkow That test failure looks just intermittent. If you just trigger another round of functional tests, that failure could disappear. (Yes I know, it is annoying.)

@mwitkow
Copy link
Contributor Author

mwitkow commented Dec 9, 2016

@dongsupark thanks, stupid question though: how do I trigger semaphore? Do i need to push another thing?

@dongsupark
Copy link
Contributor

dongsupark commented Dec 9, 2016

@mwitkow Right. Change something, commit it, and force-push it.
If you don't want to change anything, run "git commit --amend" to append a space to the commit message, to be able to force-push it again. That should do the trick.

@dongsupark
Copy link
Contributor

@mwitkow Apart from the test failure on semaphoreci, my local test works fine.
I also did run tests with the branch rebased on master, which works also fine for both gRPC and !gRPC.
I think, what needs to be done is only to call panic() instead of os.Exit(1). Just let me know, once you change that. Then I could merge.

@dongsupark
Copy link
Contributor

@mwitkow gentle ping.
I'd like to get it merged in this week, before release v1.0. Should I just merge this one first, then create another PR to replace os.Exit(1) with panic()?

@mwitkow
Copy link
Contributor Author

mwitkow commented Dec 19, 2016

@dongsupark thanks for the ping, I changed it to panic with a message.

@dongsupark
Copy link
Contributor

LGTM. Thanks for the bug report and its fix! 👍

@dongsupark dongsupark merged commit 190bcbe into coreos:master Dec 19, 2016
dongsupark pushed a commit that referenced this pull request Dec 19, 2016
make Monitor shut down the process hard if it fails to shutdown grace…
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants