Make LogFifoPath and MetricsFifoPath configurable

Before this change, firecracker-containerd was returning the paths of
the files as a part of CreateVM's response. However the files are also
useful to investage when CreateVM doesn't finish successfully.

Fixes #449

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

Author: Kazuyoshi Kato

proto/firecracker.pb.go

@@ -36,6 +36,9 @@ message CreateVMRequest {
     JailerConfig JailerConfig = 10;
 
     uint32 TimeoutSeconds = 11;
+
+    string LogFifoPath = 12;
+    string MetricsFifoPath = 13;
 }
 
 message CreateVMResponse {

proto/firecracker.proto

@@ -270,32 +270,41 @@ func (j *runcJailer) BuildJailedRootHandler(cfg *config.Config, machineConfig *f
 	}
 }
 
+func (j *runcJailer) makeLinkInJail(src string) (string, error) {
+	root := j.RootPath()
+
+	base := filepath.Base(src)
+	dst := filepath.Join(root, base)
+
+	// Since Firecracker is unaware that we are in a jailed environment and
+	// what owner/group to set this as when creating, we will manually have
+	// to adjust the permission bits ourselves
+	if err := linkAndChown(src, dst, j.Config.UID, j.Config.GID); err != nil {
+		return "", err
+	}
+
+	// this path needs to be relative to the root path, and since we are
+	// placing the file in the root path the value should just be the file name.
+	return base, nil
+}
+
 // BuildLinkFifoHandler will return a new firecracker.Handler with the function
 // that will allow linking of the fifos making them visible to Firecracker.
 func (j *runcJailer) BuildLinkFifoHandler() firecracker.Handler {
 	return firecracker.Handler{
 		Name: jailerFifoHandlerName,
 		Fn: func(ctx context.Context, m *firecracker.Machine) error {
-			contentsPath := j.RootPath()
-			fifoFileName := filepath.Base(m.Cfg.LogFifo)
-			newFifoPath := filepath.Join(contentsPath, fifoFileName)
-			// Since Firecracker is unaware that we are in a jailed environment and
-			// what owner/group to set this as when creating, we will manually have
-			// to adjust the permission bits ourselves
-			if err := linkAndChown(m.Cfg.LogFifo, newFifoPath, j.Config.UID, j.Config.GID); err != nil {
+			logFifo, err := j.makeLinkInJail(m.Cfg.LogPath)
+			if err != nil {
 				return err
 			}
-			// this path needs to be relative to the root path, and since we are
-			// placing the file in the root path the LogFifo value should just be the
-			// file name.
-			m.Cfg.LogFifo = fifoFileName
-
-			metricFifoFileName := filepath.Base(m.Cfg.MetricsFifo)
-			newMetricFifoPath := filepath.Join(contentsPath, metricFifoFileName)
-			if err := linkAndChown(m.Cfg.MetricsFifo, newMetricFifoPath, j.Config.UID, j.Config.GID); err != nil {
+			m.Cfg.LogFifo = logFifo
+
+			metricsFifo, err := j.makeLinkInJail(m.Cfg.MetricsPath)
+			if err != nil {
 				return err
 			}
-			m.Cfg.MetricsFifo = metricFifoFileName
+			m.Cfg.MetricsFifo = metricsFifo
 
 			return nil
 		},

runtime/runc_jailer.go

@@ -24,6 +24,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	// disable gosec check for math/rand. We just need a random starting
@@ -635,8 +636,8 @@ func (s *service) GetVMInfo(requestCtx context.Context, request *proto.GetVMInfo
 	return &proto.GetVMInfoResponse{
 		VMID:            s.vmID,
 		SocketPath:      s.shimDir.FirecrackerSockPath(),
-		LogFifoPath:     s.machineConfig.LogFifo,
-		MetricsFifoPath: s.machineConfig.MetricsFifo,
+		LogFifoPath:     s.machineConfig.LogPath,
+		MetricsFifoPath: s.machineConfig.MetricsPath,
 		CgroupPath:      cgroupPath,
 	}, nil
 }
@@ -736,13 +737,34 @@ func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker
 			Path: relVSockPath,
 			ID:   "agent_api",
 		}},
-		LogFifo:     s.shimDir.FirecrackerLogFifoPath(),
-		MetricsFifo: s.shimDir.FirecrackerMetricsFifoPath(),
-		MachineCfg:  machineConfigurationFromProto(s.config, req.MachineCfg),
-		LogLevel:    s.config.DebugHelper.GetFirecrackerLogLevel(),
-		VMID:        s.vmID,
+		MachineCfg: machineConfigurationFromProto(s.config, req.MachineCfg),
+		LogLevel:   s.config.DebugHelper.GetFirecrackerLogLevel(),
+		VMID:       s.vmID,
 	}
 
+	logPath := s.shimDir.FirecrackerLogFifoPath()
+	if req.LogFifoPath != "" {
+		logPath = req.LogFifoPath
+	}
+	err = syscall.Mkfifo(logPath, 0700)
+	if err != nil {
+		return nil, err
+	}
+
+	metricsPath := s.shimDir.FirecrackerMetricsFifoPath()
+	if req.MetricsFifoPath != "" {
+		metricsPath = req.MetricsFifoPath
+	}
+	err = syscall.Mkfifo(metricsPath, 0700)
+	if err != nil {
+		return nil, err
+	}
+
+	// The Config struct has LogFifo and MetricsFifo, but they will be deprecated since
+	// Firecracker doesn't have the corresponding fields anymore.
+	cfg.LogPath = logPath
+	cfg.MetricsPath = metricsPath
+
 	if req.JailerConfig != nil {
 		cfg.NetNS = req.JailerConfig.NetNS
 	}

runtime/service.go

@@ -14,6 +14,7 @@
 package main
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"fmt"
@@ -1870,6 +1871,23 @@ loop:
 	require.Equal(t, expected, actual)
 }
 
