Merge pull request #153 from kzys/exit-error

Report its exit status from Wait() correctly

Author: Kazuyoshi Kato

machine.go

@@ -173,8 +173,8 @@ type Machine struct {
 	startOnce sync.Once
 	// exitCh is a channel which gets closed when the VMM exits
 	exitCh chan struct{}
-	// err records any error executing the VMM
-	err error
+	// fatalErr records an error that either stops or prevent starting the VMM
+	fatalErr error
 
 	// callbacks that should be run when the machine is being torn down
 	cleanupOnce  sync.Once
@@ -349,7 +349,7 @@ func (m *Machine) Wait(ctx context.Context) error {
 	case <-ctx.Done():
 		return ctx.Err()
 	case <-m.exitCh:
-		return m.err
+		return m.fatalErr
 	}
 }
 
@@ -443,7 +443,10 @@ func (m *Machine) startVMM(ctx context.Context) error {
 
 	if err != nil {
 		m.logger.Errorf("Failed to start VMM: %s", err)
+
+		m.fatalErr = err
 		close(m.exitCh)
+
 		return err
 	}
 	m.logger.Debugf("VMM started socket path is %s", m.Cfg.SocketPath)
@@ -459,17 +462,19 @@ func (m *Machine) startVMM(ctx context.Context) error {
 
 	errCh := make(chan error)
 	go func() {
-		if err := m.cmd.Wait(); err != nil {
-			m.logger.Warnf("firecracker exited: %s", err.Error())
+		waitErr := m.cmd.Wait()
+		if waitErr != nil {
+			m.logger.Warnf("firecracker exited: %s", waitErr.Error())
 		} else {
 			m.logger.Printf("firecracker exited: status=0")
 		}
 
-		if err := m.doCleanup(); err != nil {
-			m.logger.Errorf("failed to cleanup after VM exit: %v", err)
+		cleanupErr := m.doCleanup()
+		if cleanupErr != nil {
+			m.logger.Errorf("failed to cleanup after VM exit: %v", cleanupErr)
 		}
 
-		errCh <- err
+		errCh <- multierror.Append(waitErr, cleanupErr).ErrorOrNil()
 
 		// Notify subscribers that there will be no more values.
 		// When err is nil, two reads are performed (waitForSocket and close exitCh goroutine),
@@ -495,17 +500,18 @@ func (m *Machine) startVMM(ctx context.Context) error {
 	// Wait for firecracker to initialize:
 	err = m.waitForSocket(3*time.Second, errCh)
 	if err != nil {
-		msg := fmt.Sprintf("Firecracker did not create API socket %s: %s", m.Cfg.SocketPath, err)
-		err = errors.New(msg)
+		err = errors.Wrapf(err, "Firecracker did not create API socket %s", m.Cfg.SocketPath)
+		m.fatalErr = err
 		close(m.exitCh)
+
 		return err
 	}
 	go func() {
 		select {
 		case <-ctx.Done():
-			m.err = ctx.Err()
+			m.fatalErr = ctx.Err()
 		case err := <-errCh:
-			m.err = err
+			m.fatalErr = err
 		}
 
 		signal.Stop(sigchan)

machine_test.go

@@ -1075,3 +1075,115 @@ func TestCaptureFifoToFile_leak(t *testing.T) {
 	expected := `io.Copy failed to copy contents of fifo pipe: read testdata/TestCaptureFifoToFileLeak.fifo: file already closed`
 	assert.Contains(t, loggerBuffer.String(), expected)
 }
+
+func TestWaitWithKill(t *testing.T) {
+	fctesting.RequiresRoot(t)
+	ctx := context.Background()
+
+	socketPath := filepath.Join(testDataPath, t.Name())
+	defer os.Remove(socketPath)
+
+	cfg := createValidConfig(t, socketPath)
+	cmd := VMCommandBuilder{}.
+		WithSocketPath(cfg.SocketPath).
+		WithBin(getFirecrackerBinaryPath()).
+		Build(ctx)
+	m, err := NewMachine(ctx, cfg, WithProcessRunner(cmd))
+	require.NoError(t, err)
+
+	err = m.Start(ctx)
+	require.NoError(t, err)
+
+	go func() {
+		pid, err := m.PID()
+		require.NoError(t, err)
+
+		process, err := os.FindProcess(pid)
+		require.NoError(t, err)
+
+		err = process.Kill()
+		require.NoError(t, err)
+	}()
+
+	err = m.Wait(ctx)
+	require.Error(t, err, "Firecracker was killed and it must be reported")
+}
+
+func TestWaitWithInvalidBinary(t *testing.T) {
+	ctx := context.Background()
+
+	socketPath := filepath.Join(testDataPath, t.Name())
+	defer os.Remove(socketPath)
+
+	cfg := createValidConfig(t, socketPath)
+	cmd := VMCommandBuilder{}.
+		WithSocketPath(socketPath).
+		WithBin("invalid-bin").
+		Build(ctx)
+	m, err := NewMachine(ctx, cfg, WithProcessRunner(cmd))
+	require.NoError(t, err)
+
+	ch := make(chan error)
+
+	go func() {
+		err := m.Wait(ctx)
+		require.Error(t, err, "Wait() reports an error")
+		ch <- err
+	}()
+
+	err = m.Start(ctx)
+	require.Error(t, err, "Start() reports an error")
+
+	select {
+	case errFromWait := <-ch:
+		require.Equal(t, errFromWait, err)
+	}
+}
+
+func TestWaitWithNoSocket(t *testing.T) {
+	ctx := context.Background()
+
+	socketPath := filepath.Join(testDataPath, t.Name())
+	defer os.Remove(socketPath)
+	cfg := createValidConfig(t, socketPath)
+
+	m, err := NewMachine(ctx, cfg, WithProcessRunner(exec.Command("sleep", "10")))
+	require.NoError(t, err)
+
+	ch := make(chan error)
+
+	go func() {
+		err := m.Wait(ctx)
+		require.Error(t, err, "Wait() reports an error")
+		ch <- err
+	}()
+
+	err = m.Start(ctx)
+	require.Error(t, err, "Start() reports an error")
+
+	select {
+	case errFromWait := <-ch:
+		require.Equal(t, errFromWait, err)
+	}
+}
+
+func createValidConfig(t *testing.T, socketPath string) Config {
+	return Config{
+		SocketPath:      socketPath,
+		KernelImagePath: getVmlinuxPath(t),
+		MachineCfg: models.MachineConfiguration{
+			VcpuCount:   Int64(2),
+			CPUTemplate: models.CPUTemplate(models.CPUTemplateT2),
+			MemSizeMib:  Int64(256),
+			HtEnabled:   Bool(false),
+		},
+		Drives: []models.Drive{
+			{
+				DriveID:      String("root"),
+				IsRootDevice: Bool(true),
+				IsReadOnly:   Bool(true),
+				PathOnHost:   String(testRootfs),
+			},
+		},
+	}
+}

network_test.go

@@ -316,7 +316,11 @@ func TestNetworkMachineCNI(t *testing.T) {
 
 			require.NoError(t, m.StopVMM(), "failed to stop machine")
 			waitCtx, waitCancel := context.WithTimeout(ctx, 3*time.Second)
-			assert.NoError(t, m.Wait(waitCtx), "failed waiting for machine stop")
+
+			// Having an error is fine, since StopVM() kills a Firecracker process.
+			// Shutdown() uses SendCtrAltDel action, which doesn't work with the kernel we are using here.
+			// https://github.com/firecracker-microvm/firecracker/issues/1095
+			assert.NotEqual(t, m.Wait(waitCtx), context.DeadlineExceeded, "failed waiting for machine stop")
 			waitCancel()
 
 			_, err := os.Stat(expectedCacheDirPath)