Make sure Machine#Wait() gurantee that the underlying Firecracker process is stopped #182

kzys · 2020-01-31T18:35:00Z

The method should guarantee that the underlying process is dead,
but it didn't.

Signed-off-by: Kazuyoshi Kato [email protected]

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

smira

👍

mxpv · 2020-02-03T21:10:43Z

machine_test.go

+func isProcessAlive(pid int) (bool, error) {
+	// Using kill(2) with signal=0 to check the existence of the process,
+	// because os.FindProcess always returns a process, regardless of whether the process is
+	// alive or not.


Can you elaborate on this?
As far as I see go's FindProcess does the same:
https://golang.org/src/os/exec_unix.go#L88
https://golang.org/src/os/exec_unix.go#L73
and its expected to receive err os: process already finished

Clarification: you can use it like err := os.FindProcess(x).Signal(0)

The error from Process#Signal() is not typed. The only way to check the type of the error is string comparison. So I'm unsure that is reliable over time.

https://github.com/golang/go/blob/753d56d3642eb83848aa39e65982a9fc77e722d7/src/os/exec_unix.go#L53

Don't think you need to cast the error. Will be nice to get rid of the unneeded if block
https://github.com/golang/go/blob/753d56d3642eb83848aa39e65982a9fc77e722d7/src/os/exec_unix.go#L72-L74

kill(2) returns EINVAL, EPERM or ESRCH as an error. Go's Signal() distinguish them, but doesn't provide a programmatic way of checking the type.

Would it be better to do something like below rather than using syscall directly? Personally I don't want to depend on the string representation because it might change.

if err.Error() == "os: process already finished" { ... }

Thoughts?

According to Signal it will only return os: process already finished error when syscall.ESRCH occurs. So, to me I think it doesn't matter either way. I think the string comparison is more ugly than checking against ESRCH, but I am fine with either solution.

edit: Apparently it sets done based on Wait, which may return the already finished error, which complicates the checks. We have to know whether or not Wait has been called and if it has, we cannot rely on ESRCH. So a more simple approach is string comparison of the error. :/

Apparently it sets done based on Wait, which may return the already finished error, which complicates the checks.

And all of these are happening inside os.Process. syscall.Kill is much simpler I think.

I'd use os.Process, but I agree that strings comparison is ugly. Unfortunately sometimes its hard to get away from it (for instance there is a similar checker in containerd: https://github.com/containerd/containerd/blob/b0821c801dc2225bb7478f91e967888a353fb60a/pkg/process/utils.go#L131)

Okay. The last revision uses os.Process instead.

xibz · 2020-02-03T21:21:56Z

machine.go

-			m.fatalErr = err
-		}
+		<-ctx.Done()
+		m.stopVMM()


Should we be ignoring this error?

The error from stopVMM(). Yeah, I think we should not ignore the error. Let me fix.

The last commit logs the error. Passing that to errCh seems harder than I thought, since the channel will be closed after the underlying process is finished.

The method should guarantee that the underlying process is dead, but it doesn't. Signed-off-by: Kazuyoshi Kato <[email protected]>

The context is used to let the SDK kill its underlying Firecracker process. Closing the channel has to be happening after the actual termination of the process. Signed-off-by: Kazuyoshi Kato <[email protected]>

Regardless of how the machine is stopped, Wait() should gurantee that the process is not there. Signed-off-by: Kazuyoshi Kato <[email protected]>

machine_test.go

VMCommandBuilder#Build()'s context directly controls exec.Cmd. To test the goroutines inside Machine#startVMM(), we cannot use VMCommandBuilder. Signed-off-by: Kazuyoshi Kato <[email protected]>

xibz

firecracker-microvm/firecracker-go-sdk#182

The fix below is needed to fix the issue. While the SDK version is 0.20.x, it should work with Firecracker 0.19.x as well. firecracker-microvm/firecracker-go-sdk#182 Signed-off-by: Kazuyoshi Kato <[email protected]>

Integrate fccontrol plugin with shim.

kzys marked this pull request as ready for review January 31, 2020 18:51

