Repository: firecracker-microvm/firecracker-containerd
Pull request #632 (2022-05-19 22:05:33)

fix: cleanup of network devices for CNI tests

*Issue #, if available:*

*Description of changes:*
While running integration tests, I ran into issues with tests not cleaning up the devices on failure.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: Austin Vazquez <macedonv@amazon.com>

kzys: How about using https://pkg.go.dev/testing#T.Cleanup?

Patch:
diff --git a/volume/set.go b/volume/set.go
index a7c7e38..7e7ecb7 100644
--- a/volume/set.go
+++ b/volume/set.go
@@ -36,20 +36,18 @@ const (
 
 // Set is a set of volumes.
 type Set struct {
-	volumes   map[string]*Volume
-	tempDir   string
-	runtime   string
-	volumeDir string
+	volumes map[string]*Volume
+	tempDir string
 }
 
 // NewSet returns a new volume set.
-func NewSet(runtime string) *Set {
-	return NewSetWithTempDir(runtime, os.TempDir())
+func NewSet() *Set {
+	return NewSetWithTempDir(os.TempDir())
 }
 
 // NewSetWithTempDir returns a new volume set and creates all temporary files under the tempDir.
-func NewSetWithTempDir(runtime, tempDir string) *Set {
-	return &Set{runtime: runtime, volumes: make(map[string]*Volume), tempDir: tempDir}
+func NewSetWithTempDir(tempDir string) *Set {
+	return &Set{volumes: make(map[string]*Volume), tempDir: tempDir}
 }
 
 // Add a volume to the set.
@@ -98,38 +96,6 @@ func mountDiskImage(source, target string) error {
 	return mount.All([]mount.Mount{{Type: fsType, Source: source, Options: []string{"loop"}}}, target)
 }
 
-// PrepareDirectory creates a directory that have volumes.
-func (vs *Set) PrepareDirectory(ctx context.Context) (retErr error) {
-	dir, err := os.MkdirTemp(vs.tempDir, "Prepare")
-	if err != nil {
-		retErr = err
-		return
-	}
-	defer func() {
-		if retErr != nil {
-			err := os.Remove(dir)
-			if err != nil {
-				retErr = multierror.Append(retErr, err)
-			}
-		}
-	}()
-
-	for _, v := range vs.volumes {
-		path := filepath.Join(dir, v.name)
-		if v.hostPath == "" {
-			continue
-		}
-		err := fs.CopyDir(path, v.hostPath)
-		if err != nil {
-			retErr = fmt.Errorf("failed to copy volume %q: %w", v.name, err)
-			return
-		}
-	}
-
-	vs.volumeDir = dir
-	return
-}
-
 // PrepareDriveMount returns a FirecrackerDriveMount that could be used with CreateVM.
 func (vs *Set) PrepareDriveMount(ctx context.Context, size int64) (dm *proto.FirecrackerDriveMount, retErr error) {
 	path, err := vs.createDiskImage(ctx, size)
@@ -189,7 +155,6 @@ func (vs *Set) PrepareDriveMount(ctx context.Context, size int64) (dm *proto.Fir
 		FilesystemType: fsType,
 		IsWritable:     true,
 	}
-	vs.volumeDir = vmVolumePath
 	return
 }
 
@@ -221,7 +186,7 @@ func (vs *Set) WithMounts(mountpoints []Mount) (oci.SpecOpts, error) {
 		mounts = append(mounts, specs.Mount{
 			// TODO: for volumes that are provided by the guest (e.g. in-VM snapshotters)
 			// We may be able to have bind-mounts from in-VM snapshotters' mount points.
-			Source:      filepath.Join(vs.volumeDir, v.name),
+			Source:      filepath.Join(vmVolumePath, v.name),
 			Destination: mp.Destination,
 			Type:        "bind",
 			Options:     options,


Test patch:
diff --git a/internal/network_test_utils.go b/internal/network_test_utils.go
index 307f2a9..eb1dc57 100644
--- a/internal/network_test_utils.go
+++ b/internal/network_test_utils.go
@@ -20,6 +20,7 @@ import (
 	"net"
 	"net/http"
 	"os/exec"
+	"testing"
 
 	"github.com/miekg/dns"
 	"github.com/pkg/errors"
@@ -55,7 +56,7 @@ type LocalNetworkServices interface {
 // "domain" to that ip. The HTTP server will serve a pages at
 // paths defined in the keys of "webpages", with the content set
 // in the values of that map.
-func NewLocalNetworkServices(webpages map[string]string) (LocalNetworkServices, error) {
+func NewLocalNetworkServices(t *testing.T, webpages map[string]string) (LocalNetworkServices, error) {
 	testDevName := "testdev0"
 	ipAddr := "10.0.0.1"
 	ipCidr := fmt.Sprintf("%s/32", ipAddr)
@@ -64,16 +65,28 @@ func NewLocalNetworkServices(webpages map[string]string) (LocalNetworkServices,
 	if err != nil {
 		return nil, errors.Wrapf(err, `failed to add tun dev, "ip" command output: %s`, string(output))
 	}
+	cleanupTap := func() {
+		exec.Command("ip", "tuntap", "del", testDevName, "mode", "tun").Run()
+	}
+	t.Cleanup(cleanupTap)
 
 	output, err = exec.Command("ip", "addr", "add", ipCidr, "dev", testDevName).CombinedOutput()
 	if err != nil {
 		return nil, errors.Wrapf(err, `failed to assign ip to tun dev, "ip" command output: %s`, string(output))
 	}
+	cleanupAddress := func() {
+		exec.Command("ip", "addr", "del", ipCidr, "dev", testDevName).Run()
+	}
+	t.Cleanup(cleanupAddress)
 
 	output, err = exec.Command("ip", "link", "set", "dev", testDevName, "up").CombinedOutput()
 	if err != nil {
 		return nil, errors.Wrapf(err, `failed to set tun dev up, "ip" command output: %s`, string(output))
 	}
+	cleanupLink := func() {
+		exec.Command("ip", "link", "del", "dev", testDevName).Run()
+	}
+	t.Cleanup(cleanupLink)
 
 	return &localNetworkServices{
 		domain:   domainName,
diff --git a/runtime/cni_integ_test.go b/runtime/cni_integ_test.go
index 512b6c8..6184bb2 100644
--- a/runtime/cni_integ_test.go
+++ b/runtime/cni_integ_test.go
@@ -64,7 +64,7 @@ func TestCNISupport_Isolated(t *testing.T) {
 		webpages[vmID] = fmt.Sprintf("Hello, my virtual machine %s\n", vmID)
 	}
 
-	localServices, err := internal.NewLocalNetworkServices(webpages)
+	localServices, err := internal.NewLocalNetworkServices(t, webpages)
 	require.NoError(t, err, "failed to create local network test services")
 
 	cniNetworkName := "fcnet-test"
@@ -170,7 +170,7 @@ func TestAutomaticCNISupport_Isolated(t *testing.T) {
 		webpages[taskID] = fmt.Sprintf("Hello, my task %s\n", taskID)
 	}
 
-	localServices, err := internal.NewLocalNetworkServices(webpages)
+	localServices, err := internal.NewLocalNetworkServices(t, webpages)
 	require.NoError(t, err, "failed to create local network test services")
 
 	cniNetworkName := "fcnet"
diff --git a/runtime/volume_integ_test.go b/runtime/volume_integ_test.go
index 60ceac0..5ba7307 100644
--- a/runtime/volume_integ_test.go
+++ b/runtime/volume_integ_test.go
@@ -20,7 +20,6 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
-	"strings"
 	"testing"
 
 	"github.com/containerd/containerd"
@@ -40,22 +39,11 @@ const mib = 1024 * 1024
 func TestVolumes_Isolated(t *testing.T) {
 	integtest.Prepare(t)
 
-	runtimes := []string{firecrackerRuntime, "io.containerd.runc.v2"}
-
-	for _, rt := range runtimes {
-		t.Run(rt, func(t *testing.T) {
-			testVolumes(t, rt)
-		})
-	}
-}
-
-func testVolumes(t *testing.T, runtime string) {
 	const vmID = 0
-	testName := strings.ReplaceAll(t.Name(), "/", "_")
 
 	ctx := namespaces.WithNamespace(context.Background(), "default")
 
-	client, err := containerd.New(containerdSockPath, containerd.WithDefaultRuntime(runtime))
+	client, err := containerd.New(containerdSockPath, containerd.WithDefaultRuntime(firecrackerRuntime))
 	require.NoError(t, err, "unable to create client to containerd service at %s, is containerd running?", containerdSockPath)
 	defer client.Close()
 
@@ -66,7 +54,7 @@ func testVolumes(t *testing.T, runtime string) {
 	require.NoError(t, err, "failed to create fccontrol client")
 
 	// Make volumes.
-	path, err := os.MkdirTemp("", testName)
+	path, err := os.MkdirTemp("", t.Name())
 	require.NoError(t, err)
 
 	f, err := os.Create(filepath.Join(path, "hello.txt"))
@@ -76,27 +64,22 @@ func testVolumes(t *testing.T, runtime string) {
 	require.NoError(t, err)
 
 	const volName = "volume1"
-	vs := volume.NewSet(runtime)
+	vs := volume.NewSet()
 	vs.Add(volume.FromHost(volName, path))
 
-	containers := []string{"c1", "c2"}
+	// Since CreateVM doesn't take functional options, we need to explicitly create
+	// a FirecrackerDriveMount
+	mount, err := vs.PrepareDriveMount(ctx, 10*mib)
+	require.NoError(t, err)
 
-	if runtime == firecrackerRuntime {
-		// Since CreateVM doesn't take functional options, we need to explicitly create
-		// a FirecrackerDriveMount
-		mount, err := vs.PrepareDriveMount(ctx, 10*mib)
-		require.NoError(t, err)
+	containers := []string{"c1", "c2"}
 
-		_, err = fcClient.CreateVM(ctx, &proto.CreateVMRequest{
-			VMID:           strconv.Itoa(vmID),
-			ContainerCount: int32(len(containers)),
-			DriveMounts:    []*proto.FirecrackerDriveMount{mount},
-		})
-		require.NoError(t, err, "failed to create VM")
-	} else {
-		err := vs.PrepareDirectory(ctx)
-		require.NoError(t, err)
-	}
+	_, err = fcClient.CreateVM(ctx, &proto.CreateVMRequest{
+		VMID:           strconv.Itoa(vmID),
+		ContainerCount: int32(len(containers)),
+		DriveMounts:    []*proto.FirecrackerDriveMount{mount},
+	})
+	require.NoError(t, err, "failed to create VM")
 
 	// Make containers with the volume.
 	dir := "/path/in/container"
@@ -120,8 +103,6 @@ func testVolumes(t *testing.T, runtime string) {
 		)
 		require.NoError(t, err, "failed to create container %s", name)
 
-		defer container.Delete(ctx, containerd.WithSnapshotCleanup)
-
 		var stdout, stderr bytes.Buffer
 
 		task, err := container.NewTask(ctx, cio.NewCreator(cio.WithStreams(nil, &stdout, &stderr)))
@@ -154,7 +135,6 @@ func testVolumes(t *testing.T, runtime string) {
 		),
 	)
 	require.NoError(t, err, "failed to create container %s", name)
-	defer container.Delete(ctx, containerd.WithSnapshotCleanup)
 
 	var stdout, stderr bytes.Buffer
 	task, err := container.NewTask(ctx, cio.NewCreator(cio.WithStreams(nil, &stdout, &stderr)))
diff --git a/tools/docker/Dockerfile.integ-test b/tools/docker/Dockerfile.integ-test
index 6e56f02..3e24e7b 100644
--- a/tools/docker/Dockerfile.integ-test
+++ b/tools/docker/Dockerfile.integ-test
@@ -28,10 +28,6 @@ RUN mkdir -p \
     ${FICD_LOG_DIR} \
     /etc/cni/net.d
 
-RUN wget https://github.com/containerd/containerd/releases/download/v1.6.4/containerd-1.6.4-linux-amd64.tar.gz && \
-  tar zxvf containerd-1.6.4-linux-amd64.tar.gz -C /tmp/ && \
-  install -D -o root -g root -m755 -t /usr/local/bin /tmp/bin/containerd-shim-runc-v2 && \
-  rm -rf containerd-1.6.4-linux-amd64.tar.gz /tmp/bin
 
 # Pull the images the tests need into the content store so we don't need internet
 # access during the tests themselves. This runs as a seperate step before the other