Merge pull request #14143 from fuweid/follow-up-13542

cri: only unmount image volumes when mounting fails

integration/image_volume_linux_test.go

@@ -188,6 +188,35 @@ func TestImageVolumeBasic(t *testing.T) {
 	}
 }
 
+func TestImageVolumeReusableAfterFailure(t *testing.T) {
+	ctx := namespaces.WithNamespace(context.Background(), "k8s.io")
+	image := images.Get(images.Alpine)
+	pullImagesByCRI(t, imageService, image)
+
+	img, err := containerdClient.GetImage(ctx, image)
+	require.NoError(t, err)
+	podCtx := newPodTCtx(t, runtimeService, t.Name(), "image-volume")
+	defer podCtx.stop(true)
+
+	target := filepath.Join(podCtx.imageVolumeDir(), img.Target().Digest.Encoded())
+
+	cfg := ContainerConfig("invalid-image-subpath", image,
+		WithImageVolumeMount(image, "does-not-exist", "/image-mount"))
+	_, err = podCtx.rSvc.CreateContainer(podCtx.id, cfg, podCtx.cfg)
+	require.ErrorContains(t, err, "failed to ensure image subpath")
+
+	_, err = containerdClient.SnapshotService("overlayfs").Stat(ctx, target)
+	require.NoError(t, err, "image volume snapshot should remain after the failed attempt")
+
+	cnID := podCtx.createContainer("valid-image-subpath", image,
+		criruntime.ContainerState_CONTAINER_RUNNING,
+		WithCommand("sleep", "1d"),
+		WithImageVolumeMount(image, "etc", "/image-mount"))
+	stdout, _, err := podCtx.rSvc.ExecSync(cnID, []string{"cat", "/image-mount/os-release"}, 0)
+	require.NoError(t, err)
+	require.Contains(t, string(stdout), "Alpine Linux")
+}
+
 func setupRunningContainerWithImageVolume(t *testing.T, selinuxLevel string, containerImage string, imageVolumeName, imageSubPath, containerPath string) (podCtx *podTCtx, cnID string, err error) {
 	podLogDir := t.TempDir()
 

internal/cri/server/container_image_mount.go

@@ -72,7 +72,7 @@ func (c *criService) mutateImageMount(
 	snapshotter string,
 	sandboxID string,
 	platform imagespec.Platform,
-) (retErr error) {
+) error {
 	imageSpec := extraMount.GetImage()
 	if imageSpec == nil {
 		return nil
@@ -103,6 +103,9 @@ func (c *criService) mutateImageMount(
 	target := c.getImageVolumeHostPath(sandboxID, imageID)
 
 	// Already mounted in another container on the same pod
+	//
+	// TODO: Serialize image volume setup per target to avoid races between
+	// checking the mount and mounting it.
 	mounted, err := ensureImageVolumeMounted(target)
 	if err != nil {
 		return fmt.Errorf("failed to ensure %s is mounted: %w", target, err)
@@ -131,6 +134,10 @@ func (c *criService) mutateImageMount(
 		}
 
 		s := c.client.SnapshotService(snapshotter)
+
+		// Keep the snapshot on failure because the leased mount-manager
+		// activation can back-reference it. Sandbox cleanup and GC
+		// release both resources.
 		mounts, err := s.Prepare(ctx, target, chainID, snapshotOpts...)
 		if err != nil {
 			if errdefs.IsAlreadyExists(err) {
@@ -140,18 +147,12 @@ func (c *criService) mutateImageMount(
 		if err != nil {
 			return fmt.Errorf("failed to prepare for image volume %q: %w", ref, err)
 		}
-		defer func() {
-			if retErr != nil {
-				_ = s.Remove(ctx, target)
-			}
-		}()
 
 		err = os.MkdirAll(target, 0755)
 		if err != nil {
 			return fmt.Errorf("failed to create directory to image volume target path %q: %w", target, err)
 		}
 
-		mm := c.client.MountManager()
 		id := fmt.Sprintf("cri-image-mount-%s", target)
 		activateOpts := []mount.ActivateOpt{
 			mount.WithLabels(map[string]string{
@@ -159,6 +160,7 @@ func (c *criService) mutateImageMount(
 			}),
 		}
 
+		mm := c.client.MountManager()
 		info, err := mm.Activate(ctx, id, mounts, activateOpts...)
 		if err == nil {
 			mounts = info.System
@@ -176,16 +178,10 @@ func (c *criService) mutateImageMount(
 
 		mounts = addVolatileOptionOnImageVolumeMount(mounts)
 
-		// if mutateImageMount() fails, do not leak the new mounts
-		mountTarget := target
-		defer func() {
-			if retErr != nil {
-				if err := mount.UnmountAll(mountTarget, 0); err != nil {
-					log.G(ctx).WithError(err).Errorf("failed to unmount image volume component %q", mountTarget)
-				}
-			}
-		}()
 		if err := mount.All(mounts, target); err != nil {
+			if unmountErr := mount.UnmountAll(target, 0); unmountErr != nil {
+				log.G(ctx).WithError(unmountErr).Errorf("failed to unmount image volume component %q", target)
+			}
 			return fmt.Errorf("failed to mount image volume component %q: %w", target, err)
 		}
 	}