kzys changed the title ~~Test that Machine#Wait() with context cancellation~~ Make sure Machine#Wait() gurantee that the underlying Firecracker process is stopped Jan 31, 2020

kzys force-pushed the wait-and-cancel branch from faa04fe to 7051355 Compare January 31, 2020 19:41

smira approved these changes Feb 3, 2020

View reviewed changes

kzys requested review from xibz, IRCody and mxpv February 3, 2020 19:02

mxpv reviewed Feb 3, 2020

View reviewed changes

xibz reviewed Feb 3, 2020

View reviewed changes

kzys force-pushed the wait-and-cancel branch 2 times, most recently from 77dd681 to 05aa740 Compare February 4, 2020 01:03

kzys added 3 commits February 4, 2020 13:54

Test that Machine#Wait() with context cancellation

2fd80c0

The method should guarantee that the underlying process is dead, but it doesn't. Signed-off-by: Kazuyoshi Kato <[email protected]>

Don't close m.exitCh just after context cancelletion

8dba8bd

The context is used to let the SDK kill its underlying Firecracker process. Closing the channel has to be happening after the actual termination of the process. Signed-off-by: Kazuyoshi Kato <[email protected]>

Combine multiple test cases around Wait()

ec7ef18

Regardless of how the machine is stopped, Wait() should gurantee that the process is not there. Signed-off-by: Kazuyoshi Kato <[email protected]>

kzys force-pushed the wait-and-cancel branch from 05aa740 to d30ecbb Compare February 4, 2020 21:56

xibz suggested changes Feb 4, 2020

View reviewed changes

machine_test.go Outdated Show resolved Hide resolved

Use Start's context correctly

25ecff7

VMCommandBuilder#Build()'s context directly controls exec.Cmd. To test the goroutines inside Machine#startVMM(), we cannot use VMCommandBuilder. Signed-off-by: Kazuyoshi Kato <[email protected]>

kzys force-pushed the wait-and-cancel branch from d30ecbb to 25ecff7 Compare February 4, 2020 22:16

xibz approved these changes Feb 4, 2020

View reviewed changes

mxpv approved these changes Feb 4, 2020

View reviewed changes

mxpv merged commit c569328 into firecracker-microvm:master Feb 4, 2020

kzys added a commit to kzys/firecracker-containerd that referenced this pull request Feb 10, 2020

Upgrade Go SDK to fix Machine#Wait()

cc81160

firecracker-microvm/firecracker-go-sdk#182

thepwagner mentioned this pull request Feb 19, 2020

machine.Start() context cancellation ignores clean exit #200

Closed

pendo324 pushed a commit to pendo324/firecracker-go-sdk that referenced this pull request Aug 27, 2024

Merge pull request firecracker-microvm#182 from sipsma/fccontrol

6d2781e

Integrate fccontrol plugin with shim.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure Machine#Wait() gurantee that the underlying Firecracker process is stopped #182

Make sure Machine#Wait() gurantee that the underlying Firecracker process is stopped #182

kzys commented Jan 31, 2020 •

edited

Loading

smira left a comment

mxpv Feb 3, 2020 •

edited

Loading

mxpv Feb 3, 2020

kzys Feb 3, 2020

xibz Feb 4, 2020 •

edited

Loading

kzys Feb 4, 2020 •

edited

Loading

xibz Feb 4, 2020 •

edited

Loading

kzys Feb 4, 2020

mxpv Feb 4, 2020

kzys Feb 4, 2020

xibz Feb 3, 2020

kzys Feb 3, 2020

kzys Feb 3, 2020

xibz left a comment

Make sure Machine#Wait() gurantee that the underlying Firecracker process is stopped #182

Make sure Machine#Wait() gurantee that the underlying Firecracker process is stopped #182

Conversation

kzys commented Jan 31, 2020 • edited Loading

smira left a comment

Choose a reason for hiding this comment

mxpv Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xibz Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

kzys Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

xibz Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment

kzys commented Jan 31, 2020 •

edited

Loading

mxpv Feb 3, 2020 •

edited

Loading

xibz Feb 4, 2020 •

edited

Loading

kzys Feb 4, 2020 •

edited

Loading

xibz Feb 4, 2020 •

edited

Loading