fix: remove the imageExists check before Pull (#359)

Containerd does not need us to check for the existence of an image for it; it will download any layers which do not exist and wont re-download the same image twice. This check is causing us trouble now because with our capmvm base images we push new shas to the same tags. When we run this `imageExists` check using the ref (name+label) we don't get new changes on that version. This fix just leaves containerd to get on with it.
liquidmetal-dev · Jan 24, 2022 · 48a1ae5 · 48a1ae5
1 parent f7bc8d6
commit 48a1ae5
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 68 deletions.
diff --git a/infrastructure/containerd/image_service.go b/infrastructure/containerd/image_service.go
@@ -68,18 +68,9 @@ func (im *imageService) PullAndMount(ctx context.Context, input *ports.ImageMoun
 		return nil, fmt.Errorf("getting lease for image pulling and mounting: %w", err)
 	}
 
-	var image containerd.Image
-
-	exists, image, err := im.imageExists(leaseCtx, input.ImageName, input.Owner)
+	image, err := im.pullImage(leaseCtx, input.ImageName, input.Owner)
 	if err != nil {
-		return nil, fmt.Errorf("checking if image %s exists for owner %s: %w", input.ImageName, input.Owner, err)
-	}
-
-	if !exists {
-		image, err = im.pullImage(leaseCtx, input.ImageName, input.Owner)
-		if err != nil {
-			return nil, fmt.Errorf("getting image %s for owner %s: %w", input.ImageName, input.Owner, err)
-		}
+		return nil, fmt.Errorf("getting image %s for owner %s: %w", input.ImageName, input.Owner, err)
 	}
 
 	ss := im.getSnapshotter(input.Use)
@@ -94,7 +85,7 @@ func (im *imageService) Exists(ctx context.Context, input *ports.ImageSpec) (boo
 
 	nsCtx := namespaces.WithNamespace(ctx, im.config.Namespace)
 
-	exists, _, err := im.imageExists(nsCtx, input.ImageName, input.Owner)
+	exists, err := im.imageExists(nsCtx, input.ImageName, input.Owner)
 	if err != nil {
 		return false, fmt.Errorf("checking image exists: %w", err)
 	}
@@ -109,7 +100,7 @@ func (im *imageService) IsMounted(ctx context.Context, input *ports.ImageMountSp
 
 	nsCtx := namespaces.WithNamespace(ctx, im.config.Namespace)
 
-	exists, _, err := im.imageExists(nsCtx, input.ImageName, input.Owner)
+	exists, err := im.imageExists(nsCtx, input.ImageName, input.Owner)
 	if err != nil {
 		return false, fmt.Errorf("checking image exists: %w", err)
 	}
@@ -130,22 +121,21 @@ func (im *imageService) IsMounted(ctx context.Context, input *ports.ImageMountSp
 	return snapshotExists, nil
 }
 
-func (im *imageService) imageExists(ctx context.Context, imageName, owner string) (bool, containerd.Image, error) {
+func (im *imageService) imageExists(ctx context.Context, imageName, owner string) (bool, error) {
 	leaseCtx, err := withOwnerLease(ctx, owner, im.client)
 	if err != nil {
-		return false, nil, fmt.Errorf("getting lease for owner: %w", err)
+		return false, fmt.Errorf("getting lease for owner: %w", err)
 	}
 
-	image, err := im.client.GetImage(leaseCtx, imageName)
-	if err != nil {
+	if _, err := im.client.GetImage(leaseCtx, imageName); err != nil {
 		if errdefs.IsNotFound(err) {
-			return false, nil, nil
+			return false, nil
 		}
 
-		return false, nil, fmt.Errorf("getting image from containerd %s: %w", imageName, err)
+		return false, fmt.Errorf("getting image from containerd %s: %w", imageName, err)
 	}
 
-	return true, image, nil
+	return true, nil
 }
 
 func (im *imageService) pullImage(ctx context.Context, imageName string, owner string) (containerd.Image, error) {

diff --git a/infrastructure/containerd/image_service_test.go b/infrastructure/containerd/image_service_test.go
@@ -141,7 +141,7 @@ func TestImageService_PullAndMount(t *testing.T) {
 		RootFS(gomock.Any()).
 		Return([]digest.Digest{digest.Digest(hash)}, nil)
 	containerdClient.EXPECT().
-		GetImage(gomock.Any(), testImage).
+		Pull(gomock.Any(), testImage).
 		Return(image, nil)
 
 	_, err := client.PullAndMount(ctx, &ports.ImageMountSpec{
@@ -224,46 +224,6 @@ func TestImageService_PullAndMount_failedLeaseOnImageCheck(t *testing.T) {
 	g.Expect(err).To(g.HaveOccurred())
 }
 
-// TestImageService_PullAndMount_failedImageCheck tests what happens when
-// something goes wrong with imageExists (and not Leases).
-func TestImageService_PullAndMount_failedImageCheck(t *testing.T) {
-	g.RegisterTestingT(t)
-
-	mockCtrl := gomock.NewController(t)
-	containerdClient := mock.NewMockClient(mockCtrl)
-	leasesManager := mock.NewMockManager(mockCtrl)
-	svcConfig := containerd.Config{
-		SnapshotterKernel: "native",
-		SnapshotterVolume: "devmapper",
-		SocketPath:        "/something",
-		Namespace:         "unit_test_ns",
-	}
-	ctx := context.Background()
-	client := containerd.NewImageServiceWithClient(&svcConfig, containerdClient)
-
-	leasesManager.EXPECT().
-		List(gomock.Any(), fmt.Sprintf("id==flintlock/%s", testOwner)).
-		Times(2)
-	leasesManager.EXPECT().
-		Create(gomock.Any(), gomock.Any()).
-		Times(2)
-	containerdClient.EXPECT().
-		LeasesService().
-		Return(leasesManager).
-		Times(2)
-	containerdClient.EXPECT().
-		GetImage(gomock.Any(), testImage).
-		Return(nil, errors.New("nope"))
-
-	_, err := client.PullAndMount(ctx, &ports.ImageMountSpec{
-		ImageName:    testImage,
-		Owner:        testOwner,
-		Use:          models.ImageUseVolume,
-		OwnerUsageID: testOwnerID,
-	})
-	g.Expect(err).To(g.HaveOccurred())
-}
-
 // TestImageService_PullAndMount_failedUnpackCheck tests what happens when
 // something goes wrong with unpack check.
 func TestImageService_PullAndMount_failedUnpackCheck(t *testing.T) {
@@ -299,7 +259,7 @@ func TestImageService_PullAndMount_failedUnpackCheck(t *testing.T) {
 		Return(leasesManager).
 		Times(2)
 	containerdClient.EXPECT().
-		GetImage(gomock.Any(), testImage).
+		Pull(gomock.Any(), testImage).
 		Return(image, nil)
 
 	_, err := client.PullAndMount(ctx, &ports.ImageMountSpec{
@@ -350,7 +310,7 @@ func TestImageService_PullAndMount_failedUnpack(t *testing.T) {
 		Return(leasesManager).
 		Times(2)
 	containerdClient.EXPECT().
-		GetImage(gomock.Any(), testImage).
+		Pull(gomock.Any(), testImage).
 		Return(image, nil)
 
 	_, err := client.PullAndMount(ctx, &ports.ImageMountSpec{
@@ -386,6 +346,9 @@ func TestImageService_PullAndMount_failedRootFS(t *testing.T) {
 	leasesManager.EXPECT().
 		Create(gomock.Any(), gomock.Any()).
 		Times(2)
+	containerdClient.EXPECT().
+		Pull(gomock.Any(), testImage).
+		Return(image, nil)
 	image.EXPECT().
 		Name().
 		Return(testImage).
@@ -403,9 +366,6 @@ func TestImageService_PullAndMount_failedRootFS(t *testing.T) {
 		LeasesService().
 		Return(leasesManager).
 		Times(2)
-	containerdClient.EXPECT().
-		GetImage(gomock.Any(), testImage).
-		Return(image, nil)
 
 	_, err := client.PullAndMount(ctx, &ports.ImageMountSpec{
 		ImageName:    testImage,
@@ -467,7 +427,7 @@ func TestImageService_PullAndMount_failedSnapshotCheck(t *testing.T) {
 		Return(leasesManager).
 		Times(2)
 	containerdClient.EXPECT().
-		GetImage(gomock.Any(), testImage).
+		Pull(gomock.Any(), testImage).
 		Return(image, nil)
 
 	_, err := client.PullAndMount(ctx, &ports.ImageMountSpec{
@@ -537,7 +497,7 @@ func TestImageService_PullAndMount_failedPrepare(t *testing.T) {
 		Return(leasesManager).
 		Times(2)
 	containerdClient.EXPECT().
-		GetImage(gomock.Any(), testImage).
+		Pull(gomock.Any(), testImage).
 		Return(image, nil)
 
 	_, err := client.PullAndMount(ctx, &ports.ImageMountSpec{