diff --git a/internal/controller/linuxcontainer/container.go b/internal/controller/linuxcontainer/container.go index d28ff65a51..5f36374cca 100644 --- a/internal/controller/linuxcontainer/container.go +++ b/internal/controller/linuxcontainer/container.go @@ -255,6 +255,26 @@ func (c *Controller) releaseResources(ctx context.Context) error { c.layers.layersCombined = false } + // DeleteContainerState performs the guest-side container delete: it unmounts + // the pod's sandbox mounts and the scratch-backed bundle bind, deletes the + // runtime state, and removes the bundle and scratch directories. We run it + // before removing the overlay layer devices and detaching the scratch disk, + // so the guest releases everything backed by those disks while they are still + // attached (the overlay mount itself has already been removed above). + // Short-circuit on isContainerStateDeleted so retries don't re-issue Capabilities. + if !c.isContainerStateDeleted { + if caps := c.guest.Capabilities(); caps != nil && caps.IsDeleteContainerStateSupported() { + // GCS bridge evicts the container from its host-state map even if the inner Delete fails, + // so retries will always return not-found. + if err := c.guest.DeleteContainerState(ctx, c.gcsContainerID); err != nil && !isResourceAlreadyReleased(err) { + return fmt.Errorf("delete container state: %w", err) + } + + // Set isContainerStateDeleted to true so that we do not retry this post successful delete. + c.isContainerStateDeleted = true + } + } + // Unmap the scratch layer. A zero ID indicates it has already been // unmapped on a prior call. var zeroGUID guid.GUID @@ -300,23 +320,6 @@ func (c *Controller) releaseResources(ctx context.Context) error { } } - // After layer overlay has been removed, we can safely delete the - // bundle path inside the UVM for the container. Therefore, delete - // the guest-side container state if supported. Short-circuit on - // isContainerStateDeleted so retries don't re-issue Capabilities. - if !c.isContainerStateDeleted { - if caps := c.guest.Capabilities(); caps != nil && caps.IsDeleteContainerStateSupported() { - // GCS bridge evicts the container from its host-state map even if the inner Delete fails, - // so retries will always return not-found. - if err := c.guest.DeleteContainerState(ctx, c.gcsContainerID); err != nil && !isResourceAlreadyReleased(err) { - return fmt.Errorf("delete container state: %w", err) - } - - // Set isContainerStateDeleted to true so that we do not retry this post successful delete. - c.isContainerStateDeleted = true - } - } - return nil } diff --git a/internal/controller/linuxcontainer/container_test.go b/internal/controller/linuxcontainer/container_test.go index 8350161788..a8de0f5b44 100644 --- a/internal/controller/linuxcontainer/container_test.go +++ b/internal/controller/linuxcontainer/container_test.go @@ -675,12 +675,15 @@ func TestReleaseResources_StopsOnFirstError(t *testing.T) { scsiCtrl.EXPECT().UnmapFromGuest(gomock.Any(), scsiGUID).Return(tc.scsiErr) + // The DeleteContainerState capability gate now runs early (after + // RemoveCombinedLayers, before the unmaps), so Capabilities is always + // queried regardless of whether a later unmap fails. + guestCtrl.EXPECT().Capabilities().Return(&gcs.LCOWGuestDefinedCapabilities{}) + if !tc.wantStops { - // Tolerated error: the chain must proceed through plan9, vpci, - // and the DeleteContainerState capability gate. + // Tolerated error: the chain must proceed through plan9 and vpci. plan9Ctrl.EXPECT().UnmapFromGuest(gomock.Any(), plan9GUID).Return(nil) vpciCtrl.EXPECT().RemoveFromVM(gomock.Any(), deviceGUID).Return(nil) - guestCtrl.EXPECT().Capabilities().Return(&gcs.LCOWGuestDefinedCapabilities{}) } err := c.releaseResources(t.Context()) @@ -777,13 +780,16 @@ func TestReleaseResources_RemoveCombinedLayersFails(t *testing.T) { // failure leaves the scratch reservation intact for retry. func TestReleaseResources_ScratchUnmapFails(t *testing.T) { t.Parallel() - c, scsiCtrl, _, _, _ := newContainerTestController(t) + c, scsiCtrl, _, _, guestCtrl := newContainerTestController(t) scratchGUID, _ := guid.NewV4() c.layers = &scsiLayers{ scratch: scsiReservation{id: scratchGUID, guestPath: "/dev/scratch"}, } + // DeleteContainerState's capability gate runs before the scratch unmap. + guestCtrl.EXPECT().Capabilities().Return(&gcs.LCOWGuestDefinedCapabilities{}) + wantErr := errors.New("scratch unmap failed") scsiCtrl.EXPECT().UnmapFromGuest(gomock.Any(), scratchGUID).Return(wantErr) @@ -801,7 +807,7 @@ func TestReleaseResources_ScratchUnmapFails(t *testing.T) { // retained while already-unmapped entries are dropped. func TestReleaseResources_ROLayerUnmapMidwayFails(t *testing.T) { t.Parallel() - c, scsiCtrl, _, _, _ := newContainerTestController(t) + c, scsiCtrl, _, _, guestCtrl := newContainerTestController(t) roGUIDs := [3]guid.GUID{} for i := range roGUIDs { @@ -816,6 +822,9 @@ func TestReleaseResources_ROLayerUnmapMidwayFails(t *testing.T) { }, } + // DeleteContainerState's capability gate runs before the RO layer unmaps. + guestCtrl.EXPECT().Capabilities().Return(&gcs.LCOWGuestDefinedCapabilities{}) + wantErr := errors.New("ro layer unmap failed") gomock.InOrder( scsiCtrl.EXPECT().UnmapFromGuest(gomock.Any(), roGUIDs[0]).Return(nil), diff --git a/internal/guest/runtime/hcsv2/bundle_bind_test.go b/internal/guest/runtime/hcsv2/bundle_bind_test.go new file mode 100644 index 0000000000..63364e3d22 --- /dev/null +++ b/internal/guest/runtime/hcsv2/bundle_bind_test.go @@ -0,0 +1,67 @@ +//go:build linux +// +build linux + +package hcsv2 + +import "testing" + +// TestBundleNeedsScratchBind covers the path-based rule that decides whether a +// container bundle must be bind-mounted onto its scratch disk. The cases mirror +// the layouts observed for both shims: +// - V1 non-shared: the shim mounts the scratch disk at the bundle path, so the +// scratch dir is nested under the bundle and no bind is needed. +// - V1 shared-scratch: the scratch lives under the sandbox's bundle, so a +// workload's own bundle is not disk-backed and needs a bind. +// - V2: the scratch is mounted at a global path unrelated to the bundle, so +// the bundle needs a bind. +func TestBundleNeedsScratchBind(t *testing.T) { + for _, tc := range []struct { + name string + bundleDir string + scratchDir string + want bool + }{ + { + name: "no scratch", + bundleDir: "/run/gcs/c/abc", + scratchDir: "", + want: false, + }, + { + name: "v1 non-shared scratch under bundle", + bundleDir: "/run/gcs/c/abc", + scratchDir: "/run/gcs/c/abc/scratch/abc", + want: false, + }, + { + name: "v1 shared scratch under sandbox bundle", + bundleDir: "/run/gcs/c/workload", + scratchDir: "/run/gcs/c/sandbox/scratch/workload", + want: true, + }, + { + name: "v2 scratch on global scsi mount", + bundleDir: "/run/gcs/pods/pod/ctr", + scratchDir: "/run/mounts/scsi/0_2_0/scratch/pod/ctr", + want: true, + }, + { + name: "scratch equals bundle", + bundleDir: "/run/gcs/c/abc", + scratchDir: "/run/gcs/c/abc", + want: false, + }, + { + name: "sibling dir with shared prefix is not under bundle", + bundleDir: "/run/gcs/c/abc", + scratchDir: "/run/gcs/c/abcdef/scratch/abcdef", + want: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if got := bundleNeedsScratchBind(tc.bundleDir, tc.scratchDir); got != tc.want { + t.Errorf("bundleNeedsScratchBind(%q, %q) = %v, want %v", tc.bundleDir, tc.scratchDir, got, tc.want) + } + }) + } +} diff --git a/internal/guest/runtime/hcsv2/container.go b/internal/guest/runtime/hcsv2/container.go index 26924687d3..c5a1f43025 100644 --- a/internal/guest/runtime/hcsv2/container.go +++ b/internal/guest/runtime/hcsv2/container.go @@ -266,6 +266,21 @@ func (c *Container) Delete(ctx context.Context) error { retErr = err } + // If the bundle was bind-mounted onto the scratch disk during create + // (modifyCombinedLayers), undo that bind. The same path-based rule is used + // here as at create time, so both shims are handled identically. Order + // matters: this runs before removing the scratch/bundle dirs below (the + // bind's source lives under scratchDirPath) and before the host unmaps the + // scratch disk, so no mount still references that disk. removeTarget=false + // detaches the bind only - the ociBundlePath directory itself remains and is + // deleted by RemoveAll below. + if bundleNeedsScratchBind(c.ociBundlePath, c.scratchDirPath) { + if err := storage.UnmountPath(ctx, c.ociBundlePath, false); err != nil { + entity.WithError(err).WithField("bundle", c.ociBundlePath). + Warn("failed to unmount scratch-backed bundle bind") + } + } + if err := os.RemoveAll(c.scratchDirPath); err != nil { if retErr != nil { retErr = fmt.Errorf("errors deleting container state: %w; %w", retErr, err) diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 45266e4193..2527393953 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -1731,11 +1731,31 @@ func (h *Host) modifyCombinedLayers( }() } + // The sandbox root (the bundle dir) must be disk-backed so that + // everything created under it - sandbox:// mounts, logs, etc. - lives + // on disk rather than the UVM tmpfs. When the shim already mounts the + // scratch disk at the bundle path, the scratch dir is nested under the + // bundle and there is nothing to do. Otherwise (the scratch is mounted + // elsewhere and the bundle would be tmpfs) bind a scratch directory + // onto the bundle. This is decided purely from the paths, so V1 and V2 + // are handled by the same rule. + bundleDir := filepath.Dir(cl.ContainerRootPath) + bindBundle := bundleNeedsScratchBind(bundleDir, cl.ScratchPath) + if bindBundle { + if bindErr := bindBundleToScratch(cl.ScratchPath, bundleDir); bindErr != nil { + return bindErr + } + } + // Correctness for policy transaction rollback: // MountLayer does two things - mkdir, then mount. On mount failure, the // target directory is cleaned up. Therefore we're clean in terms of // side effects. - return overlay.MountLayer(ctx, layerPaths, upperdirPath, workdirPath, cl.ContainerRootPath, readonly) + mountErr := overlay.MountLayer(ctx, layerPaths, upperdirPath, workdirPath, cl.ContainerRootPath, readonly) + if mountErr != nil && bindBundle { + _ = storage.UnmountPath(ctx, bundleDir, false) + } + return mountErr case guestrequest.RequestTypeRemove: // cl.ContainerID is not set on remove requests, but rego checks that we can // only umount previously mounted targets anyway @@ -1775,6 +1795,46 @@ func (h *Host) modifyCombinedLayers( }) } +// bundleNeedsScratchBind reports whether the container's bundle directory needs +// to be bind-mounted onto its scratch disk to be disk-backed. It returns false +// when there is no scratch, or when the scratch dir is already nested under the +// bundle dir (i.e. the shim mounted the scratch disk at the bundle path, so the +// bundle is already on disk). It returns true when the scratch is mounted +// elsewhere and the bundle would otherwise live on the UVM tmpfs. The decision +// is derived only from the paths, so it applies identically to both shims. +func bundleNeedsScratchBind(bundleDir, scratchDir string) bool { + if scratchDir == "" { + return false + } + rel, err := filepath.Rel(bundleDir, scratchDir) + if err != nil { + // Unrelated paths: the scratch is not under the bundle, so it needs a bind. + return true + } + // A bind is needed only when scratchDir is not nested under bundleDir, i.e. + // when the relative path escapes the bundle (is ".." or starts with "../"). + return rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + +// bindBundleToScratch backs a container bundle directory with a directory on the +// container's scratch disk, so the sandbox root - and everything created under +// it (sandbox:// mounts, logs, etc.) - is disk-backed rather than tmpfs-backed. +// The bind is removed during container delete, before the scratch disk is +// unmapped. +func bindBundleToScratch(scratchPath, bundleDir string) error { + src := filepath.Join(scratchPath, "bundle") + if err := os.MkdirAll(src, 0755); err != nil { + return fmt.Errorf("create bundle backing dir %s: %w", src, err) + } + if err := os.MkdirAll(bundleDir, 0755); err != nil { + return fmt.Errorf("create bundle dir %s: %w", bundleDir, err) + } + if err := unix.Mount(src, bundleDir, "", unix.MS_BIND, ""); err != nil { + return fmt.Errorf("bind bundle dir %s to %s: %w", src, bundleDir, err) + } + return nil +} + func modifyNetwork(ctx context.Context, rt guestrequest.RequestType, na *guestresource.LCOWNetworkAdapter) (err error) { switch rt { case guestrequest.RequestTypeAdd: