Merge pull request #158 from kzys/enable-race-detector

Enable Go's race detector

Author: kzys

.buildkite/pipeline.yml

@@ -52,7 +52,7 @@ steps:
       - 'ln -s /var/lib/fc-ci/rootfs.ext4 testdata/root-drive.img'
       - 'ln -s /usr/local/bin/firecracker-v0.19.0 testdata/firecracker'
       - 'ln -s /usr/local/bin/jailer-v0.19.0 testdata/jailer'
-      - "FC_TEST_TAP=fc-test-tap${BUILDKITE_BUILD_NUMBER} make test EXTRAGOARGS='-v -count=1' DISABLE_ROOT_TESTS=true"
+      - "FC_TEST_TAP=fc-test-tap${BUILDKITE_BUILD_NUMBER} make test EXTRAGOARGS='-v -count=1 -race' DISABLE_ROOT_TESTS=true"
     agents:
       queue: "${BUILDKITE_AGENT_META_DATA_QUEUE:-default}"
 
@@ -63,7 +63,7 @@ steps:
       - 'cp /usr/local/bin/firecracker-v0.19.0 testdata/firecracker'
       - 'cp /usr/local/bin/jailer-v0.19.0 testdata/jailer'
       - 'make -C cni install CNI_BIN_ROOT=$(pwd)/testdata/bin'
-      - "sudo FC_TEST_TAP=fc-root-tap${BUILDKITE_BUILD_NUMBER} make test EXTRAGOARGS='-v -count=1' DISABLE_ROOT_TESTS="
+      - "sudo FC_TEST_TAP=fc-root-tap${BUILDKITE_BUILD_NUMBER} make test EXTRAGOARGS='-v -count=1 -race' DISABLE_ROOT_TESTS="
     agents:
       queue: "${BUILDKITE_AGENT_META_DATA_QUEUE:-default}"
 

machine.go

