drivers/docker: add support for STOPSIGNAL #10441

isabeldepapel · 2021-04-23T21:43:40Z

Still need to figure out the tests but this is what I have so far

This fixes a bug where Nomad overrides a Dockerfile's STOPSIGNAL with
the default kill_signal (SIGTERM).

This adds a check for kill_signal. If it's not set, it calls
StopContainer instead of Signal, which uses STOPSIGNAL if it's
specified. If both kill_signal and STOPSIGNAL are set, Nomad tries to
stop the container with kill_signal first, before then calling
StopContainer.

Fixes #9989

hashicorp-cla · 2021-04-23T21:43:44Z

All committers have signed the CLA.

tgross

This is looking good, @isabeldepapel.

It looks like there are some tests that exercise the stop behavior vs a real Docker already (example). So you can probably verify this works by querying for Docker API events in those tests, and then making assertions about those events.

drivers/docker/handle.go

tgross · 2021-04-28T12:37:13Z

drivers/docker/handle.go

+	// Calling StopContainer lets docker handle the stop signal (specified
+	// in the Dockerfile or defaulting to SIGTERM). If kill_signal is specified,
+	// Signal is used to kill the container with the desired signal before
+	// calling StopContainer


👍 Good explanatory comment for summarizing the domain-specific knowledge you'd need to understand the code here.

notnoop

This looks great. Thanks @isabeldepapel

Made a couple of comments - mostly how the existing code doesn't follow some golang convention. Noting them for educational value only.

notnoop · 2021-04-28T18:28:27Z

drivers/docker/handle.go

