Ignore SIGURG on Linux. by cpuguy83 · Pull Request #2929 · docker/cli

cpuguy83 · 2021-01-13T18:01:44Z

In go1.14+, SIGURG is used by the runtime to handle preemtable system
calls.
In practice this signal is caught frequently.

For reference:

https://go.googlesource.com/proposal/+/master/design/24543-non-cooperative-preemption.md
golang/go#37942

codecov-io · 2021-01-13T18:30:45Z

Codecov Report

Merging #2929 (fff164c) into master (9a3fdc1) will increase coverage by 0.06%.
The diff coverage is 78.57%.

@@ Coverage Diff @@## master #2929 +/- ##
==========================================
+ Coverage 57.04% 57.10% +0.06% 
==========================================
Files 297 299 +2 Lines 18655 18665 +10 ==========================================
+ Hits 10641 10658 +17 + Misses 7154 7146 -8 - Partials 860 861 +1

silvin-lubecki

Thank you for this PR @cpuguy83 👍 Code looks good, but could we add a unit test on this new behavior ? 🙏

cpuguy83 · 2021-01-14T15:22:27Z

I'm not sure we can.

Since it is dealing with signals it is a bit more tricky in general since it is a global resource
Once we upgrade go, it is even more tricky since go used the signal internally.

silvin-lubecki · 2021-01-14T18:10:25Z

I wonder if we can play with the channel returned by the ForwardAllSignals, sending the signal via this way?

ch:=ForwardAllSignals(ctx, fakeCli, "id")
ch<-unix.SIGURG// check fakeCli.ContainerKill was not called

WDYT?

cpuguy83 · 2021-01-14T18:17:02Z

