Repository: firecracker-microvm/firecracker-containerd
Pull request #656 (2022-05-31 17:21:20)

add: no namespace walk support to demux snapshotter
Signed-off-by: Austin Vazquez <macedonv@amazon.com>

*Issue #, if available:*
#652 

*Description of changes:*
This PR adds the ability for no namespaced walks on the demux snapshotter to be broadcasted to all cached snapshotters.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/Makefile b/Makefile
index 334dfb1..214bcde 100644
--- a/Makefile
+++ b/Makefile
@@ -411,17 +411,3 @@ $(STARGZ_BIN): $(STARGZ_DIR)/go.mod tools/stargz-builder-stamp
 	--workdir /src/$(STARGZ_DIR) \
 	localhost/$(STARGZ_BUILDER_NAME):$(DOCKER_IMAGE_TAG) \
 	make
-
-##########################
-# eStargz formatted image 
-##########################
-CTR_REMOTE_BIN=$(STARGZ_DIR)/out/ctr-remote
-DEFAULT_BASE_IMAGE=public.ecr.aws/amazonlinux/amazonlinux:latest
-DEFAULT_ESGZ_IMAGE=ghcr.io/firecracker-microvm/firecracker-containerd/amazonlinux:latest-esgz
-.PHONY: esgz-test-image push-esgz-test-image
-esgz-test-image: stargz-snapshotter
-	$(CTR_REMOTE_BIN) image pull $(DEFAULT_BASE_IMAGE)
-	$(CTR_REMOTE_BIN) image optimize --oci $(DEFAULT_BASE_IMAGE) $(DEFAULT_ESGZ_IMAGE)
-
-push-esgz-test-image:
-	$(CTR_REMOTE_BIN) image push -u $(GH_USER):$(GH_PERSONAL_ACCESS_TOKEN) $(DEFAULT_ESGZ_IMAGE) $(DEFAULT_ESGZ_IMAGE)
\ No newline at end of file
diff --git a/snapshotter/demux/README.md b/snapshotter/demux/README.md
deleted file mode 100644
index dcab81a..0000000
--- a/snapshotter/demux/README.md
+++ /dev/null
@@ -1,49 +0,0 @@
-# snapshotter
-
-## Building an esgz Formatted Image
-
-The demux snapshotter uses a publicly available [eStargz](https://github.com/containerd/stargz-snapshotter/blob/main/docs/estargz.md) formatted image in its integration testing pipeline. 
-This image has the reference `ghcr.io/firecracker-microvm/firecracker-containerd/amazonlinux:latest-esgz`. 
-
-### Optimize an Amazon Linux Container Image for esgz
-
-The `ctr-remote` binary is needed to optimize an image for the esgz image format. Building the stargz-snapshotter submodule provides `ctr-remote`, a `ctr` wrapper, in `_submodules/stargz-snapshotter/out/ctr-remote`.
-Refer to the [containerd stargz-snapshotter documentation on `ctr-remote`](https://github.com/containerd/stargz-snapshotter/blob/main/docs/ctr-remote.md) for additional details.
-
-The root level Makefile provides `make esgz-test-image` for pulling the latest Amazon Linux image locally and optimizing it for eStargz using `ctr-remote` into an image `ghcr.io/firecracker-microvm/firecracker-containerd/amazonlinux:latest-esgz`. 
-Code owners of `firecracker-containerd` then push the eStargz optimized image as this publicly available reference using `make push-esgz-test-image` with proper credential environment variables `GH_USER` and `GH_PERSONAL_ACCESS_TOKEN` set.
-
-### Example optimization and push to GHCR
-
-Using default base and esgz images:
-
-```bash
-$ make esgz-test-image
-$ GH_USER=xxx
-# set GH_PERSONAL_ACCESS_TOKEN with command substitution such that it does not show in shell history
-$ make \
-    GH_USER=$GH_USER \
-    GH_PERSONAL_ACCESS_TOKEN=`cat`
-    push-esgz-test-image
-# enter personal access token and CTRL^D
-```
-
-To pull, convert, and push to a repo where the user has permissions with custom base and esgz images:
-
-```bash
-$ DEFAULT_BASE_IMAGE=public.ecr.aws/amazonlinux/amazonlinux:2022
-$ DEFAULT_ESGZ_IMAGE=ghcr.io/$DESTINATION_REPO/amazonlinux:2022-esgz
-$ make \
-    DEFAULT_BASE_IMAGE=$DEFAULT_BASE_IMAGE \
-    DEFAULT_ESGZ_IMAGE=$DEFAULT_ESGZ_IMAGE \
-    esgz-test-image
-$ GH_USER=xxx
-# set GH_PERSONAL_ACCESS_TOKEN with command substitution such that it does not show in shell history
-$ make \
-    GH_USER=$GH_USER \
-    GH_PERSONAL_ACCESS_TOKEN=`cat`
-    DEFAULT_ESGZ_IMAGE=$DEFAULT_ESGZ_IMAGE \
-    push-esgz-test-image
-# enter personal access token and CTRL^D
-```
-
diff --git a/snapshotter/demux/cache/cache.go b/snapshotter/demux/cache/cache.go
index 1250144..9d1c3cb 100644
--- a/snapshotter/demux/cache/cache.go
+++ b/snapshotter/demux/cache/cache.go
@@ -16,6 +16,7 @@ package cache
 import (
 	"context"
 
+	"github.com/containerd/containerd/snapshots"
 	"github.com/firecracker-microvm/firecracker-containerd/snapshotter/demux/proxy"
 )
 
@@ -28,6 +29,9 @@ type Cache interface {
 	// fetch function if the snapshotter is not currently cached.
 	Get(ctx context.Context, key string, fetch SnapshotterProvider) (*proxy.RemoteSnapshotter, error)
 
+	// WalkAll applies the provided function across all cached snapshotters.
+	WalkAll(ctx context.Context, fn snapshots.WalkFunc, filters ...string) error
+
 	// Closes the snapshotter and removes it from the cache.
 	Evict(key string) error
 
diff --git a/snapshotter/demux/cache/snapshotter_cache.go b/snapshotter/demux/cache/snapshotter_cache.go
index 761cbf8..7915a92 100644
--- a/snapshotter/demux/cache/snapshotter_cache.go
+++ b/snapshotter/demux/cache/snapshotter_cache.go
@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"sync"
 
+	"github.com/containerd/containerd/snapshots"
 	"github.com/firecracker-microvm/firecracker-containerd/snapshotter/demux/proxy"
 	"github.com/hashicorp/go-multierror"
 )
@@ -56,6 +57,21 @@ func (cache *SnapshotterCache) Get(ctx context.Context, key string, fetch Snapsh
 	return snapshotter, nil
 }
 
+// WalkAll applies the provided function to all cached snapshotters.
+func (cache *SnapshotterCache) WalkAll(ctx context.Context, fn snapshots.WalkFunc, filters ...string) error {
+	cache.mutex.RLock()
+	defer cache.mutex.RUnlock()
+
+	var allErr error
+
+	for namespace, snapshotter := range cache.snapshotters {
+		if err := snapshotter.Walk(ctx, fn, filters...); err != nil {
+			allErr = multierror.Append(allErr, fmt.Errorf("failed to walk function on snapshotter[%s]: %w", namespace, err))
+		}
+	}
+	return allErr
+}
+
 // Evict removes a cached snapshotter for a given key.
 func (cache *SnapshotterCache) Evict(key string) error {
 	cache.mutex.RLock()
diff --git a/snapshotter/demux/snapshotter.go b/snapshotter/demux/snapshotter.go
index 345f6fc..b1e55d6 100644
--- a/snapshotter/demux/snapshotter.go
+++ b/snapshotter/demux/snapshotter.go
@@ -243,7 +243,8 @@ func (s *Snapshotter) Walk(ctx context.Context, fn snapshots.WalkFunc, filters .
 	contextLogger := log.G(ctx).WithField("function", "Walk")
 	namespace, err := getNamespaceFromContext(ctx, contextLogger)
 	if err != nil {
-		return err
+		contextLogger.Debug("no namespace found, proxying walk function to all cached snapshotters")
+		return s.cache.WalkAll(ctx, fn, filters...)
 	}
 	logger := contextLogger.WithField("namespace", namespace)
 


Test patch:
diff --git a/snapshotter/demux/cache/snapshotter_cache_test.go b/snapshotter/demux/cache/snapshotter_cache_test.go
index 46405eb..bb84f45 100644
--- a/snapshotter/demux/cache/snapshotter_cache_test.go
+++ b/snapshotter/demux/cache/snapshotter_cache_test.go
@@ -17,6 +17,7 @@ import (
 	"context"
 	"testing"
 
+	"github.com/containerd/containerd/snapshots"
 	"github.com/hashicorp/go-multierror"
 	"github.com/pkg/errors"
 
@@ -55,13 +56,53 @@ func getCachedSnapshotter(uut *SnapshotterCache) error {
 	return nil
 }
 
-func getSnapshotterPropogatesErrors(uut *SnapshotterCache) error {
+func getSnapshotterPropagatesErrors(uut *SnapshotterCache) error {
 	if _, err := uut.Get(context.Background(), "SnapshotterKey", getSnapshotterErrorFunction); err == nil {
 		return errors.New("Get function did not propagate errors from snapshotter generator function")
 	}
 	return nil
 }
 
+func successfulWalk(ctx context.Context, info snapshots.Info) error {
+	return nil
+}
+
+func applyWalkFunctionOnEmptyCache(uut *SnapshotterCache) error {
+	if err := uut.WalkAll(context.Background(), successfulWalk); err != nil {
+		return errors.New("WalkAll on empty cache incorrectly resulted in error")
+	}
+	return nil
+}
+
+func applyWalkFunctionToAllCachedSnapshotters(uut *SnapshotterCache) error {
+	if _, err := uut.Get(context.Background(), "Snapshotter-A", getSnapshotterOkFunction); err != nil {
+		return errors.Wrap(err, "Adding snapshotter A to empty cache incorrectly resulted in error")
+	}
+	if _, err := uut.Get(context.Background(), "Snapshotter-B", getSnapshotterOkFunction); err != nil {
+		return errors.Wrap(err, "Adding snapshotter B to empty cache incorrectly resulted in error")
+	}
+	if err := uut.WalkAll(context.Background(), successfulWalk); err != nil {
+		return errors.New("WalkAll on populated cache incorrectly resulted in error")
+	}
+	return nil
+}
+
+func applyWalkFunctionPropagatesErrors(uut *SnapshotterCache) error {
+	if _, err := uut.Get(context.Background(), "Snapshotter-A", getFailingSnapshotterOkFunction); err != nil {
+		return errors.Wrap(err, "Adding snapshotter A to empty cache incorrectly resulted in error")
+	}
+	// The failing snapshotter mock will fail all Walk calls before applying
+	// the snapshots.WalkFunc, but for the purposes of this test that is fine.
+	// In which case, any function will do.
+	walkFunc := func(ctx context.Context, info snapshots.Info) error {
+		return nil
+	}
+	if err := uut.WalkAll(context.Background(), walkFunc); err == nil {
+		return errors.New("WalkAll did not propagate errors from walk function")
+	}
+	return nil
+}
+
 func evictSnapshotterFromEmptyCache(uut *SnapshotterCache) error {
 	if err := uut.Evict("SnapshotterKey"); err == nil {
 		return errors.New("Evict function did not return error on call on empty cache")
@@ -80,7 +121,7 @@ func evictSnapshotterFromCache(uut *SnapshotterCache) error {
 	return nil
 }
 
-func evictSnapshotterFromCachePropogatesCloseError(uut *SnapshotterCache) error {
+func evictSnapshotterFromCachePropagatesCloseError(uut *SnapshotterCache) error {
 	if _, err := uut.Get(context.Background(), "SnapshotterKey", getFailingSnapshotterOkFunction); err != nil {
 		return errors.Wrap(err, "Adding snapshotter to empty cache incorrectly resulted in error")
 	}
@@ -156,14 +197,36 @@ func TestGetSnapshotterFromCache(t *testing.T) {
 	}{
 		{"AddSnapshotterToCache", getSnapshotterFromEmptyCache},
 		{"GetCachedSnapshotter", getCachedSnapshotter},
-		{"PropogateFetchSnapshotterErrors", getSnapshotterPropogatesErrors},
+		{"PropogateFetchSnapshotterErrors", getSnapshotterPropagatesErrors},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			uut := NewSnapshotterCache()
+			if err := test.run(uut); err != nil {
+				t.Fatalf("%s: %s", test.name, err.Error())
+			}
+		})
+	}
+}
+
+func TestWalkAllFunctionOnCache(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		run  func(*SnapshotterCache) error
+	}{
+		{"ApplyWalkFunctionOnEmptyCache", applyWalkFunctionOnEmptyCache},
+		{"ApplyWalkFunctionToAllCachedSnapshotters", applyWalkFunctionToAllCachedSnapshotters},
+		{"ApplyWalkFunctionPropogatesErrors", applyWalkFunctionPropagatesErrors},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			uut := NewSnapshotterCache()
 			if err := test.run(uut); err != nil {
-				t.Fatal(test.name + ": " + err.Error())
+				t.Fatalf("%s: %s", test.name, err.Error())
 			}
 		})
 	}
@@ -178,14 +241,14 @@ func TestEvictSnapshotterFromCache(t *testing.T) {
 	}{
 		{"EvictSnapshotterFromEmptyCache", evictSnapshotterFromEmptyCache},
 		{"EvictSnapshotterFromCache", evictSnapshotterFromCache},
-		{"PropogateEvictSnapshotterCloseErrors", evictSnapshotterFromCachePropogatesCloseError},
+		{"PropogateEvictSnapshotterCloseErrors", evictSnapshotterFromCachePropagatesCloseError},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			uut := NewSnapshotterCache()
 			if err := test.run(uut); err != nil {
-				t.Fatal(test.name + ": " + err.Error())
+				t.Fatalf("%s: %s", test.name, err.Error())
 			}
 		})
 	}
@@ -207,7 +270,7 @@ func TestCloseCache(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			uut := NewSnapshotterCache()
 			if err := test.run(uut); err != nil {
-				t.Fatal(test.name + ": " + err.Error())
+				t.Fatalf("%s: %s", test.name, err.Error())
 			}
 		})
 	}
@@ -228,7 +291,7 @@ func TestListSnapshotters(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			uut := NewSnapshotterCache()
 			if err := test.run(uut); err != nil {
-				t.Fatal(test.name + ": " + err.Error())
+				t.Fatalf("%s: %s", test.name, err.Error())
 			}
 		})
 	}
