Repository: firecracker-microvm/firecracker-containerd
Pull request #379 (2020-01-16 21:26:58)

Copy sparse files as spase files
*Issue #, if available:*

NA

*Description of changes:*

Filesystem images are often big (multiple GBs) but sparse (have
zeros inside). At least mkfs.ext4 makes them as sparse files, but
io.Copy ignores the fact and copy all zeros as is, which is slow and
ineffecent.

This change adds sparse file support which makes runc_jailer's initial
file copy faster.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


aaithal: Is there additional test data that you can add here to make sure that this solution is as performant as `cp --sparse=always` ?
kzys: It is faster than GNU cp! only if the sparse file is pretty much empty :(

I've added my benchmark commit, and here is the result on my EC2 box.

```
% go test -bench=BenchmarkCopy -benchtime=5s
...                
goos: linux
goarch: amd64
pkg: github.com/firecracker-microvm/firecracker-containerd/runtime
BenchmarkCopyGoEmptyFile-48               200000             46494 ns/op
BenchmarkCopyGoExt4-48                        20         579197267 ns/op
BenchmarkCopyGoNaiveEmptyFile-48              10         658009553 ns/op
BenchmarkCopyGoNaiveExt4-48                   10         682055838 ns/op
BenchmarkCopyExecEmptyFile-48              10000            975919 ns/op
BenchmarkCopyExecExt4-48                     300          18730759 ns/op
PASS
ok      github.com/firecracker-microvm/firecracker-containerd/runtime   62.182s
```

Apparently [GNU cp is using FIEMAP](https://lwn.net/Articles/789623/), whereas this Go implementation is using SEEK_HOLE and SEEK_DATA.

> But he did note that the way sparse files were handled using FIEMAP in cp was great. Ts'o pointed out that FIEMAP is a great example of how the process should work.

Regarding the benefits, `cp --sparse=always` is GNU only option and not supported by other cp's such as BusyBox. 

But I do agree that we should use cp when there is. Let me update the PR.


xibz: @kzys - Are we ever going to be copying empty files though? I would much prefer `io.Copy` or the use of `cp` as they are much simpler implementations, and the difference between `io.Copy` and the sparse copy seems to be negligible in `ext4` files. Why are we optimizing this in the first place? Was there a bottle neck found?

Patch:
diff --git a/agent/Makefile b/agent/Makefile
index 7984a39..51a1b2b 100644
--- a/agent/Makefile
+++ b/agent/Makefile
@@ -17,15 +17,14 @@ GOMOD := $(shell go env GOMOD)
 GOSUM := $(GOMOD:.mod=.sum)
 SOURCES:=$(shell find . -name '*.go')
 DOCKER_IMAGE_TAG?=latest
-REVISION := $(shell git rev-parse HEAD)
 
 all: agent
 
 agent: *.go $(GOMOD) $(GOSUM)
 ifneq ($(STATIC_AGENT),)
-	CGO_ENABLED=0 go build -installsuffix cgo -a -ldflags "-s -X main.revision=$(REVISION)" $(EXTRAGOARGS) -o agent
+	CGO_ENABLED=0 go build -installsuffix cgo -a -ldflags "-s" $(EXTRAGOARGS) -o agent
 else
-	go build $(EXTRAGOARGS) -ldflags "-X main.revision=$(REVISION)" -o agent
+	go build $(EXTRAGOARGS) -o agent
 endif
 
 test:
diff --git a/agent/main.go b/agent/main.go
index 7cd61d1..1557c1e 100644
--- a/agent/main.go
+++ b/agent/main.go
@@ -16,7 +16,6 @@ package main
 import (
 	"context"
 	"flag"
-	"fmt"
 	"os"
 	"os/signal"
 	"syscall"
@@ -48,31 +47,20 @@ const (
 	enableSubreaper = 1
 )
 
-var (
-	revision string
-)
-
 func main() {
 	var (
-		port    int
-		debug   bool
-		version bool
+		port  int
+		debug bool
 	)
 
 	flag.IntVar(&port, "port", defaultPort, "Vsock port to listen to")
 	flag.BoolVar(&debug, "debug", false, "Turn on debug mode")
-	flag.BoolVar(&version, "version", false, "Show the version")
 	flag.Parse()
 
 	if debug {
 		logrus.SetLevel(logrus.DebugLevel)
 	}
 
-	if version {
-		showVersion()
-		return
-	}
-
 	signals := make(chan os.Signal, 32)
 	signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM, unix.SIGCHLD)
 
@@ -167,9 +155,3 @@ func main() {
 		panic(err)
 	}
 }
-
-func showVersion() {
-	// Once https://github.com/golang/go/issues/29814 is resolved,
-	// we can use runtime/debug.BuildInfo instead of calling git(1) from Makefile
-	fmt.Printf("containerd Firecracker agent (git commit: %s)\n", revision)
-}
diff --git a/firecracker-control/cmd/containerd/Makefile b/firecracker-control/cmd/containerd/Makefile
index 1b186dd..9b2d885 100644
--- a/firecracker-control/cmd/containerd/Makefile
+++ b/firecracker-control/cmd/containerd/Makefile
@@ -17,20 +17,16 @@ EXTRAGOARGS:=
 SRC := $(shell find . -name '*.go')
 GOMOD := $(shell go env GOMOD)
 GOSUM := $(GOMOD:.mod=.sum)
-REVISION=$(shell git rev-parse HEAD)
-VERSION_LDFLAGS="-X github.com/containerd/containerd/version.Revision=$(REVISION)"
 
 all: build
 
 build: firecracker-containerd firecracker-ctr
 
 firecracker-containerd: $(SRC) $(GOMOD) $(GOSUM)
-	go build $(EXTRAGOARGS) \
-		-ldflags $(VERSION_LDFLAGS) -o firecracker-containerd
+	go build $(EXTRAGOARGS) -o firecracker-containerd
 
 firecracker-ctr: $(GOMOD) $(GOSUM)
-	GOBIN=$(CURDIR) go install -tags=no_cri $(EXTRAGOARGS) \
-		-ldflags $(VERSION_LDFLAGS) github.com/containerd/containerd/cmd/ctr
+	GOBIN=$(CURDIR) go install -tags=no_cri $(EXTRAGOARGS) github.com/containerd/containerd/cmd/ctr
 	mv ctr firecracker-ctr
 
 install: firecracker-containerd firecracker-ctr
diff --git a/runtime/Makefile b/runtime/Makefile
index 15519fc..a92b227 100644
--- a/runtime/Makefile
+++ b/runtime/Makefile
@@ -22,8 +22,6 @@ DOCKER_IMAGE_TAG?=latest
 GO_CACHE_VOLUME_NAME?=gocache
 FIRECRACKER_CONTAINERD_TEST_IMAGE?=localhost/firecracker-containerd-test
 
-REVISION=$(shell git rev-parse HEAD)
-
 INTEG_TEST_SUFFIX := _Isolated
 INTEG_TESTNAMES=$(shell docker run --rm \
 	--network=none \
@@ -39,7 +37,7 @@ all: runtime
 runtime: containerd-shim-aws-firecracker
 
 containerd-shim-aws-firecracker: $(SOURCES) $(GOMOD) $(GOSUM)
-	go build -o containerd-shim-aws-firecracker $(EXTRAGOARGS) -ldflags "-X main.revision=$(REVISION)"
+	go build -o containerd-shim-aws-firecracker $(EXTRAGOARGS)
 
 install: containerd-shim-aws-firecracker
 	install -D -o root -g root -m755 -t $(INSTALLROOT)/bin containerd-shim-aws-firecracker
diff --git a/runtime/main.go b/runtime/main.go
index 015c99d..305e449 100644
--- a/runtime/main.go
+++ b/runtime/main.go
@@ -23,8 +23,6 @@ import (
 
 const shimID = "aws.firecracker"
 
-var revision string
-
 func init() {
 	logrus.SetFormatter(&logrus.TextFormatter{
 		TimestampFormat: log.RFC3339NanoFixed,
diff --git a/runtime/runc_jailer.go b/runtime/runc_jailer.go
index 296d000..822fd47 100644
--- a/runtime/runc_jailer.go
+++ b/runtime/runc_jailer.go
@@ -17,7 +17,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -348,23 +347,12 @@ func exposeBlockDeviceToJail(dst string, rdev, uid, gid int) error {
 }
 
 func copyFile(src, dst string, mode os.FileMode) error {
-	srcFile, err := os.Open(src)
+	// --sparse=always is a GNU-only option
+	output, err := exec.Command("cp", "--sparse=always", src, dst).CombinedOutput()
 	if err != nil {
-		return errors.Wrapf(err, "failed to open %v", src)
+		return errors.Wrapf(err, "failed to copy %q to %q: %s", src, dst, output)
 	}
-	defer srcFile.Close()
-
-	dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_EXCL, mode)
-	if err != nil {
-		return errors.Wrapf(err, "failed to open %v", dstFile)
-	}
-	defer dstFile.Close()
-
-	_, err = io.Copy(dstFile, srcFile)
-	if err != nil {
-		return errors.Wrap(err, "failed to copy to destination")
-	}
-	return nil
+	return os.Chmod(dst, mode)
 }
 
 func (j *runcJailer) jailerCommand(containerName string, isDebug bool) *exec.Cmd {
diff --git a/runtime/service.go b/runtime/service.go
index 1ab2990..59b402b 100644
--- a/runtime/service.go
+++ b/runtime/service.go
@@ -33,6 +33,7 @@ import (
 	"math/rand" // #nosec
 
 	"github.com/containerd/containerd/errdefs"
+	"github.com/containerd/containerd/events"
 	"github.com/containerd/containerd/events/exchange"
 	"github.com/containerd/containerd/log"
 	"github.com/containerd/containerd/namespaces"
@@ -217,36 +218,25 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish
 	return s, nil
 }
 
-func (s *service) startEventForwarders(remotePublisher shim.Publisher) {
-	ns, ok := namespaces.Namespace(s.shimCtx)
-	if !ok {
-		s.logger.Error("failed to fetch the namespace from the context")
-	}
-	ctx := namespaces.WithNamespace(context.Background(), ns)
-
+func (s *service) startEventForwarders(remotePublisher events.Publisher) {
 	// Republish each event received on our exchange to the provided remote publisher.
 	// TODO ideally we would be forwarding events instead of re-publishing them, which would
 	// preserve the events' original timestamps and namespaces. However, as of this writing,
 	// the containerd v2 runtime model only provides a shim with a publisher, not a forwarder.
-	republishCh := eventbridge.Republish(ctx, s.eventExchange, remotePublisher)
-
 	go func() {
-		<-s.vmReady
-
-		// Once the VM is ready, also start forwarding events from it to our exchange
-		attachCh := eventbridge.Attach(ctx, s.eventBridgeClient, s.eventExchange)
-
-		err := <-attachCh
+		err := <-eventbridge.Republish(s.shimCtx, s.eventExchange, remotePublisher)
 		if err != nil && err != context.Canceled {
-			s.logger.WithError(err).Error("error while forwarding events from VM agent")
+			s.logger.WithError(err).Error("error while republishing events")
 		}
+	}()
 
-		err = <-republishCh
+	// Once the VM is ready, also start forwarding events from it to our exchange
+	go func() {
+		<-s.vmReady
+		err := <-eventbridge.Attach(s.shimCtx, s.eventBridgeClient, s.eventExchange)
 		if err != nil && err != context.Canceled {
-			s.logger.WithError(err).Error("error while republishing events")
+			s.logger.WithError(err).Error("error while forwarding events from VM agent")
 		}
-
-		remotePublisher.Close()
 	}()
 }
 
@@ -342,6 +332,10 @@ func (s *service) StartShim(shimCtx context.Context, containerID, containerdBina
 		// task is deleted
 		containerCount = 1
 		exitAfterAllTasksDeleted = true
+
+		log.Info("will start a single-task VM since no VMID has been provided")
+	} else {
+		log.WithField("vmID", s.vmID).Info("will start a persistent VM")
 	}
 
 	client, err := ttrpcutil.NewClient(containerdTTRPCAddress)
@@ -364,16 +358,6 @@ func (s *service) StartShim(shimCtx context.Context, containerID, containerdBina
 		}
 	}
 
-	// The shim cannot support traditional -version/-v flag because
-	// - shim.Run() will call flag.Parse(). So our main cannot call flag.Parse() before that.
-	// - -address is required and NewService() won't be called if the flag is missing.
-	// So we log the version informaion here instead
-	str := ""
-	if exitAfterAllTasksDeleted {
-		str = " The VM will be torn down after serving a single task."
-	}
-	log.WithField("vmID", s.vmID).Infof("successfully started shim (git commit: %s).%s", revision, str)
-
 	return fcShim.SocketAddress(shimCtx, s.vmID)
 }
 


Test patch:
diff --git a/runtime/jailer_test.go b/runtime/jailer_test.go
index d13fba3..95504a9 100644
--- a/runtime/jailer_test.go
+++ b/runtime/jailer_test.go
@@ -15,11 +15,15 @@ package main
 
 import (
 	"context"
+	"io/ioutil"
 	"os"
+	"path/filepath"
+	"syscall"
 	"testing"
 
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/firecracker-microvm/firecracker-containerd/proto"
 )
@@ -36,6 +40,45 @@ func TestCopyFile_simple(t *testing.T) {
 	info, err := os.Stat(dstPath)
 	assert.NoError(t, err, "failed to stat file")
 	assert.Equal(t, os.FileMode(expectedMode), info.Mode())
+	assert.NotEqual(t, 0, int(info.Size()))
+}
+
+func createSparseFile(path string, size int) error {
+	f, err := os.Create(path)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+
+	err = f.Truncate(int64(size))
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+func TestCopyFile_sparse(t *testing.T) {
+	dir, err := ioutil.TempDir("", t.Name())
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+
+	expectedSize := 1024
+
+	src := filepath.Join(dir, "original-sparse-file")
+	err = createSparseFile(src, expectedSize)
+	require.NoError(t, err)
+
+	dst := filepath.Join(dir, "copied-as-sparse")
+	err = copyFile(src, dst, 0600)
+	require.NoError(t, err, "failed to copy file")
+
+	stat, err := os.Stat(dst)
+	require.NoError(t, err)
+	assert.Equal(t, expectedSize, int(stat.Size()), "metadata-wise, the file is not empty")
+
+	unixStat, ok := (stat.Sys()).(*syscall.Stat_t)
+	require.True(t, ok)
+	assert.Equal(t, int64(0), unixStat.Blocks, "it doesn't allocate any blocks, since the file is empty")
 }
 
 func TestCopyFile_invalidPaths(t *testing.T) {
diff --git a/runtime/service_integ_test.go b/runtime/service_integ_test.go
index 111c94f..d5014d6 100644
--- a/runtime/service_integ_test.go
+++ b/runtime/service_integ_test.go
@@ -1323,7 +1323,6 @@ func TestRandomness_Isolated(t *testing.T) {
 func TestStopVM_Isolated(t *testing.T) {
 	prepareIntegTest(t)
 	require := require.New(t)
-	assert := assert.New(t)
 
 	client, err := containerd.New(containerdSockPath, containerd.WithDefaultRuntime(firecrackerRuntime))
 	require.NoError(err, "unable to create client to containerd service at %s, is containerd running?", containerdSockPath)
@@ -1376,9 +1375,7 @@ func TestStopVM_Isolated(t *testing.T) {
 			},
 			stopFunc: func(ctx context.Context, fcClient fccontrol.FirecrackerService, vmID string) {
 				_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: vmID})
-				errCode := status.Code(err)
-				assert.NotEqual(codes.Unknown, errCode, "the error code must not be Unknown")
-				assert.Equal(codes.DeadlineExceeded, errCode, "the error code must be DeadlineExceeded")
+				require.Equal(status.Code(err), codes.DeadlineExceeded)
 			},
 		},
 
@@ -1466,73 +1463,6 @@ func TestStopVM_Isolated(t *testing.T) {
 		})
 	}
 }
-func TestEvents_Isolated(t *testing.T) {
-	prepareIntegTest(t)
-	require := require.New(t)
-
-	client, err := containerd.New(containerdSockPath, containerd.WithDefaultRuntime(firecrackerRuntime))
-	require.NoError(err, "unable to create client to containerd service at %s, is containerd running?", containerdSockPath)
-	defer client.Close()
-
-	ctx := namespaces.WithNamespace(context.Background(), "default")
-
-	// If we don't have enough events within 30 seconds, the context will be cancelled and the loop below will be interrupted
-	subscribeCtx, subscribeCancel := context.WithTimeout(ctx, 30*time.Second)
-	defer subscribeCancel()
-	eventCh, errCh := client.Subscribe(subscribeCtx, "topic")
-
-	image, err := alpineImage(ctx, client, defaultSnapshotterName())
-	require.NoError(err, "failed to get alpine image")
-
-	pluginClient, err := ttrpcutil.NewClient(containerdSockPath + ".ttrpc")
-	require.NoError(err, "failed to create ttrpc client")
-
-	vmID := testNameToVMID(t.Name())
-
-	fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
-	_, err = fcClient.CreateVM(ctx, &proto.CreateVMRequest{VMID: vmID})
-	require.NoError(err)
-
-	c, err := client.NewContainer(ctx,
-		"container-"+vmID,
-		containerd.WithSnapshotter(defaultSnapshotterName()),
-		containerd.WithNewSnapshot("snapshot-"+vmID, image),
-		containerd.WithNewSpec(oci.WithProcessArgs("/bin/echo", "-n", "hello"), firecrackeroci.WithVMID(vmID)),
-	)
-	require.NoError(err)
-
-	stdout := startAndWaitTask(ctx, t, c)
-	require.Equal("hello", stdout)
-
-	_, err = fcClient.StopVM(ctx, &proto.StopVMRequest{VMID: vmID})
-	require.Equal(status.Code(err), codes.OK)
-
-	expected := []string{
-		"/snapshot/prepare",
-		"/snapshot/commit",
-		"/firecracker-vm/start",
-		"/snapshot/prepare",
-		"/containers/create",
-		"/tasks/create",
-		"/tasks/start",
-		"/tasks/exit",
-		"/tasks/delete",
-		"/firecracker-vm/stop",
-	}
-	var actual []string
-
-loop:
-	for len(actual) < len(expected) {
-		select {
-		case event := <-eventCh:
-			actual = append(actual, event.Topic)
-		case err := <-errCh:
-			assert.NoError(t, err)
-			break loop
-		}
-	}
-	require.Equal(expected, actual)
-}
 
 func findProcWithName(name string) func(context.Context, *process.Process) (bool, error) {
 	return func(ctx context.Context, p *process.Process) (bool, error) {