Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 20 additions & 17 deletions internal/controller/linuxcontainer/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
19 changes: 14 additions & 5 deletions internal/controller/linuxcontainer/container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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)

Expand All @@ -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 {
Expand All @@ -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),
Expand Down
67 changes: 67 additions & 0 deletions internal/guest/runtime/hcsv2/bundle_bind_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
})
}
}
15 changes: 15 additions & 0 deletions internal/guest/runtime/hcsv2/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
62 changes: 61 additions & 1 deletion internal/guest/runtime/hcsv2/uvm.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
Loading