Update runc kill wrapper to take a string for signal #78

katiewasnothere · 2021-09-14T22:43:36Z

This PR is part of the work for containerd/containerd#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 opencontainers/runc#3213.

This is a breaking change for this function signature.

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

codecov-commenter · 2021-09-14T22:44:20Z

Codecov Report

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

❗ 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%)`	⬇️
io_unix.go	`0.00% <0.00%> (ø)`
runc.go	`25.00% <100.00%> (+3.92%)`	⬆️

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 · 2021-09-14T22:44:42Z

@thaJeztah @kzys btw

dims

LGTM

thaJeztah

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

thaJeztah · 2021-09-15T07:11:57Z

runc.go

@@ -340,14 +340,14 @@ func (o *KillOpts) args() (out []string) {
 }

 // Kill sends the specified signal to the container
-func (r *Runc) Kill(context context.Context, id string, sig int, opts *KillOpts) error {
+func (r *Runc) Kill(context context.Context, id string, sig string, opts *KillOpts) error {


I see the go-runc module is already at v1.0.0, and I see there's various projects using this module (outside of containerd); changing this signature would require incrementing the major version to v2.0.0.

Perhaps instead, we should take the same approach here as discussed in the containerd ticket;

add a Signal (string) field to KillOpts

if both sig and KillOpts.Signal are set, then the latter overrides the former

callers can decide to either use sig, or KillOpts.Signal

Probably warrants an update to the GoDoc of this function to describe how it should be used (and that KillOpts.Signal should be considered in favor of sig by callers that do not want to (or can't) perform the conversion of string to signal (numeric), such as in the LCOW case.

I agree with @thaJeztah. As like containerd, we should keep go-runc backward-compatible as much as we can.

thaJeztah · 2021-09-15T07:32:55Z

(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 #61). Looks like that change caused a bit of a ripple effect (containerd/containerd#4317, but also now having to maintain a windows variant just to make it compile; #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 · 2021-09-15T08:24:57Z

Opened #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 · 2021-09-15T17:25:12Z

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 · 2021-09-15T18:59:58Z

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 · 2021-09-15T20:19:35Z

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 · 2021-09-16T19:08:01Z

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

katiewasnothere · 2021-09-17T03:41:06Z

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 · 2021-09-17T08:00:18Z

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 · 2021-09-17T22:18:28Z

@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 · 2021-09-29T20:05:04Z

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

thaJeztah · 2021-09-30T13:28:25Z

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 · 2021-09-30T19:38:19Z

@thaJeztah added!

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

thaJeztah

LGTM!

kzys

Since runc wouldn't use moby/sys (opencontainers/runc#3213), the only benefit of this change is making the interface closer to what runc actually takes (both strings and numbers). Real-time signals must be passed as integers though.

Is it really worth to do vs. the UX complexity the change brings (honor KillOpts over sig)?

kzys · 2021-09-30T23:23:36Z

runc.go

@@ -327,6 +327,7 @@ func (r *Runc) Delete(context context.Context, id string, opts *DeleteOpts) erro
 type KillOpts struct {
 All bool
 ExtraArgs []string
+ RawSignal string


Not really sure Raw is the right prefix here. Signals in Unix are integers and this parameter can take both integers and strings.

Signal or SignalName?

Yeah, the naming for this is a little difficult 😅 With Raw I was hoping to convey that the signal has potentially not been parsed yet.

thaJeztah · 2021-10-08T14:13:45Z

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 · 2021-10-15T19:28:20Z

What is the best path forward then here? @thaJeztah

katiewasnothere mentioned this pull request Sep 15, 2021

Add support for passing signals as strings containerd/containerd#6002

Closed

dims approved these changes Sep 15, 2021

View reviewed changes

thaJeztah suggested changes Sep 15, 2021

View reviewed changes

katiewasnothere force-pushed the runc_kill_string branch from 9d4c2d0 to 74d18cf Compare September 17, 2021 03:43

dmcgowan added this to Needs Discussion in Code Review Sep 27, 2021

katiewasnothere force-pushed the runc_kill_string branch from 74d18cf to 5b453a9 Compare September 30, 2021 19:37

Update runc kill wrapper to take a string for signal

de8d0d3

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

katiewasnothere force-pushed the runc_kill_string branch from 5b453a9 to de8d0d3 Compare September 30, 2021 19:40

thaJeztah approved these changes Sep 30, 2021

View reviewed changes

kzys reviewed Sep 30, 2021

View reviewed changes

thaJeztah mentioned this pull request May 6, 2022

pass syscall.Signal for stop-signals to reduce type conversions moby/moby#43563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update runc kill wrapper to take a string for signal #78

Update runc kill wrapper to take a string for signal #78

katiewasnothere commented Sep 14, 2021

codecov-commenter commented Sep 14, 2021 •

edited

katiewasnothere commented Sep 14, 2021

dims left a comment

thaJeztah left a comment

thaJeztah Sep 15, 2021

kzys Sep 15, 2021

thaJeztah commented Sep 15, 2021

thaJeztah commented Sep 15, 2021

kzys commented Sep 15, 2021

katiewasnothere commented Sep 15, 2021

kzys commented Sep 15, 2021

katiewasnothere commented Sep 16, 2021

katiewasnothere commented Sep 17, 2021

thaJeztah commented Sep 17, 2021

katiewasnothere commented Sep 17, 2021

katiewasnothere commented Sep 29, 2021

thaJeztah commented Sep 30, 2021 •

edited

katiewasnothere commented Sep 30, 2021

thaJeztah left a comment

kzys left a comment

kzys Sep 30, 2021

katiewasnothere Oct 1, 2021

thaJeztah commented Oct 8, 2021

katiewasnothere commented Oct 15, 2021

Update runc kill wrapper to take a string for signal #78

Are you sure you want to change the base?

Update runc kill wrapper to take a string for signal #78

Conversation

katiewasnothere commented Sep 14, 2021

codecov-commenter commented Sep 14, 2021 • edited

Codecov Report

katiewasnothere commented Sep 14, 2021

dims left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Sep 15, 2021

Choose a reason for hiding this comment

kzys Sep 15, 2021

Choose a reason for hiding this comment

thaJeztah commented Sep 15, 2021

thaJeztah commented Sep 15, 2021

kzys commented Sep 15, 2021

katiewasnothere commented Sep 15, 2021

kzys commented Sep 15, 2021

katiewasnothere commented Sep 16, 2021

katiewasnothere commented Sep 17, 2021

thaJeztah commented Sep 17, 2021

katiewasnothere commented Sep 17, 2021

katiewasnothere commented Sep 29, 2021

thaJeztah commented Sep 30, 2021 • edited

katiewasnothere commented Sep 30, 2021

thaJeztah left a comment

Choose a reason for hiding this comment

kzys left a comment

Choose a reason for hiding this comment

kzys Sep 30, 2021

Choose a reason for hiding this comment

katiewasnothere Oct 1, 2021

Choose a reason for hiding this comment

thaJeztah commented Oct 8, 2021

katiewasnothere commented Oct 15, 2021

codecov-commenter commented Sep 14, 2021 •

edited

thaJeztah commented Sep 30, 2021 •

edited