search

coreos / fleet

fleet ties together systemd and etcd into a distributed init system
Apache License 2.0
2.42k stars 302 forks source link

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

Closed mwitkow closed 7 years ago

mwitkow commented 7 years ago

Should fix: https://github.com/coreos/fleet/issues/1716

jonboulle commented 7 years ago

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

mwitkow commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

dongsupark commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

dongsupark commented 7 years ago

LGTM. Thanks for the bug report and its fix! :+1: