Refactor container image management

.github/workflows/unit_tests.yml

@@ -66,6 +66,10 @@ jobs:
   firecracker-containerd-interface-test:
     name: "Unit tests: Firecracker-containerd interface"
     runs-on: [self-hosted, integ]
+    strategy:
+      fail-fast: false
+      matrix:
+        module: [ ctriface, ctriface/image ]
     steps:
 
     - name: Set up Go 1.19
@@ -94,9 +98,13 @@ jobs:
       run: go build -race -v -a ./...
 
     - name: Run tests in submodules
+      env:
+        MODULE: ${{ matrix.module }}
+        AWS_ACCESS_KEY: ${{ secrets.AWS_ACCESS_KEY }}
+        AWS_SECRET_KEY: ${{ secrets.AWS_SECRET_KEY }}
       run: |
-        make -C ctriface test
-        make -C ctriface test-man
+        make -C $MODULE test
+        make -C $MODULE test-man
 
     - name: Cleaning
       if: ${{ always() }}

ctriface/iface.go

@@ -24,10 +24,6 @@ package ctriface
 
 import (
 	"context"
-	"fmt"
-	"net"
-	"net/http"
-	"net/url"
 	"os"
 	"os/exec"
 	"strings"
@@ -41,7 +37,6 @@ import (
 	"github.com/containerd/containerd/cio"
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/oci"
-	"github.com/containerd/containerd/remotes/docker"
 
 	"github.com/firecracker-microvm/firecracker-containerd/proto" // note: from the original repo
 	"github.com/firecracker-microvm/firecracker-containerd/runtime/firecrackeroci"
@@ -285,74 +280,8 @@ func (o *Orchestrator) StopSingleVM(ctx context.Context, vmID string) error {
 	return nil
 }
 
-// Checks whether a URL has a .local domain
-func isLocalDomain(s string) (bool, error) {
-	if !strings.Contains(s, "://") {
-		s = "dummy://" + s
-	}
-
-	u, err := url.Parse(s)
-	if err != nil {
-		return false, err
-	}
-
-	host, _, err := net.SplitHostPort(u.Host)
-	if err != nil {
-		host = u.Host
-	}
-
-	i := strings.LastIndex(host, ".")
-	tld := host[i+1:]
-
-	return tld == "local", nil
-}
-
-// Converts an image name to a url if it is not a URL
-func getImageURL(image string) string {
-	// Pull from dockerhub by default if not specified (default k8s behavior)
-	if strings.Contains(image, ".") {
-		return image
-	}
-	return "docker.io/" + image
-
-}
-
 func (o *Orchestrator) getImage(ctx context.Context, imageName string) (*containerd.Image, error) {
-	image, found := o.cachedImages[imageName]
-	if !found {
-		var err error
-		log.Debug(fmt.Sprintf("Pulling image %s", imageName))
-
-		imageURL := getImageURL(imageName)
-		local, _ := isLocalDomain(imageURL)
-		if local {
-			// Pull local image using HTTP
-			resolver := docker.NewResolver(docker.ResolverOptions{
-				Client: http.DefaultClient,
-				Hosts: docker.ConfigureDefaultRegistries(
-					docker.WithPlainHTTP(docker.MatchAllHosts),
-				),
-			})
-			image, err = o.client.Pull(ctx, imageURL,
-				containerd.WithPullUnpack,
-				containerd.WithPullSnapshotter(o.snapshotter),
-				containerd.WithResolver(resolver),
-			)
-		} else {
-			// Pull remote image
-			image, err = o.client.Pull(ctx, imageURL,
-				containerd.WithPullUnpack,
-				containerd.WithPullSnapshotter(o.snapshotter),
-			)
-		}
-
-		if err != nil {
-			return &image, err
-		}
-		o.cachedImages[imageName] = image
-	}
-
-	return &image, nil
+	return o.imageManager.GetImage(ctx, imageName)
 }
 
 func getK8sDNS() []string {

ctriface/image/Makefile

@@ -0,0 +1,35 @@
+# MIT License
+#
+# Copyright (c) 2023 Georgiy Lebedev, Dmitrii Ustiugov, Plamen Petrov and vHive team
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in all
+# copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+
+EXTRAGOARGS:=-v -race -cover
+CTRDLOGDIR:=/tmp/ctrd-logs
+
+test:
+	./../../scripts/clean_fcctr.sh
+	sudo mkdir -m777 -p $(CTRDLOGDIR) && sudo env "PATH=$(PATH)" /usr/local/bin/firecracker-containerd --config /etc/firecracker-containerd/config.toml 1>$(CTRDLOGDIR)/ctriface_log.out 2>$(CTRDLOGDIR)/ctriface_log.err &
+	sudo env "PATH=$(PATH)" go test ./ $(EXTRAGOARGS)
+	./../../scripts/clean_fcctr.sh
+
+test-man:
+	echo "Nothing to test manually"
+
+.PHONY: test test-man

ctriface/image/manager.go

@@ -0,0 +1,167 @@
+// MIT License
+//
+// Copyright (c) 2023 Georgiy Lebedev, Amory Hoste and vHive team
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Package ctrimages provides an image manager that manages and caches container images.
+package image
+
+import (
+	"context"
+	"github.com/containerd/containerd"
+	"github.com/containerd/containerd/remotes/docker"
+	log "github.com/sirupsen/logrus"
+	"net"
+	"net/http"
+	"net/url"
+	"strings"
+	"sync"
+)
+
+// ImageState is used for synchronization to avoid pulling the same image multiple times concurrently.
+type ImageState struct {
+	sync.Mutex
+	isCached bool
+}
+
+// NewImageState creates a new ImageState object that can be used to synchronize pulling a single image
+func NewImageState() *ImageState {
+	state := new(ImageState)
+	state.isCached = false
+	return state
+}
+
+// ImageManager manages the images that have been pulled to the node.
+type ImageManager struct {
+	sync.Mutex
+	snapshotter  string // image snapshotter
+	cachedImages map[string]containerd.Image // Cached container images
+	imageStates  map[string]*ImageState
+	client       *containerd.Client
+}
+
+// NewImageManager creates a new image manager that can be used to fetch container images.
+func NewImageManager(client *containerd.Client, snapshotter string) *ImageManager {
+	log.Info("Creating image manager")
+	manager := new(ImageManager)
+	manager.snapshotter = snapshotter
+	manager.cachedImages = make(map[string]containerd.Image)
+	manager.imageStates = make(map[string]*ImageState)
+	manager.client = client
+	return manager
+}
+
+// pullImage fetches an image and adds it to the cached image list
+func (mgr *ImageManager) pullImage(ctx context.Context, imageName string) error {
+	var err error
+	var image containerd.Image
+
+	imageURL := getImageURL(imageName)
+	local, _ := isLocalDomain(imageURL)
+	if local {
+		// Pull local image using HTTP
+		resolver := docker.NewResolver(docker.ResolverOptions{
+			Client: http.DefaultClient,
+			Hosts: docker.ConfigureDefaultRegistries(
+				docker.WithPlainHTTP(docker.MatchAllHosts),
+			),
+		})
+		image, err = mgr.client.Pull(ctx, imageURL,
+			containerd.WithPullUnpack,
+			containerd.WithPullSnapshotter(mgr.snapshotter),
+			containerd.WithResolver(resolver),
+		)
+	} else {
+		// Pull remote image
+		image, err = mgr.client.Pull(ctx, imageURL,
+			containerd.WithPullUnpack,
+			containerd.WithPullSnapshotter(mgr.snapshotter),
+		)
+	}
+	if err != nil {
+		return err
+	}
+	mgr.Lock()
+	mgr.cachedImages[imageName] = image
+	mgr.Unlock()
+	return nil
+}
+
+// GetImage fetches an image that can be used to create a container using containerd. Synchronization is implemented
+// on a per image level to keep waiting to a minimum.
+func (mgr *ImageManager) GetImage(ctx context.Context, imageName string) (*containerd.Image, error) {
+	// Get reference to synchronization object for image
+	mgr.Lock()
+	imgState, found := mgr.imageStates[imageName]
+	if !found {
+		imgState = NewImageState()
+		mgr.imageStates[imageName] = imgState
+	}
+	mgr.Unlock()
+
+	// Pull image if necessary. The image will only be pulled by the first thread to take the lock.
+	imgState.Lock()
+	if !imgState.isCached {
+		if err := mgr.pullImage(ctx, imageName); err != nil {
+			imgState.Unlock()
+			return nil, err
+		}
+		imgState.isCached = true
+	}
+	imgState.Unlock()
+
+	mgr.Lock()
+	image := mgr.cachedImages[imageName]
+	mgr.Unlock()
+
+	return &image, nil
+}
+
+// Converts an image name to a url if it is not a URL
+func getImageURL(image string) string {
+	// Pull from dockerhub by default if not specified (default k8s behavior)
+	if strings.Contains(image, ".") {
+		return image
+	}
+	return "docker.io/" + image
+
+}
+
+// Checks whether a URL has a .local domain
+func isLocalDomain(s string) (bool, error) {
+	if ! strings.Contains(s, "://") {
+		s = "dummy://" + s
+	}
+
+	u, err := url.Parse(s)
+	if err != nil {
+		return false, err
+	}
+
+	host, _, err := net.SplitHostPort(u.Host)
+	if err != nil {
+		host = u.Host
+	}
+
+	i := strings.LastIndex(host, ".")
+	tld := host[i+1:]
+
+	return tld == "local", nil
+}