Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

signal: Backtrace on SIGUSR1 #79

Merged
merged 6 commits into from
May 11, 2018
Merged

signal: Backtrace on SIGUSR1 #79

merged 6 commits into from
May 11, 2018

Conversation

jodh-intel
Copy link
Contributor

Rework the signal handling code so that if debug is enabled and a
SIGUSR1 signal is received, backtrace to the system log but continue
to run.

Added some basic tests for the signal handling code.

Fixes #76.

Signed-off-by: James O. D. Hunt [email protected]

@jodh-intel
Copy link
Contributor Author

@amshinde, @sboeuf, @bergwolf - I'm raising this now but the tests fail due to a data race. fwics, it's related to io.Copy() not being interruptable. However, even after fiddling around with TestShimOps() for a bit, I'm struggling to resolve the issue. Have any of you encountered this before?

=== RUN   TestSignalBacktrace
==================
WARNING: DATA RACE
Write at 0x00000128f800 by goroutine 40:
  github.com/kata-containers/shim.TestSignalBacktrace()
      /home/james/go/src/github.com/kata-containers/shim/signals_test.go:108 +0x10f
  testing.tRunner()
      /usr/local/go/go1.9/go/src/testing/testing.go:746 +0x16c

Previous read at 0x00000128f800 by goroutine 91:
  github.com/kata-containers/shim.logger()
      github.com/kata-containers/shim/_test/_obj_test/main.go:39 +0x52
  github.com/kata-containers/shim.(*shim).proxyStdio.func2()
      github.com/kata-containers/shim/_test/_obj_test/shim.go:91 +0xf4

Goroutine 40 (running) created at:
  testing.(*T).Run()
      /usr/local/go/go1.9/go/src/testing/testing.go:789 +0x568
  testing.runTests.func1()
      /usr/local/go/go1.9/go/src/testing/testing.go:1004 +0xa7
  testing.tRunner()
      /usr/local/go/go1.9/go/src/testing/testing.go:746 +0x16c
  testing.runTests()
      /usr/local/go/go1.9/go/src/testing/testing.go:1002 +0x521
  testing.(*M).Run()
      /usr/local/go/go1.9/go/src/testing/testing.go:921 +0x206
  main.main()
      github.com/kata-containers/shim/_test/_testmain.go:128 +0x2f0

Goroutine 91 (finished) created at:
  github.com/kata-containers/shim.(*shim).proxyStdio()
      github.com/kata-containers/shim/_test/_obj_test/shim.go:86 +0x1eb
  github.com/kata-containers/shim.TestShimOps()
      /home/james/go/src/github.com/kata-containers/shim/shim_test.go:54 +0x553
  testing.tRunner()
      /usr/local/go/go1.9/go/src/testing/testing.go:746 +0x16c
==================
==================
WARNING: DATA RACE
Write at 0x00c4200743c0 by goroutine 40:
  github.com/kata-containers/shim.TestSignalBacktrace()
      /home/james/go/src/github.com/kata-containers/shim/signals_test.go:119 +0x236
  testing.tRunner()
      /usr/local/go/go1.9/go/src/testing/testing.go:746 +0x16c

Previous read at 0x00c4200743c0 by goroutine 91:
  github.com/kata-containers/shim/vendor/github.com/sirupsen/logrus.Entry.log()
      /home/james/go/src/github.com/kata-containers/shim/vendor/github.com/sirupsen/logrus/entry.go:117 +0x5e8
  github.com/kata-containers/shim/vendor/github.com/sirupsen/logrus.(*Entry).Info()
      /home/james/go/src/github.com/kata-containers/shim/vendor/github.com/sirupsen/logrus/entry.go:144 +0x116
  github.com/kata-containers/shim.(*shim).proxyStdio.func2()
      github.com/kata-containers/shim/_test/_obj_test/shim.go:91 +0x179

Goroutine 40 (running) created at:
  testing.(*T).Run()
      /usr/local/go/go1.9/go/src/testing/testing.go:789 +0x568
  testing.runTests.func1()
      /usr/local/go/go1.9/go/src/testing/testing.go:1004 +0xa7
  testing.tRunner()
      /usr/local/go/go1.9/go/src/testing/testing.go:746 +0x16c
  testing.runTests()
      /usr/local/go/go1.9/go/src/testing/testing.go:1002 +0x521
  testing.(*M).Run()
      /usr/local/go/go1.9/go/src/testing/testing.go:921 +0x206
  main.main()
      github.com/kata-containers/shim/_test/_testmain.go:128 +0x2f0

Goroutine 91 (finished) created at:
  github.com/kata-containers/shim.(*shim).proxyStdio()
      github.com/kata-containers/shim/_test/_obj_test/shim.go:86 +0x1eb
  github.com/kata-containers/shim.TestShimOps()
      /home/james/go/src/github.com/kata-containers/shim/shim_test.go:54 +0x553
  testing.tRunner()
      /usr/local/go/go1.9/go/src/testing/testing.go:746 +0x16c
==================
--- FAIL: TestSignalBacktrace (0.13s)
        testing.go:699: race detected during execution of test

@jodh-intel
tests: Fix TestShimOps
2503995 
Changed `TestShimOps()` to wait for the go routines started by
`proxyStdio()` to end. Without this fix, `go test` fails with a data
race panic.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel
signal: Rename fatal.go to signals.go
348eb10 
The fatal file is going to also deal with non-fatal signals so rename
it.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel
main: Enable debug if log level is debug
2d5d91e 
The debug option is now auto-enabled if the user specifies the debug
log level.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel
logging: Remove unnecessary logging argument
041690d 
Don't pass an empty string to the logger as it is not required.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel
signal: Send fatal signal to workload before dying
ef3858e 
A signal that the shim considers as fatal should be forwarded to the
container before being handled by the shim (to ensure the former also
receives it).

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel
Copy link
Contributor Author

Atlast had a chance to look at this again. The data race was caused by a missing waitgroup.Wait() call in the tests (see commit 2503995).

Removed dnm label.

@jodh-intel
signal: Backtrace on SIGUSR1
bae08c9 
Rework the signal handling code so that if debug is enabled and a
`SIGUSR1` signal is received, backtrace to the system log but continue
to run.

Added some basic tests for the signal handling code.

Fixes kata-containers#76.

Signed-off-by: James O. D. Hunt <[email protected]>
@codecov
Copy link

codecov bot commented May 11, 2018 •
edited
Loading

Codecov Report

Merging #79 into master will increase coverage by 6.28%.
The diff coverage is 60.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   45.45%   51.73%   +6.28%     
==========================================
  Files           6        6              
  Lines         242      259      +17     
==========================================
+ Hits          110      134      +24     
+ Misses        120      114       -6     
+ Partials       12       11       -1
Impacted Files Coverage Δ
shim.go 53.33% <0%> (+0.45%) ⬆️
main.go 23.45% <0%> (-0.3%) ⬇️
signals.go 64.7% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45f17ad...bae08c9. Read the comment docs.

@jodh-intel
Copy link
Contributor Author

Yay for the log parser which just found a subtle test bug ;)

@jodh-intel
Copy link
Contributor Author

Ping @kata-containers/shim.

@jodh-intel
Copy link
Contributor Author

Hi @amshinde - ptal. @egernst's approval should be sufficient, but he's a new member to the approval group and pullapprove seems to have cached the old list of members, so his ack isn't registering for pullapprove.

@amshinde amshinde merged commit 2457ccc into kata-containers:master May 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@egernst egernst egernst approved these changes

@amshinde amshinde amshinde approved these changes

@bergwolf bergwolf bergwolf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jodh-intel @bergwolf @amshinde @egernst