Update runc kill wrapper to take a string for signal

[katiewasnothere]

This PR is part of the work for https://github.com/containerd/containerd/issues/5931. Previous code sent the signal as an integer and converted it to a string using strconv. Instead, change the signature to take a string for the signal instead. This will accommodate sending a signal as either it's integer representation (ex: 9) and the string representation (ex:SIGKILL). This will be combined with the work in https://github.com/opencontainers/runc/pull/3213.

This is a breaking change for this function signature.

Signed-off-by: Kathryn Baldauf [email protected]

[codecov-commenter]

Codecov Report

Merging #78 (415932d) into main (d1e3ca2) will increase coverage by 2.16%. The diff coverage is 40.00%.

:exclamation: Current head 415932d differs from pull request most recent head de8d0d3. Consider uploading reports for the commit de8d0d3 to get more accurate results

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   21.52%   23.69%   +2.16%     
==========================================
  Files           7        7              
  Lines         683      688       +5     
==========================================
+ Hits          147      163      +16     
+ Misses        498      484      -14     
- Partials       38       41       +3     

Impacted Files	Coverage Δ
io.go	6.45% <0.00%> (-0.15%)	:arrow_down:
io_unix.go	0.00% <0.00%> (ø)
runc.go	25.00% <100.00%> (+3.92%)	:arrow_up:

---

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 d1e3ca2...de8d0d3. Read the comment docs.

[katiewasnothere]

@thaJeztah @kzys btw

[thaJeztah]

(sorry, orthogonal comment)

Hm... not something we need to change for this PR, but looking at the code in this project, I wonder why we changed some uses of syscall.Signal to unix.Signal (in https://github.com/containerd/go-runc/pull/61). Looks like that change caused a bit of a ripple effect (https://github.com/containerd/containerd/pull/4317, but also now having to maintain a windows variant just to make it compile; https://github.com/containerd/go-runc/pull/66, which currently even leads to them being out of sync (the Windows variant is missing the ExtraArgs{} field). Given that unix.Signal is an alias for syscall.Signal, I think we should consider changing that back (at least for the unix.Signal cases.

[thaJeztah]

Opened https://github.com/containerd/go-runc/pull/79 for the above, but perhaps we need more cleaning up to exclude code on Windows, or move the Monitor code to a subdirectory / package, so that it can be consumed without pulling in everything else.

[kzys]

Also, I'm not familiar about the role go-runc plays in LCOW and WCOW. Passing signal strings allow us to defer the cross-platform conversion and doing that in higher-level components sounds reasonable. go-runc (or runc) is a relatively low-level component. What we can unblock by having this change?

[katiewasnothere]

Also, I'm not familiar about the role go-runc plays in LCOW and WCOW. Passing signal strings allow us to defer the cross-platform conversion and doing that in higher-level components sounds reasonable. go-runc (or runc) is a relatively low-level component. What we can unblock by having this change?

LCOW talks to runc via a guest component that runs inside the Linux VM, so we have control on our side to accommodate sending the signal as a string. This PR just allows us to update the code in containerd that calls into runc to pass through signals as strings for consistency.

[kzys]

I don't think we have to make them consistent if so. Cross-platform components (such as Kubernetes and containerd) can handle string-based signals to let low-level components handle (including covert) them. Low-level, single-platform components (such as runc) can use number-based signals.

[katiewasnothere]

@kzys Per our discussion yesterday, what are your thoughts on this?

[katiewasnothere]

Left a suggestion; perhaps would be good to add a unit test as well to verify the behavior (order of preference / conversion)

@thaJeztah Are you recommending adding tests in this repo for that? Checking the signal conversion would require that the tests actually run runc which appears to not be done today.

[thaJeztah]

Checking the signal conversion would require that the tests actually run runc which appears to not be done today.

Sorry, I should probably have been more clear; full integration tests would be good, but I think that should be mostly covered in the containerd repository, and (as you mentioned) not in place here (yet).

I was thinking of a test to verify that the right order of preference is taken, which can end with checking that the expected value is passed as the command-line arguments for "runc".

This should probably be written as a test-table, but something like below (stubbing out runc for /bin/true, which looks to be used for some other tests), as we're only interested in this code passing the command-line arguments that we hand it; I'm not sure if the code needs handling of the 0 case (no signal given at all)? If we consider that a responsibility for the caller, that of course can be skipped;

	ctx := context.Background()
	okRunc := &Runc{
		Command: "/bin/true",
	}

	// Should produce an error?? (no signal given?)
	_ := okRunc.Kill(ctx, "fake-id", 0, &KillOpts{})

	// Should exec `/bin/true kill fake-id 123`
	_ := okRunc.Kill(ctx, "fake-id", 123, &KillOpts{})

	// Should also exec `/bin/true kill fake-id 123`
	_ := okRunc.Kill(ctx, "fake-id", 123, &KillOpts{RawSignal: ""})

	// Should also exec `/bin/true kill fake-id 123` ???
	_ := okRunc.Kill(ctx, "fake-id", 123, &KillOpts{RawSignal: "0"})


	// Should exec `/bin/true kill fake-id 456`
	_ := okRunc.Kill(ctx, "fake-id", 0, &KillOpts{RawSignal: "456"})

	// Should also exec `/bin/true kill fake-id 456`
	_ := okRunc.Kill(ctx, "fake-id", 123, &KillOpts{RawSignal: "456"})

	// Should exec `/bin/true kill fake-id SIGFOOBAR`
	_ := okRunc.Kill(ctx, "fake-id", 123, &KillOpts{RawSignal: "SIGFOOBAR"})

	// Should exec `/bin/true kill fake-id SIGFOOBAR`
	_ := okRunc.Kill(ctx, "fake-id", 123, &KillOpts{RawSignal: "SIGFOOBAR"})

[katiewasnothere]

@thaJeztah afaict there's no way for us to capture what's actually sent to the runc command without adding more functionality to the kill func to pass in our own IO. Or making a much more complicated runc command that will copy input to an expected location.

[katiewasnothere]

@thaJeztah Any thoughts on what tests would be best here given my last comment?

[thaJeztah]

Hmm... good one; I thought it would be easier to capture the arguments.

A "quick and dirty" way would be a shell script that debugs what it received, and returns the output. Looks like we don't print the output unless the command fails, so it needs to exit with a non-zero exit code (which is fine for this, as we're only interested in verifying it received the right arguments?), so something like:

