do not stop health check before sending signal#39454
Merged
thaJeztah merged 2 commits intomoby:masterfrom Jul 14, 2019
Merged
Conversation
Member
|
🤦♂ sorry overlooked this; looks like you added a test to the old integration-cli suite, but it should be in the |
Contributor
Author
|
Then why does the old one even exist seb? |
Member
Contributor
Author
|
I don't think i'll move it, its just another half baked move to create more work. |
Member
|
It's "half baked" b/c it is indeed a large body of work with potential of causing regressions during move. |
Member
|
I don't think I understand the logic here. You can't add a new health check fix unless you add a health check test, but you can't add a health check test unless you also take the responsibility of porting ALL the health check tests to a new test framework? |
Member
|
No, we just don't want to add a new test to integration-cli. |
Member
|
My recommendation, merge this and migrate all the health checks as a whole in a separate change. The functions used to implement this test are just like the other health check tests and defined in that package, moving the test would be equivalent to migrating. |
Member
|
We have not accepted new tests in integration-cli for quite some time, why would we add a new test now? |
Member
|
Updated to move the test. |
Member
|
Hmmm... something became more noisy in the tests; Details |
Member
|
Yes, I see that in other PR, same exact test case. |
Member
|
I see something interesting there; looks like the daemon arguments do not override, but append to the defaults, so adding Guess we should filter duplicates when starting the API. I'll open a PR |
Docker daemon always stops healthcheck before sending signal to a container now. However, when we use "docker kill" to send signals other than SIGTERM or SIGKILL to a container, such as SIGINT, daemon still stops container health check though container process handles the signal normally and continues to work. Signed-off-by: Ruilin Li <liruilin4@huawei.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Member
|
rebased to get the testing fixes in from master |
This was referenced Jul 19, 2019
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Jul 19, 2019
This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in moby#39454, but rewritten in a commit in the same PR: moby@f8aef6a In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Contributor
|
One q: Should we assert on something else after moby/integration/container/health_test.go Lines 59 to 61 in f1b5612 |
Member
|
The test should fail if the status is not healthy within the allotted 30s. |
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Jul 19, 2019
This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in moby/moby#39454, but rewritten in a commit in the same PR: moby/moby@f8aef6a In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: eeaa0b30d47e6b9dac8d8ea2ced6d5ce44c24463 Component: engine
This was referenced Aug 12, 2019
dani-docker
pushed a commit
to dani-docker/moby
that referenced
this pull request
Aug 14, 2019
This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in moby#39454, but rewritten in a commit in the same PR: moby@f8aef6a In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit eeaa0b3) Signed-off-by: Dani Louca <dani.louca@docker.com>
dani-docker
pushed a commit
to dani-docker/moby
that referenced
this pull request
Aug 14, 2019
This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in moby#39454, but rewritten in a commit in the same PR: moby@f8aef6a In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit eeaa0b3) Signed-off-by: Dani Louca <dani.louca@docker.com>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Aug 15, 2019
This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in moby/moby#39454, but rewritten in a commit in the same PR: moby/moby@f8aef6a In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit eeaa0b30d47e6b9dac8d8ea2ced6d5ce44c24463) Signed-off-by: Dani Louca <dani.louca@docker.com> Upstream-commit: f481d4c02366093b337e9aebfbbf23b1ff3968fe Component: engine
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Aug 15, 2019
This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in moby/moby#39454, but rewritten in a commit in the same PR: moby/moby@f8aef6a In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit eeaa0b30d47e6b9dac8d8ea2ced6d5ce44c24463) Signed-off-by: Dani Louca <dani.louca@docker.com> Upstream-commit: ef5dd6e46de6930b7adb2715ebc0473f25ba7995 Component: engine
2 tasks
burnMyDread
pushed a commit
to burnMyDread/moby
that referenced
this pull request
Oct 21, 2019
This test is failing on Windows currently: ``` 11:59:47 --- FAIL: TestHealthKillContainer (8.12s) 11:59:47 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1 `` That test was added recently in moby#39454, but rewritten in a commit in the same PR: moby@f8aef6a In that rewrite, there were some changes: - originally it was skipped on Windows, but the rewritten test doesn't have that skip: ```go testRequires(c, DaemonIsLinux) // busybox doesn't work on Windows ``` - the original test used `SIGINT`, but the new one uses `SIGUSR1` Analysis: - The Error bubbles up from: https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal.go#L29-L44 - Interestingly; `ContainerKill` should validate if a signal is valid for the given platform, but somehow we don't hit that part; https://github.com/moby/moby/blob/f1b5612f2008827fdcf838abb4539064c682181e/daemon/kill.go#L40-L48 - Windows only looks to support 2 signals currently https://github.com/moby/moby/blob/8e610b2b55bfd1bfa9436ab110d311f5e8a74dcb/pkg/signal/signal_windows.go#L17-L26 - Upstream Golang looks to define `SIGINT` as well; https://github.com/golang/go/blob/77f9b2728eb08456899e6500328e00ec4829dddf/src/runtime/defs_windows.go#L44 - This looks like the current list of Signals upstream in Go; https://github.com/golang/sys/blob/3b58ed4ad3395d483fc92d5d14123ce2c3581fec/windows/types_windows.go#L52-L67 ```go const ( // More invented values for signals SIGHUP = Signal(0x1) SIGINT = Signal(0x2) SIGQUIT = Signal(0x3) SIGILL = Signal(0x4) SIGTRAP = Signal(0x5) SIGABRT = Signal(0x6) SIGBUS = Signal(0x7) SIGFPE = Signal(0x8) SIGKILL = Signal(0x9) SIGSEGV = Signal(0xb) SIGPIPE = Signal(0xd) SIGALRM = Signal(0xe) SIGTERM = Signal(0xf) ) ``` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: zach <Zachary.Joyner@linux.com>
|
@crosbymichael my healthcheck has a notification when check failed. when docker stop , it does not stop the health check progress , but stop the container first , this caused healthcheck check failed and send a notification, but it should not because it's a stop progress. It would be fine , if the health check progress stop first and then stop the conatiner .
example code from #36233
It would be better we stop health check progress first and destory the container How can I fix this for now . by the way , I am using docker swarm . |
|
I'm facing the same issue as @suconghou , will it be fixed on v20.03.0? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docker daemon always stops healthcheck before sending signal to a
container now. However, when we use "docker kill" to send signals
other than SIGTERM or SIGKILL to a container, such as SIGINT,
daemon still stops container health check though container process
handles the signal normally and continues to work.
carry and closes #37263
Signed-off-by: Ruilin Li liruilin4@huawei.com