This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320

Merged
andrewhsu merged 4 commits into
docker-archive:18.09from
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged

[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from
dani-docker:18.09-bk-3945401

Conversation

@dani-docker

@dani-dockerdani-docker commented Aug 12, 2019•
edited by thaJeztah
Loading

Copy link
Copy Markdown

addresses ENGCORE-945

backports of:

Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.
The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.

Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501

Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@dani-docker
dani-docker changed the base branch from master to 18.09August 12, 2019 20:17
@dani-dockerdani-docker changed the title 18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker changed the title [18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah added this to the 18.09.9 milestone Aug 13, 2019
@thaJeztah

thaJeztah commented Aug 13, 2019

Copy link
Copy Markdown
Member

No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?

https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console

20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah

thaJeztah commented Aug 13, 2019

Copy link
Copy Markdown
Member

Will probably have to cherry-pick moby#39575 as well

@thaJeztah

thaJeztah commented Aug 13, 2019

Copy link
Copy Markdown
Member

tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔

thaJeztah
thaJeztah reviewed Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go Outdated
@thaJeztahthaJeztah mentioned this pull request Aug 13, 2019
Merged
@dani-docker

dani-docker commented Aug 13, 2019

Copy link
Copy Markdown
Author

@thaJeztah
Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it

@thaJeztah

thaJeztah commented Aug 14, 2019

Copy link
Copy Markdown
Member

Experimental failures look like flaky tests

https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console

19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah

thaJeztah commented Aug 14, 2019

Copy link
Copy Markdown
Member

Windows is actually failing, but it's reporting as "green"; moby#39576

Can you cherry-pick moby#39575 ?

18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others added 4 commits August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah approved these changes Aug 14, 2019
View reviewed changes

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️

@thaJeztah

thaJeztah commented Aug 15, 2019

Copy link
Copy Markdown
Member

failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console

tracked through moby#39352

00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu approved these changes Aug 15, 2019
View reviewed changes

@andrewhsuandrewhsu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewhsu
andrewhsu merged commit 808ed02 into docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah mentioned this pull request Oct 8, 2019
Merged
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi
2 more reviewers
@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content

This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320
Merged
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged
[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Conversation
@dani-docker
@dani-dockerdani-docker
commented
Aug 12, 2019•

edited by thaJeztah

Loading

Copy link


Copy Markdown

addresses ENGCORE-945
backports of:
Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.

The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.
Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501
Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
@dani-docker
dani-docker
changed the base branch from
master
to
18.09August 12, 2019 20:17
@dani-dockerdani-docker
changed the title
18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker
changed the title
[18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah
added this to the 18.09.9 milestone
Aug 13, 2019
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
Will probably have to cherry-pick moby#39575 as well
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔
thaJeztah
thaJeztah
reviewed
Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go
Outdated

@thaJeztahthaJeztah
mentioned this pull request
Aug 13, 2019
 Merged

@dani-docker
dani-docker
commented
Aug 13, 2019

Copy link


Copy Markdown

Author
@thaJeztah

Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it
@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Experimental failures look like flaky tests
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console
19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Windows is actually failing, but it's reporting as "green"; moby#39576
Can you cherry-pick moby#39575 ?
18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others
added 4 commits
August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah
approved these changes
Aug 14, 2019
View reviewed changes
@thaJeztahthaJeztah
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️
@thaJeztah
thaJeztah
commented
Aug 15, 2019

Copy link


Copy Markdown

Member
failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console
tracked through moby#39352
00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu
approved these changes
Aug 15, 2019
View reviewed changes
@andrewhsuandrewhsu
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@andrewhsu
andrewhsu
merged commit 808ed02
into
docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah
mentioned this pull request
Oct 8, 2019
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi

2 more reviewers

@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320
Merged
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged
[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Conversation
@dani-docker
@dani-dockerdani-docker
commented
Aug 12, 2019•

edited by thaJeztah

Loading

Copy link


Copy Markdown

addresses ENGCORE-945
backports of:
Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.

The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.
Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501
Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
@dani-docker
dani-docker
changed the base branch from
master
to
18.09August 12, 2019 20:17
@dani-dockerdani-docker
changed the title
18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker
changed the title
[18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah
added this to the 18.09.9 milestone
Aug 13, 2019
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
Will probably have to cherry-pick moby#39575 as well
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔
thaJeztah
thaJeztah
reviewed
Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go
Outdated

@thaJeztahthaJeztah
mentioned this pull request
Aug 13, 2019
 Merged

@dani-docker
dani-docker
commented
Aug 13, 2019

Copy link


Copy Markdown

Author
@thaJeztah

Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it
@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Experimental failures look like flaky tests
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console
19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Windows is actually failing, but it's reporting as "green"; moby#39576
Can you cherry-pick moby#39575 ?
18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others
added 4 commits
August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah
approved these changes
Aug 14, 2019
View reviewed changes
@thaJeztahthaJeztah
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️
@thaJeztah
thaJeztah
commented
Aug 15, 2019

Copy link


Copy Markdown

Member
failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console
tracked through moby#39352
00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu
approved these changes
Aug 15, 2019
View reviewed changes
@andrewhsuandrewhsu
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@andrewhsu
andrewhsu
merged commit 808ed02
into
docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah
mentioned this pull request
Oct 8, 2019
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi

2 more reviewers

@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320
Merged
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged
[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Conversation
@dani-docker
@dani-dockerdani-docker
commented
Aug 12, 2019•

edited by thaJeztah

Loading

Copy link


Copy Markdown

addresses ENGCORE-945
backports of:
Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.

The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.
Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501
Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
@dani-docker
dani-docker
changed the base branch from
master
to
18.09August 12, 2019 20:17
@dani-dockerdani-docker
changed the title
18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker
changed the title
[18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah
added this to the 18.09.9 milestone
Aug 13, 2019
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
Will probably have to cherry-pick moby#39575 as well
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔
thaJeztah
thaJeztah
reviewed
Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go
Outdated

@thaJeztahthaJeztah
mentioned this pull request
Aug 13, 2019
 Merged

@dani-docker
dani-docker
commented
Aug 13, 2019

Copy link


Copy Markdown

Author
@thaJeztah

Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it
@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Experimental failures look like flaky tests
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console
19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Windows is actually failing, but it's reporting as "green"; moby#39576
Can you cherry-pick moby#39575 ?
18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others
added 4 commits
August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah
approved these changes
Aug 14, 2019
View reviewed changes
@thaJeztahthaJeztah
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️
@thaJeztah
thaJeztah
commented
Aug 15, 2019

Copy link


Copy Markdown

Member
failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console
tracked through moby#39352
00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu
approved these changes
Aug 15, 2019
View reviewed changes
@andrewhsuandrewhsu
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@andrewhsu
andrewhsu
merged commit 808ed02
into
docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah
mentioned this pull request
Oct 8, 2019
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi

2 more reviewers

@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content

This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320
Merged
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged
[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Conversation
@dani-docker
@dani-dockerdani-docker
commented
Aug 12, 2019•

edited by thaJeztah

Loading

Copy link


Copy Markdown

addresses ENGCORE-945
backports of:
Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.

The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.
Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501
Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
@dani-docker
dani-docker
changed the base branch from
master
to
18.09August 12, 2019 20:17
@dani-dockerdani-docker
changed the title
18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker
changed the title
[18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah
added this to the 18.09.9 milestone
Aug 13, 2019
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
Will probably have to cherry-pick moby#39575 as well
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔
thaJeztah
thaJeztah
reviewed
Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go
Outdated

@thaJeztahthaJeztah
mentioned this pull request
Aug 13, 2019
 Merged

@dani-docker
dani-docker
commented
Aug 13, 2019

Copy link


Copy Markdown

Author
@thaJeztah

Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it
@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Experimental failures look like flaky tests
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console
19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Windows is actually failing, but it's reporting as "green"; moby#39576
Can you cherry-pick moby#39575 ?
18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others
added 4 commits
August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah
approved these changes
Aug 14, 2019
View reviewed changes
@thaJeztahthaJeztah
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️
@thaJeztah
thaJeztah
commented
Aug 15, 2019

Copy link


Copy Markdown

Member
failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console
tracked through moby#39352
00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu
approved these changes
Aug 15, 2019
View reviewed changes
@andrewhsuandrewhsu
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@andrewhsu
andrewhsu
merged commit 808ed02
into
docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah
mentioned this pull request
Oct 8, 2019
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi

2 more reviewers

@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320
Merged
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged
[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Conversation
@dani-docker
@dani-dockerdani-docker
commented
Aug 12, 2019•

edited by thaJeztah

Loading

Copy link


Copy Markdown

addresses ENGCORE-945
backports of:
Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.

The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.
Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501
Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
@dani-docker
dani-docker
changed the base branch from
master
to
18.09August 12, 2019 20:17
@dani-dockerdani-docker
changed the title
18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker
changed the title
[18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah
added this to the 18.09.9 milestone
Aug 13, 2019
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
Will probably have to cherry-pick moby#39575 as well
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔
thaJeztah
thaJeztah
reviewed
Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go
Outdated

@thaJeztahthaJeztah
mentioned this pull request
Aug 13, 2019
 Merged

@dani-docker
dani-docker
commented
Aug 13, 2019

Copy link


Copy Markdown

Author
@thaJeztah

Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it
@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Experimental failures look like flaky tests
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console
19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Windows is actually failing, but it's reporting as "green"; moby#39576
Can you cherry-pick moby#39575 ?
18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others
added 4 commits
August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah
approved these changes
Aug 14, 2019
View reviewed changes
@thaJeztahthaJeztah
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️
@thaJeztah
thaJeztah
commented
Aug 15, 2019

Copy link


Copy Markdown

Member
failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console
tracked through moby#39352
00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu
approved these changes
Aug 15, 2019
View reviewed changes
@andrewhsuandrewhsu
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@andrewhsu
andrewhsu
merged commit 808ed02
into
docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah
mentioned this pull request
Oct 8, 2019
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi

2 more reviewers

@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320
Merged
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged
[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Conversation
@dani-docker
@dani-dockerdani-docker
commented
Aug 12, 2019•

edited by thaJeztah

Loading

Copy link


Copy Markdown

addresses ENGCORE-945
backports of:
Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.

The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.
Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501
Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
@dani-docker
dani-docker
changed the base branch from
master
to
18.09August 12, 2019 20:17
@dani-dockerdani-docker
changed the title
18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker
changed the title
[18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah
added this to the 18.09.9 milestone
Aug 13, 2019
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
Will probably have to cherry-pick moby#39575 as well
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔
thaJeztah
thaJeztah
reviewed
Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go
Outdated

@thaJeztahthaJeztah
mentioned this pull request
Aug 13, 2019
 Merged

@dani-docker
dani-docker
commented
Aug 13, 2019

Copy link


Copy Markdown

Author
@thaJeztah

Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it
@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Experimental failures look like flaky tests
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console
19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Windows is actually failing, but it's reporting as "green"; moby#39576
Can you cherry-pick moby#39575 ?
18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others
added 4 commits
August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah
approved these changes
Aug 14, 2019
View reviewed changes
@thaJeztahthaJeztah
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️
@thaJeztah
thaJeztah
commented
Aug 15, 2019

Copy link


Copy Markdown

Member
failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console
tracked through moby#39352
00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu
approved these changes
Aug 15, 2019
View reviewed changes
@andrewhsuandrewhsu
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@andrewhsu
andrewhsu
merged commit 808ed02
into
docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah
mentioned this pull request
Oct 8, 2019
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi

2 more reviewers

@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content

This repository was archived by the owner on Oct 13, 2023. It is now read-only.

[18.09 backport] do not stop health check before sending signal - #320
Merged
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Aug 15, 2019
Merged
[18.09 backport] do not stop health check before sending signal#320
andrewhsu merged 4 commits into
docker-archive:18.09from 
dani-docker:18.09-bk-3945401
Conversation
@dani-docker
@dani-dockerdani-docker
commented
Aug 12, 2019•

edited by thaJeztah

Loading

Copy link


Copy Markdown

addresses ENGCORE-945
backports of:
Just to summarize, the concern in this backport is the risk of leaking health monitor routines. Those routines need to be explicitly stopped.

The only other place where we explicitly stop those threads , is when we get an event from containerd, so as long as those events are delivered, we should be good.
Note, this is the change that introduced the original problem and you can see how the health monitor was explicitly stopped, (in addition to the containerd event)
moby#35501
Had a chat with @crosbymichael and @thaJeztah we decided to go ahead with the port , unless someone else has any concerns
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
@dani-docker
dani-docker
changed the base branch from
master
to
18.09August 12, 2019 20:17
@dani-dockerdani-docker
changed the title
18.09 bk 3945401[18.00 backport] do not stop health check before sending signalAug 12, 2019
@dani-dockerdani-docker
changed the title
[18.00 backport] do not stop health check before sending signal[18.09 backport] do not stop health check before sending signalAug 12, 2019
@thaJeztahthaJeztah
added this to the 18.09.9 milestone
Aug 13, 2019
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
No idea what weird stuff Jenkins is doing here; did you perhaps change the target branch after opening the PR?
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46508/console
https://jenkins.dockerproject.org/job/Docker-PRs/55419/console
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_x86.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mipsx.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_mips64x.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu_arm.go
20:17:42 Auto-merging vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/sys/cpu/cpu.go
20:17:42 Auto-merging vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (add/add): Merge conflict in vendor/golang.org/x/crypto/otr/otr.go
20:17:42 CONFLICT (rename/delete): vendor/go.etcd.io/bbolt/freelist.go deleted in HEAD and renamed in origin/pr/320. Version origin/pr/320 of vendor/go.etcd.io/bbolt/freelist.go left in tree.

@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
Will probably have to cherry-pick moby#39575 as well
@thaJeztah
thaJeztah
commented
Aug 13, 2019

Copy link


Copy Markdown

Member
tried restarting and manually changing BASE_BRANCH. Not sure if that sticks in Jenkins though 🤔
thaJeztah
thaJeztah
reviewed
Aug 13, 2019
View reviewed changes
Comment threadintegration/container/health_test.go
Outdated

@thaJeztahthaJeztah
mentioned this pull request
Aug 13, 2019
 Merged

@dani-docker
dani-docker
commented
Aug 13, 2019

Copy link


Copy Markdown

Author
@thaJeztah

Pushed the fix as a separate commit so we can keep the history trackable, let me know if you prefer to squash it
@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Experimental failures look like flaky tests
https://jenkins.dockerproject.org/job/Docker-PRs-experimental/46520/console
19:14:05 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
19:14:05 19:14:05 assertion failed: 19:14:05 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
19:14:05 ExitCode: 0
19:14:05 Error: <nil>
19:14:05 Stdout: 19:14:05 Stderr: 19:14:05 19:14:05 Failures:
19:14:05 ExitCode was 0 expected 11

19:16:34 FAIL: docker_cli_start_test.go:190: DockerSuite.TestStartReturnCorrectExitCode
19:16:34 19:16:34 assertion failed: expected an error, got nil

@thaJeztah
thaJeztah
commented
Aug 14, 2019

Copy link


Copy Markdown

Member
Windows is actually failing, but it's reporting as "green"; moby#39576
Can you cherry-pick moby#39575 ?
18:50:03 === RUN TestHealthKillContainer
18:50:12 --- FAIL: TestHealthKillContainer (9.30s)
18:50:12 health_test.go:57: assertion failed: error is not nil: Error response from daemon: Invalid signal: SIGUSR1

Ruilin Liand others
added 4 commits
August 14, 2019 16:44
Ruilin LiDani Louca
do not stop health check before sending signal
afee55c
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>
(cherry picked from commit da574f9)
Signed-off-by: Dani Louca <dani.louca@docker.com>
@cpuguy83Dani Louca
Move kill health test to integration
4cad9dd
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
(cherry picked from commit f8aef6a)
Signed-off-by: Dani Louca <dani.louca@docker.com>
Dani Louca
Fixing integration test
614daf1
Signed-off-by: Dani Louca <dani.louca@docker.com>
@thaJeztahDani Louca
Skip TestHealthKillContainer on Windows
f481d4c
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>
thaJeztah
thaJeztah
approved these changes
Aug 14, 2019
View reviewed changes
@thaJeztahthaJeztah
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ❤️
@thaJeztah
thaJeztah
commented
Aug 15, 2019

Copy link


Copy Markdown

Member
failure is a flaky test https://jenkins.dockerproject.org/job/Docker-PRs/55457/console
tracked through moby#39352
00:24:06 FAIL: docker_cli_run_test.go:1792: DockerSuite.TestRunInteractiveWithRestartPolicy
00:24:06 00:24:06 assertion failed: 00:24:06 Command: /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
00:24:06 ExitCode: 0
00:24:06 Error: <nil>
00:24:06 Stdout: 00:24:06 Stderr: 00:24:06 00:24:06 Failures:
00:24:06 ExitCode was 0 expected 11

andrewhsu
andrewhsu
approved these changes
Aug 15, 2019
View reviewed changes
@andrewhsuandrewhsu
left a comment


Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@andrewhsu
andrewhsu
merged commit 808ed02
into
docker-archive:18.09Aug 15, 2019
@thaJeztahthaJeztah
mentioned this pull request
Oct 8, 2019
 Merged

Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tonistiigitonistiigiAwaiting requested review from tonistiigi

2 more reviewers

@andrewhsuandrewhsuandrewhsu approved these changes
@thaJeztahthaJeztahthaJeztah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

18.09.9

Development

Successfully merging this pull request may close these issues.

4 participants

@dani-docker@thaJeztah@andrewhsu@cpuguy83