diff --git a/integration/image_volume_linux_test.go b/integration/image_volume_linux_test.go index 76414007e8..d4a556ebd9 100644 --- a/integration/image_volume_linux_test.go +++ b/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() diff --git a/internal/cri/server/container_image_mount.go b/internal/cri/server/container_image_mount.go index 77fa1ec603..c319179214 100644 --- a/internal/cri/server/container_image_mount.go +++ b/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) } }