#!/bin/sh
echo "$@"
exit 1

I see there's already some code to generate a script for testing, so we can probably reuse that. Here's a quick attempt (it's a quick write-up; the test itself should probably use a test-table for the different test-cases);

diff --git a/runc_test.go b/runc_test.go
index 4b1275a..1138486 100644
--- a/runc_test.go
+++ b/runc_test.go
@@ -257,6 +257,22 @@ func TestRuncStarted(t *testing.T) {
 	}
 }
 
+func TestRuncKill(t *testing.T) {
+	ctx, timeout := context.WithTimeout(context.Background(), 10*time.Second)
+	defer timeout()
+
+	dummyCommand, err := debugCommand()
+	if err != nil {
+		t.Fatalf("Failed to create dummy sleep runc: %s", err)
+	}
+	defer os.Remove(dummyCommand)
+
+	runc := &Runc{Command: dummyCommand}
+
+	err = runc.Kill(ctx, "fake-id", 0, &KillOpts{})
+	t.Log(err)
+}
+
 func extractStatus(err error) int {
 	if err == nil {
 		return 0
@@ -287,9 +303,27 @@ func interrupt(ctx context.Context, t *testing.T, started <-chan int) {
 	}
 }
 
+
+// debugCommand creates s simple script that echos the arguments passed to
+// runc, and returns them as part of the error message.
+func debugCommand() (string, error) {
+	return createScript(`#!/bin/sh
+echo "$@"
+
+# force non-zero exit code, so that the error message contains the output
+exit 1
+`)
+}
+
 // dummySleepRunc creates s simple script that just runs `sleep 10` to replace
 // runc for testing process that are longer running.
 func dummySleepRunc() (_ string, err error) {
+	return createScript("#!/bin/sh\nexec /bin/sleep 10")
+}
+
+// createScript creates s simple script with the given content, and returns
+// its filename, or an error when failing to create the script.
+func createScript(content string) (_ string, err error) {
 	fh, err := ioutil.TempFile("", "*.sh")
 	if err != nil {
 		return "", err
@@ -299,7 +333,7 @@ func dummySleepRunc() (_ string, err error) {
 			os.Remove(fh.Name())
 		}
 	}()
-	_, err = fh.Write([]byte("#!/bin/sh\nexec /bin/sleep 10"))
+	_, err = fh.Write([]byte(content))
 	if err != nil {
 		return "", err
 	}

[katiewasnothere]

@thaJeztah added!

[thaJeztah]

We'd still need to discuss the conversion for realtime signals. It's ugly, but conversion has to be done somewhere, and (from the discussions) it looks like neither containerd, nor runc would be able to judge what the "right" signal number is. which brings me back to my original opinion that at least runc has a better view of the world (as at least the platform and kernel would match the actual platform for the container), but yes, it would require runc to accept realtime signals as a string as well (and perhaps the OCI Runtime spec defining "canonical" numbers for these, by lack of a better way of converting to the correct signal).

[katiewasnothere]

What is the best path forward then here? @thaJeztah