+// parseSignal interprets the signal name into an os.Signal. If no name is
+// provided, the docker driver defaults to SIGTERM. If the OS is Windows and
+// SIGINT is provided, the signal is converted to SIGTERM.
+func (h *taskHandle) parseSignal(os, signal string) (os.Signal, error) {


This method is a simple "pure" function that doesn't depend on taskHandle/driver. You can make it a simple function and simply the tests by avoiding the dockerTest related boilerplate:

Suggested change

func (h *taskHandle) parseSignal(os, signal string) (os.Signal, error) {

func parseSignal(os, signal string) (os.Signal, error) {

notnoop · 2021-04-28T18:31:35Z

drivers/docker/driver_test.go

 	t.Run("default", func(t *testing.T) {
-		s, err := d.parseSignal(runtime.GOOS, "")
+		s, err := handle.parseSignal(runtime.GOOS, "")
 		require.NoError(t, err)
 		require.Equal(t, syscall.SIGTERM, s)
 	})


FWIW - These can be better expressed as table driver tests, a common pattern Nomad uses elsewhere: https://jayson.dev/blog/2019/06/table-driven-golang-subtests/#time-for-the-subtests .

Such a refactor is outside the scope of the PR; though, wanted to note it for educational value. If you do want to take it on, no objection ;-).

notnoop · 2021-04-28T18:47:05Z

drivers/docker/driver_test.go

@@ -2911,28 +2911,33 @@ func TestDockerDriver_memoryLimits(t *testing.T) {
 func TestDockerDriver_parseSignal(t *testing.T) {


Noting that Golang has a convention about test files: If the tested function is in handle.go, the tests belong in handle_test.go. It's odd that handle_test.go. Seeing how the two files are intertwined, it's fine to leave the test here.

notnoop

This is great! I have few suggestions for the tests; but it's solid. I'll be happy to pair, specially with regards to the concurrency issue.

drivers/docker/driver_test.go

notnoop · 2021-05-04T15:11:10Z

drivers/docker/driver_test.go

-		require.Equal(t, syscall.SIGHUP, s)
-	})
+// This test asserts that Nomad isn't overriding the STOPSIGNAL in a Dockerfile
+func TestDockerDriver_StopSignal(t *testing.T) {


This is a great test. Captures all conditions :).

drivers/docker/driver_test.go

isabeldepapel · 2021-05-05T16:27:26Z

Ready for another pass @notnoop

notnoop

Looks great. A minor suggestion to the shell script - but merge away 🎉

notnoop · 2021-05-05T16:36:04Z

drivers/docker/test-resources/docker/gen_stopsignal.sh

+#!/bin/sh
+
+# Create the tarball used in TestDockerDriver_StopSignal
+echo "FROM busybox:1.29.3\nSTOPSIGNAL 19" > tmpfile && docker build -t busybox:1.29.3-stopsignal - < tmpfile && rm tmpfile


I suspect you can avoid needing the tmp file. Also it might a bit nicer to use here-doc style in this case to avoid new lines:

Suggested change

echo "FROM busybox:1.29.3\nSTOPSIGNAL 19" > tmpfile && docker build -t busybox:1.29.3-stopsignal - < tmpfile && rm tmpfile

cat <<'EOF' | docker build -t busybox:1.29.3-stopsignal -

FROM busybox:1.29.3

STOPSIGNAL 19

EOF

This fixes a bug where Nomad overrides a Dockerfile's STOPSIGNAL with the default kill_signal (SIGTERM). This adds a check for kill_signal. If it's not set, it calls StopContainer instead of Signal, which uses STOPSIGNAL if it's specified. If both kill_signal and STOPSIGNAL are set, Nomad tries to stop the container with kill_signal first, before then calling StopContainer. Fixes #9989

drivers/docker: add support for STOPSIGNAL

github-actions · 2022-11-21T02:31:04Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

vercel bot temporarily deployed to Preview – nomad April 23, 2021 22:36 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui April 23, 2021 22:36 View deployment

tgross reviewed Apr 28, 2021

View reviewed changes

notnoop reviewed Apr 28, 2021

View reviewed changes

isabeldepapel force-pushed the b-docker-stopsignal branch from b04f8b2 to b12ba54 Compare May 3, 2021 18:44

vercel bot temporarily deployed to Preview – nomad May 3, 2021 18:44 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui May 3, 2021 18:44 View deployment

isabeldepapel force-pushed the b-docker-stopsignal branch from b12ba54 to 27f0c7e Compare May 3, 2021 20:09

vercel bot deployed to Preview – nomad-storybook-and-ui May 3, 2021 20:09 View deployment

vercel bot temporarily deployed to Preview – nomad May 3, 2021 20:09 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui May 3, 2021 23:19 View deployment

vercel bot temporarily deployed to Preview – nomad May 3, 2021 23:19 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui May 3, 2021 23:49 View deployment

vercel bot temporarily deployed to Preview – nomad May 3, 2021 23:49 Inactive

isabeldepapel force-pushed the b-docker-stopsignal branch from 232d932 to b3f6293 Compare May 4, 2021 00:07

vercel bot temporarily deployed to Preview – nomad May 4, 2021 00:07 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui May 4, 2021 00:07 View deployment

isabeldepapel marked this pull request as ready for review May 4, 2021 00:11

notnoop reviewed May 4, 2021

View reviewed changes

isabeldepapel force-pushed the b-docker-stopsignal branch from b3f6293 to 1f73582 Compare May 4, 2021 23:12

vercel bot deployed to Preview – nomad-storybook-and-ui May 4, 2021 23:12 View deployment

vercel bot temporarily deployed to Preview – nomad May 4, 2021 23:12 Inactive

notnoop approved these changes May 5, 2021

View reviewed changes

isabeldepapel force-pushed the b-docker-stopsignal branch from 1f73582 to 379c095 Compare May 5, 2021 17:28

vercel bot temporarily deployed to Preview – nomad May 5, 2021 17:28 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui May 5, 2021 17:28 View deployment

isabeldepapel merged commit 7d5a9ec into main May 5, 2021

isabeldepapel deleted the b-docker-stopsignal branch May 5, 2021 18:24

tgross added the stage/needs-backporting label May 7, 2021

tgross added this to the 1.1.0 milestone May 17, 2021

tgross pushed a commit that referenced this pull request May 17, 2021

Merge pull request #10441 from hashicorp/b-docker-stopsignal

0599064

drivers/docker: add support for STOPSIGNAL

tgross removed the stage/needs-backporting label May 24, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/docker: add support for STOPSIGNAL #10441

drivers/docker: add support for STOPSIGNAL #10441

isabeldepapel commented Apr 23, 2021

hashicorp-cla commented Apr 23, 2021 •

edited

Loading

tgross left a comment •

edited

Loading

tgross Apr 28, 2021

notnoop left a comment

notnoop Apr 28, 2021

notnoop Apr 28, 2021

notnoop Apr 28, 2021

notnoop left a comment

notnoop May 4, 2021

isabeldepapel commented May 5, 2021

notnoop left a comment

notnoop May 5, 2021

github-actions bot commented Nov 21, 2022

	func (h *taskHandle) parseSignal(os, signal string) (os.Signal, error) {
	func parseSignal(os, signal string) (os.Signal, error) {

		@@ -2911,28 +2911,33 @@ func TestDockerDriver_memoryLimits(t *testing.T) {
		func TestDockerDriver_parseSignal(t *testing.T) {

drivers/docker: add support for STOPSIGNAL #10441

drivers/docker: add support for STOPSIGNAL #10441

Conversation

isabeldepapel commented Apr 23, 2021

hashicorp-cla commented Apr 23, 2021 • edited Loading

tgross left a comment • edited Loading

Choose a reason for hiding this comment

tgross Apr 28, 2021

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

notnoop Apr 28, 2021

Choose a reason for hiding this comment

notnoop Apr 28, 2021

Choose a reason for hiding this comment

notnoop Apr 28, 2021

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

notnoop May 4, 2021

Choose a reason for hiding this comment

isabeldepapel commented May 5, 2021

notnoop left a comment

Choose a reason for hiding this comment

notnoop May 5, 2021

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2022

hashicorp-cla commented Apr 23, 2021 •

edited

Loading

tgross left a comment •

edited

Loading