diff --git a/snapshotter/demux/snapshotter_test.go b/snapshotter/demux/snapshotter_test.go
index 70d20bd..2202e1a 100644
--- a/snapshotter/demux/snapshotter_test.go
+++ b/snapshotter/demux/snapshotter_test.go
@@ -73,6 +73,27 @@ func TestReturnErrorWhenCalledWithoutNamespacedContext(t *testing.T) {
 		{"View", func() error { _, err := uut.View(ctx, "layerKey", ""); return err }},
 		{"Commit", func() error { return uut.Commit(ctx, "layer1", "layerKey") }},
 		{"Remove", func() error { return uut.Remove(ctx, "layerKey") }},
+	}
+
+	for _, test := range tests {
+		if err := test.run(); err == nil {
+			t.Fatalf("%s call did not return error", test.name)
+		}
+	}
+}
+
+func TestNoErrorWhenCalledWithoutNamespacedContext(t *testing.T) {
+	t.Parallel()
+
+	cache := cache.NewSnapshotterCache()
+	ctx := logtest.WithT(context.Background(), t)
+
+	uut := NewSnapshotter(cache, fetchOkSnapshotter)
+
+	tests := []struct {
+		name string
+		run  func() error
+	}{
 		{"Walk", func() error {
 			var callback = func(c context.Context, i snapshots.Info) error { return nil }
 			return uut.Walk(ctx, callback)
@@ -80,8 +101,8 @@ func TestReturnErrorWhenCalledWithoutNamespacedContext(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		if err := test.run(); err == nil {
-			t.Fatal(test.name + " call did not return error")
+		if err := test.run(); err != nil {
+			t.Fatalf("%s call returned error on no namespace execution", test.name)
 		}
 	}
 }
@@ -116,7 +137,7 @@ func TestReturnErrorWhenSnapshotterNotFound(t *testing.T) {
 
 	for _, test := range tests {
 		if err := test.run(); err == nil {
-			t.Fatal(test.name + " call did not return error")
+			t.Fatalf("%s call did not return error", test.name)
 		}
 	}
 }
@@ -153,7 +174,7 @@ func TestReturnErrorAfterProxyFunctionFailure(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name+"ProxyFailure", func(t *testing.T) {
 			if err := test.run(); err == nil {
-				t.Fatal(test.name + " call did not return error")
+				t.Fatalf("%s call did not return error", test.name)
 			}
 		})
 	}
@@ -191,7 +212,7 @@ func TestNoErrorIsReturnedOnSuccessfulProxyExecution(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name+"SuccessfulProxyCall", func(t *testing.T) {
 			if err := test.run(); err != nil {
-				t.Fatal(test.name + " call incorrectly returned an error")
+				t.Fatalf("%s call incorrectly returned an error", test.name)
 			}
 		})
 	}