I think unless we pass in a <-chan os.Signal (which we won't wire up to the real signals) to ForwardAllSignals we can easily end up with a blocked channel after upgrading go in the test.

thaJeztah · 2021-01-14T18:37:55Z

Would (some of) these be useful to be included in https://github.com/moby/moby/tree/master/pkg/signal ? Or too unrelated to that package?

cpuguy83 · 2021-01-14T18:56:22Z

We could probably add an except ...os.Signal to the CatchAll function.

cpuguy83 · 2021-01-14T18:58:24Z

But I really wanted to not modify that package to fix the CLI because dependency hell.

cpuguy83 · 2021-01-14T23:21:15Z

Updated this to add a couple of tests.
I did have to refactor ForwardAllSignals to receive a channel instead of returning one.

silvin-lubecki

LGTM

silvin-lubecki · 2021-01-15T09:26:29Z

@cpuguy83 the linter is complaining:

cli/command/container/signals_test.go:38:17: SA1025: it is not possible to use Reset's return value correctly, as there is a race condition between draining the channel and the new timer expiring (staticcheck)
if !timer.Reset(30 * time.Second) {

In go1.14+, SIGURG is used by the runtime to handle preemtable system calls. In practice this signal caught *frequently*. For reference: https://go.googlesource.com/proposal/+/master/design/24543-non-cooperative-preemption.md golang/go#37942 Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2021-01-15T19:15:21Z

Fixed lint issue.

Now flakey test?

=== Failed
=== FAIL: cli/command/container TestRemoveForce/without_force (0.00s)
Error: Error: No such container: nosuchcontainer
--- FAIL: TestRemoveForce/without_force (0.00s)
rm_test.go:37: assertion failed: --- removed
+++ →
{[]string}[?->0]:
-: <non-existent>
+: "mycontainer"

thaJeztah · 2021-01-16T00:39:53Z

ah, interesting; it was reported in #2882 to be flaky; so far don't have a clue yet why (perhaps you can spot the issue?)

thaJeztah · 2021-01-16T00:41:10Z

kicked CI

thaJeztah

LGTM

This extends docker#2929 to Darwin as well as Linux. Running the example in golang/go#37942 I see lots of: ``` dave@m1 sigurg % uname -ms Darwin arm64 dave@m1 sigurg % go run main.go received urgent I/O condition: 2021-05-21 16:03:03.482211 +0100 BST m=+0.014553751 received urgent I/O condition: 2021-05-21 16:03:03.507171 +0100 BST m=+0.039514459 ``` Signed-off-by: David Scott <dave@recoil.org>

This extends docker#2929 to Darwin as well as Linux. Running the example in golang/go#37942 I see lots of: ``` dave@m1 sigurg % uname -ms Darwin arm64 dave@m1 sigurg % go run main.go received urgent I/O condition: 2021-05-21 16:03:03.482211 +0100 BST m=+0.014553751 received urgent I/O condition: 2021-05-21 16:03:03.507171 +0100 BST m=+0.039514459 ``` Signed-off-by: David Scott <dave@recoil.org> (cherry picked from commit cedaf44) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cpuguy83 requested review from silvin-lubecki and simonferquel January 13, 2021 18:06

This was referenced Jan 13, 2021
containerd-shim compiled with Go 1.15 forwards SIGURG to container containerd/containerd#4935
Closed
protokube container started receiving Docker kill events with signal 23 after Flatcar upgrade to 2605.9.0 kubernetes/kops#10388
Closed

cpuguy83 force-pushed the ignore_sigurg branch from f578cda to 23856d5Compare January 13, 2021 18:25

silvin-lubecki reviewed Jan 14, 2021
View reviewed changes

cpuguy83 force-pushed the ignore_sigurg branch from 23856d5 to 52c7d66Compare January 14, 2021 23:17

silvin-lubecki approved these changes Jan 15, 2021
View reviewed changes

cpuguy83 force-pushed the ignore_sigurg branch from 52c7d66 to fff164cCompare January 15, 2021 19:03

thaJeztah approved these changes Jan 16, 2021
View reviewed changes

thaJeztah merged commit 35f023a into docker:masterJan 16, 2021

thaJeztah added this to the 20.10.3 milestone Jan 21, 2021

margamanterola mentioned this pull request Jan 26, 2021
Signal 23 (SIGURG) Docker event flood flatcar/Flatcar#315
Closed

thaJeztah modified the milestones: 20.10.3, 20.10.4 Feb 2, 2021

thaJeztah added the process/cherry-pick label Feb 4, 2021

thaJeztah removed this from the 20.10.4 milestone Feb 4, 2021

thaJeztah added this to the 21.xx milestone Feb 4, 2021

thaJeztah mentioned this pull request Feb 4, 2021
[20.10 backport] Ignore SIGURG on Linux. #2960
Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Feb 4, 2021

cpuguy83 deleted the ignore_sigurg branch April 27, 2021 19:52

This was referenced May 19, 2021
pkg/signal.CatchAll: ignore SIGURG on Linux moby/moby#42397
Merged
[20.10 backport] pkg/signal.CatchAll: ignore SIGURG on Linux moby/moby#42401
Merged

djs55 mentioned this pull request May 21, 2021
Ignore SIGURG on Unix (including Darwin) #3103
Merged

thaJeztah mentioned this pull request May 25, 2021
Fix docker start blocking on signal handling #2990
Merged

thaJeztah mentioned this pull request May 25, 2021
[20.10 backport] Ignore SIGURG on Unix (including Darwin) #3107
Merged

drewdzzz mentioned this pull request Feb 26, 2026
Revise resetting current fiber slice on SIGURG tarantool/tarantool#12349
Open

Uh oh!

Conversation

cpuguy83 commented Jan 13, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 13, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jan 14, 2021

Uh oh!

silvin-lubecki commented Jan 14, 2021

Uh oh!

cpuguy83 commented Jan 14, 2021

Uh oh!

thaJeztah commented Jan 14, 2021

Uh oh!

cpuguy83 commented Jan 14, 2021

Uh oh!

cpuguy83 commented Jan 14, 2021

Uh oh!

cpuguy83 commented Jan 14, 2021

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki commented Jan 15, 2021

Uh oh!

cpuguy83 commented Jan 15, 2021

Uh oh!

thaJeztah commented Jan 16, 2021

Uh oh!

thaJeztah commented Jan 16, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cpuguy83 commented Jan 13, 2021•
edited
Loading

codecov-io commented Jan 13, 2021•
edited
Loading