From c3bc5136e2f33d339cfb7b09f96806b4b1bb13f7 Mon Sep 17 00:00:00 2001
From: Kazuyoshi Kato <katokazu@amazon.com>
Date: Tue, 18 Feb 2020 15:53:35 -0800
Subject: [PATCH 1/2] Reproduce #198

Since the cause is a process-level resource leak, running these
benchmarks in a same process
(e.g. go test -bench=BenchmarkForwardSignals) doesn't show
the difference.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
---
 benchmark_test.go | 136 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)
 create mode 100644 benchmark_test.go

diff --git a/benchmark_test.go b/benchmark_test.go
new file mode 100644
index 000000000..7d3d627d1
--- /dev/null
+++ b/benchmark_test.go
@@ -0,0 +1,136 @@
+package firecracker
+
+import (
+	"bufio"
+	"context"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+
+	"github.com/sirupsen/logrus"
+
+	models "github.com/firecracker-microvm/firecracker-go-sdk/client/models"
+)
+
+const numberOfVMs = 200
+
+func createMachine(ctx context.Context, name string, forwardSignals []os.Signal) (*Machine, func(), error) {
+	dir, err := ioutil.TempDir("", name)
+	if err != nil {
+		return nil, nil, err
+	}
+	cleanup := func() {
+		os.RemoveAll(dir)
+	}
+
+	socketPath := filepath.Join(dir, "api.sock")
+	vmlinuxPath := filepath.Join(testDataPath, "./vmlinux")
+	logFifo := filepath.Join(dir, "log.fifo")
+	metrics := filepath.Join(dir, "metrics.fifo")
+
+	config := Config{
+		SocketPath:      socketPath,
+		KernelImagePath: vmlinuxPath,
+		LogFifo:         logFifo,
+		MetricsFifo:     metrics,
+		LogLevel:        "Info",
+		MachineCfg: models.MachineConfiguration{
+			VcpuCount:   Int64(1),
+			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),
+			},
+		},
+		ForwardSignals: forwardSignals,
+	}
+
+	cmd := VMCommandBuilder{}.
+		WithSocketPath(socketPath).
+		WithBin(getFirecrackerBinaryPath()).
+		Build(ctx)
+
+	log := logrus.New()
+	log.SetLevel(logrus.FatalLevel)
+	machine, err := NewMachine(ctx, config, WithProcessRunner(cmd), WithLogger(logrus.NewEntry(log)))
+	if err != nil {
+		return nil, cleanup, err
+	}
+
+	return machine, cleanup, nil
+}
+
+func startAndWaitVM(ctx context.Context, m *Machine) error {
+	err := m.Start(ctx)
+	if err != nil {
+		return err
+	}
+
+	file, err := os.Open(m.LogFile())
+	if err != nil {
+		return err
+	}
+
+	scanner := bufio.NewScanner(file)
+	for scanner.Scan() {
+		line := scanner.Text()
+		if strings.Contains(line, "Guest-boot-time") {
+			break
+		}
+	}
+	err = m.StopVMM()
+	if err != nil {
+		return err
+	}
+
+	err = m.Wait(ctx)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func benchmarkForwardSignals(b *testing.B, forwardSignals []os.Signal) {
+	ctx := context.Background()
+
+	b.Logf("%s: %d", b.Name(), b.N)
+
+	for i := 0; i < b.N; i++ {
+		var wg sync.WaitGroup
+		for j := 0; j < numberOfVMs; j++ {
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+
+				machine, cleanup, err := createMachine(ctx, b.Name(), forwardSignals)
+				if err != nil {
+					b.Fatalf("failed to create a VM: %s", err)
+				}
+				defer cleanup()
+
+				err = startAndWaitVM(ctx, machine)
+				if err != nil && !strings.Contains(err.Error(), "signal: terminated") {
+					b.Fatalf("failed to start the VM: %s", err)
+				}
+			}()
+		}
+		wg.Wait()
+	}
+}
+func BenchmarkForwardSignalsDefault(t *testing.B) {
+	benchmarkForwardSignals(t, nil)
+}
+
+func BenchmarkForwardSignalsDisable(t *testing.B) {
+	benchmarkForwardSignals(t, []os.Signal{})
+}

From f8cf437394bf926abd6e2c59e034867299000a58 Mon Sep 17 00:00:00 2001
From: Kazuyoshi Kato <katokazu@amazon.com>
Date: Thu, 27 Feb 2020 14:08:57 -0800
Subject: [PATCH 2/2] Remove signal handling channels correctly

The "break" here only break out from the inner select statement.
The for loop is keep running and signal.Stop() won't be called.

Seems that is the cause of #198.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
---
 machine.go | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/machine.go b/machine.go
index c71bf759c..9e7afc389 100644
--- a/machine.go
+++ b/machine.go
@@ -948,13 +948,16 @@ func (m *Machine) setupSignals() {
 	signal.Notify(sigchan, signals...)
 
 	go func() {
+	ForLoop:
 		for {
 			select {
 			case sig := <-sigchan:
-				m.logger.Printf("Caught signal %s", sig)
+				m.logger.Debugf("Caught signal %s", sig)
+				// Some signals kill the process, some of them are not.
 				m.cmd.Process.Signal(sig)
 			case <-m.exitCh:
-				break
+				// And if a signal kills the process, we can stop this for loop and remove sigchan.
+				break ForLoop
 			}
 		}