+func requireNonEmptyFifo(t testing.TB, path string) {
+	file, err := os.Open(path)
+	require.NoError(t, err)
+	defer file.Close()
+
+	reader := bufio.NewReader(file)
+
+	// Since this is a FIFO, reading till EOF would block if its writer-end is still open.
+	line, err := reader.ReadString('\n')
+	require.NoError(t, err)
+	require.NotEqualf(t, "", line, "%s must not be empty", path)
+
+	info, err := os.Stat(path)
+	require.NoError(t, err)
+	require.Equal(t, os.ModeNamedPipe, info.Mode()&os.ModeType, "%s is a FIFO file", path)
+}
+
 func TestCreateVM_Isolated(t *testing.T) {
 	prepareIntegTest(t)
 
@@ -1923,25 +1941,55 @@ func TestCreateVM_Isolated(t *testing.T) {
 		},
 	}
 
+	runTest := func(t *testing.T, request proto.CreateVMRequest, validate func(t *testing.T, err error)) {
+		vmID := testNameToVMID(t.Name())
+
+		tempDir, err := ioutil.TempDir("", vmID)
+		require.NoError(t, err)
+		defer os.RemoveAll(tempDir)
+
+		logFile := filepath.Join(tempDir, "log.fifo")
+		metricsFile := filepath.Join(tempDir, "metrics.fifo")
+
+		request.VMID = vmID
+		request.LogFifoPath = logFile
+		request.MetricsFifoPath = metricsFile
+
+		resp, createVMErr := fcClient.CreateVM(ctx, &request)
+
+		// Even CreateVM fails, the log file and the metrics file must have some data.
+		requireNonEmptyFifo(t, logFile)
+		requireNonEmptyFifo(t, metricsFile)
+
+		// Some test cases are expected to have an error, some are not.
+		validate(t, createVMErr)
+
+		// No VM to stop.
+		if createVMErr != nil {
+			return
+		}
+
+		// Ensure the response fields are populated correctly
+		assert.Equal(t, request.VMID, resp.VMID)
+
+		_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: request.VMID})
+		require.Equal(t, status.Code(err), codes.OK)
+	}
+
 	for _, subtest := range subtests {
 		request := subtest.request
 		validate := subtest.validate
-
 		t.Run(subtest.name, func(t *testing.T) {
-			request.VMID = testNameToVMID(t.Name())
-
-			resp, err := fcClient.CreateVM(ctx, &request)
-			validate(t, err)
-			if err != nil {
-				// No VM to stop.
-				return
-			}
-
-			// Ensure the response fields are populated correctly
-			assert.Equal(t, request.VMID, resp.VMID)
+			runTest(t, request, validate)
+		})
 
-			_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: request.VMID})
-			require.Equal(t, status.Code(err), codes.OK)
+		requestWithJailer := subtest.request
+		requestWithJailer.JailerConfig = &proto.JailerConfig{
+			UID: 30000,
+			GID: 30000,
+		}
+		t.Run(subtest.name+" with Jailer", func(t *testing.T) {
+			runTest(t, requestWithJailer, validate)
 		})
 	}
 }

runtime/service_integ_test.go

@@ -253,8 +253,8 @@ func TestBuildVMConfiguration(t *testing.T) {
 				Path: relVSockPath,
 				ID:   "agent_api",
 			}}
-			tc.expectedCfg.LogFifo = svc.shimDir.FirecrackerLogFifoPath()
-			tc.expectedCfg.MetricsFifo = svc.shimDir.FirecrackerMetricsFifoPath()
+			tc.expectedCfg.LogPath = svc.shimDir.FirecrackerLogFifoPath()
+			tc.expectedCfg.MetricsPath = svc.shimDir.FirecrackerMetricsFifoPath()
 
 			drives := make([]models.Drive, tc.expectedStubDriveCount)
 			for i := 0; i < tc.expectedStubDriveCount; i++ {