@@ -584,6 +584,10 @@ func (m *Machine) setupLogging(ctx context.Context) error {
 }
 
 func (m *Machine) captureFifoToFile(ctx context.Context, logger *log.Entry, fifoPath string, w io.Writer) error {
+	return m.captureFifoToFileWithChannel(ctx, logger, fifoPath, w, make(chan error, 1))
+}
+
+func (m *Machine) captureFifoToFileWithChannel(ctx context.Context, logger *log.Entry, fifoPath string, w io.Writer, done chan error) error {
 	// open the fifo pipe which will be used
 	// to write its contents to a file.
 	fifoPipe, err := fifo.OpenFifo(ctx, fifoPath, syscall.O_RDONLY|syscall.O_NONBLOCK, 0600)
@@ -620,7 +624,10 @@ func (m *Machine) captureFifoToFile(ctx context.Context, logger *log.Entry, fifo
 
 		if _, err := io.Copy(w, fifoPipe); err != nil {
 			logger.WithError(err).Warn("io.Copy failed to copy contents of fifo pipe")
+			done <- err
 		}
+
+		close(done)
 	}()
 
 	return nil

machine_test.go

@@ -1073,20 +1073,19 @@ func TestCaptureFifoToFile_leak(t *testing.T) {
 	logger := fctesting.NewLogEntry(t)
 	logger.Logger.Level = logrus.WarnLevel
 	logger.Logger.Out = loggerBuffer
-	err = m.captureFifoToFile(context.Background(), logger, fifoPath, buf)
+
+	done := make(chan error)
+	err = m.captureFifoToFileWithChannel(context.Background(), logger, fifoPath, buf, done)
 	assert.NoError(t, err, "failed to capture fifo to file")
+
+	// Stopping the machine will close the FIFO
 	close(m.exitCh)
 
-	// wait sometime for the logs to populate
-	time.Sleep(250 * time.Millisecond)
-	expectedClosedFifo := `reading from a closed fifo`
-	expectedClosedFile := `file already closed`
-	logs := loggerBuffer.String()
-	if !(strings.Contains(logs, expectedClosedFifo) ||
-		strings.Contains(logs, expectedClosedFile)) {
+	// Waiting the channel to make sure that the contents of the FIFO has been copied
+	copyErr := <-done
 
-		t.Errorf("logs did not container a closed fifo error or closed file")
-	}
+	assert.Contains(t, copyErr.Error(), `file already closed`, "error")
+	assert.Contains(t, loggerBuffer.String(), `file already closed`, "log")
 }
 
 func TestWaitWithKill(t *testing.T) {

network_test.go

@@ -295,19 +295,25 @@ func TestNetworkMachineCNI(t *testing.T) {
 	var vmWg sync.WaitGroup
 	for i := 0; i < numVMs; i++ {
 		vmWg.Add(1)
-		go func(vmID string) {
-			defer vmWg.Done()
 
-			ctx, cancel := context.WithCancel(context.Background())
+		vmID := fmt.Sprintf("%d-%s-%d", timestamp, networkName, i)
+
+		firecrackerSockPath := filepath.Join(testCNIDir, fmt.Sprintf("%s.sock", vmID))
+		rootfsPath := filepath.Join(testCNIDir, fmt.Sprintf("%s.img", vmID))
+
+		ctx, cancel := context.WithCancel(context.Background())
+		// NewMachine cannot be in the goroutine below, since go-openapi/runtime has a globally-shared mutable logger...
+		// https://github.com/go-openapi/runtime/blob/553c9d1fb273d9550562d9f76949a413af265138/client/runtime.go#L463
+		m := newCNIMachine(t, ctx, firecrackerSockPath, rootfsPath, cniConfDir, cniCacheDir, networkName, ifName, vmID, cniBinPath)
+
+		go func(ctx context.Context, cancel func(), m *Machine, vmID string) {
+			defer vmWg.Done()
 			defer cancel()
 
-			firecrackerSockPath := filepath.Join(testCNIDir, fmt.Sprintf("%s.sock", vmID))
-			rootfsPath := filepath.Join(testCNIDir, fmt.Sprintf("%s.img", vmID))
 			expectedCacheDirPath := filepath.Join(cniCacheDir, "results",
 				fmt.Sprintf("%s-%s-%s", networkName, vmID, ifName))
 
-			m, vmIP := startCNIMachine(t, ctx,
-				firecrackerSockPath, rootfsPath, cniConfDir, cniCacheDir, networkName, ifName, vmID, cniBinPath)
+			vmIP := startCNIMachine(t, ctx, m)
 			vmIPs <- vmIP
 
 			assert.FileExists(t, expectedCacheDirPath, "CNI cache dir doesn't exist after vm startup")
@@ -326,7 +332,7 @@ func TestNetworkMachineCNI(t *testing.T) {
 			_, err := os.Stat(expectedCacheDirPath)
 			assert.True(t, os.IsNotExist(err), "expected CNI cache dir to not exist after vm exit")
 
-		}(fmt.Sprintf("%d-%s-%d", timestamp, networkName, i))
+		}(ctx, cancel, m, vmID)
 	}
 	vmWg.Wait()
 	close(vmIPs)
@@ -341,7 +347,7 @@ func TestNetworkMachineCNI(t *testing.T) {
 	}
 }
 
-func startCNIMachine(t *testing.T,
+func newCNIMachine(t *testing.T,
 	ctx context.Context,
 	firecrackerSockPath,
 	rootfsPath,
@@ -351,7 +357,7 @@ func startCNIMachine(t *testing.T,
 	ifName,
 	vmID string,
 	cniBinPath []string,
-) (*Machine, string) {
+) *Machine {
 	rootfsBytes, err := ioutil.ReadFile(testRootfs)
 	require.NoError(t, err, "failed to read rootfs file")
 	err = ioutil.WriteFile(rootfsPath, rootfsBytes, 0666)
@@ -391,7 +397,11 @@ func startCNIMachine(t *testing.T,
 	}, WithProcessRunner(cmd))
 	require.NoError(t, err, "failed to create machine with CNI network interface")
 
-	err = m.Start(context.Background())
+	return m
+}
+
+func startCNIMachine(t *testing.T, ctx context.Context, m *Machine) string {
+	err := m.Start(ctx)
 	require.NoError(t, err, "failed to start machine")
 
 	staticConfig := m.Cfg.NetworkInterfaces[0].StaticConfiguration
@@ -406,7 +416,7 @@ func startCNIMachine(t *testing.T,
 	require.NotNil(t, ipConfig,
 		"cni configuration should have updated network interface ip configuration")
 
-	return m, ipConfig.IPAddr.IP.String()
+	return ipConfig.IPAddr.IP.String()
 }
 
 func testPing(t *testing.T, ip string, count int, timeout time.Duration) {