Skip to content

Commit 04acce5

Browse files
committed
stopVMM: don't signal if stopped
After the context passed to `Start()` is cancelled, the "kill the process by context cancellation" goroutine started from `startVMM()` invokes `stopVMM()`. https://github.com/firecracker-microvm/firecracker-go-sdk/blob/master/machine.go#L553-L559 A typical user may cancel the context passed to `Start()` after the firecracker VM has terminated successfully. This causes a "process already finished" error to be logged, but no real negatives. This PR extends stopVMM() to no longer send SIGTERM if `Wait()` has returned (since that signals the process has already completed). https://github.com/firecracker-microvm/firecracker-go-sdk/blob/master/machine.go#L519-L524 Since the `stopVMM()` happens async and the only effect is a log message, I modified the `TestWait()` harness to: - Wait for context cancellation to propagate (`1s`) - Capture and verify logs didn't emit the error Signed-off-by: Peter Wagner <[email protected]>
1 parent f8acfc4 commit 04acce5

File tree

2 files changed

+12
-2
lines changed

2 files changed

+12
-2
lines changed

machine.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -576,7 +576,7 @@ func (m *Machine) StopVMM() error {
576576
}
577577

578578
func (m *Machine) stopVMM() error {
579-
if m.cmd != nil && m.cmd.Process != nil {
579+
if m.cmd != nil && m.cmd.Process != nil && m.cmd.ProcessState == nil {
580580
m.logger.Debug("stopVMM(): sending sigterm to firecracker")
581581
return m.cmd.Process.Signal(syscall.SIGTERM)
582582
}

machine_test.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1139,6 +1139,7 @@ func TestWait(t *testing.T) {
11391139
m.StopVMM()
11401140
time.Sleep(1 * time.Second)
11411141
cancel()
1142+
time.Sleep(1 * time.Second)
11421143
},
11431144
},
11441145
}
@@ -1151,13 +1152,18 @@ func TestWait(t *testing.T) {
11511152
socketPath := filepath.Join(testDataPath, fsSafeTestName.Replace(t.Name()))
11521153
defer os.Remove(socketPath)
11531154

1155+
// Tee logs for validation:
1156+
var logBuffer bytes.Buffer
1157+
machineLogger := logrus.New()
1158+
machineLogger.Out = io.MultiWriter(os.Stderr, &logBuffer)
1159+
11541160
cfg := createValidConfig(t, socketPath)
11551161
m, err := NewMachine(ctx, cfg, func(m *Machine) {
11561162
// Rewriting m.cmd partially wouldn't work since Cmd has
11571163
// some unexported members
11581164
args := m.cmd.Args[1:]
11591165
m.cmd = exec.Command(getFirecrackerBinaryPath(), args...)
1160-
})
1166+
}, WithLogger(logrus.NewEntry(machineLogger)))
11611167
require.NoError(t, err)
11621168

11631169
err = m.Start(vmContext)
@@ -1186,6 +1192,10 @@ func TestWait(t *testing.T) {
11861192
require.Equal(t, "os: process already finished", err.Error())
11871193

11881194
wg.Wait()
1195+
1196+
machineLogs := logBuffer.String()
1197+
assert.NotContains(t, machineLogs, "level=error")
1198+
assert.NotContains(t, machineLogs, "process already finished")
11891199
})
11901200
}
11911201
}

0 commit comments

Comments
 (0)