Skip to content

Commit 4df7d37

Browse files
authored
[shimV2] order container teardown so the UVM can reap bundle dirs (#2737)
* Deleting an LCOW container no longer leaves the bundle/scratch parent pod dir (/run/gcs/pods/<podID>) inside the UVM. * KillProcess fans the signal out to every exec when the caller asks for "all" or passes an empty execID, matching the "kill the whole container" path used by delete. Theme: re-sequence host- and guest-side teardown around DeleteContainerState. internal/controller/linuxcontainer: - Relocate the DeleteContainerState GCS RPC from closeContainer() to the tail of releaseResources(), so it runs after the combined-layer overlay, SCSI mounts, and VPCI devices are released. Issuing it earlier left the overlay mounted over the bundle root, which blocked the GCS from removing it. - Gate the RPC behind a new isContainerStateDeleted flag. - closeContainer() now only closes the container handle and terminatedCh. Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
1 parent c1a159c commit 4df7d37

3 files changed

Lines changed: 224 additions & 34 deletions

File tree

internal/controller/linuxcontainer/container.go

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/stats"
1313
"github.com/Microsoft/hcsshim/internal/controller/process"
1414
"github.com/Microsoft/hcsshim/internal/gcs"
15+
"github.com/Microsoft/hcsshim/internal/hcs"
1516
"github.com/Microsoft/hcsshim/internal/hcs/schema1"
1617
hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
1718
"github.com/Microsoft/hcsshim/internal/log"
@@ -90,6 +91,9 @@ type Controller struct {
9091

9192
// ioRetryTimeout is the duration to retry IO relay operations before giving up.
9293
ioRetryTimeout time.Duration
94+
95+
// isContainerStateDeleted tracks whether guest-side container state has been deleted.
96+
isContainerStateDeleted bool
9397
}
9498

9599
// New creates a ready-to-use Controller.
@@ -158,9 +162,8 @@ func (c *Controller) Create(ctx context.Context, spec *specs.Spec, opts *task.Cr
158162
if releaseErr := c.releaseResources(ctx); releaseErr != nil {
159163
log.G(ctx).WithError(releaseErr).Error("failed to release resources during create")
160164
}
161-
if closeErr := c.closeContainer(ctx); closeErr != nil {
162-
log.G(ctx).WithError(closeErr).Error("failed to close container during create")
163-
}
165+
// Close the container handle.
166+
c.closeContainer()
164167
}
165168
}()
166169

@@ -200,17 +203,8 @@ func (c *Controller) Create(ctx context.Context, spec *specs.Spec, opts *task.Cr
200203

201204
// closeContainer performs container teardown. It is safe to retry on
202205
// failure. Needs to be called while holding c.mu lock.
203-
func (c *Controller) closeContainer(ctx context.Context) error {
206+
func (c *Controller) closeContainer() {
204207
if c.container != nil {
205-
// Delete the guest-side container state if supported. If this
206-
// fails, return early without nil'ing c.container so a retry
207-
// re-issues the request.
208-
if c.guest.Capabilities().IsDeleteContainerStateSupported() {
209-
if err := c.guest.DeleteContainerState(ctx, c.gcsContainerID); err != nil {
210-
return fmt.Errorf("delete container state: %w", err)
211-
}
212-
}
213-
214208
// Close the container handle. The calling code never returns error.
215209
_ = c.container.Close()
216210
c.container = nil
@@ -224,7 +218,6 @@ func (c *Controller) closeContainer(ctx context.Context) error {
224218
default:
225219
close(c.terminatedCh)
226220
}
227-
return nil
228221
}
229222

230223
// releaseResources undoes each allocation in reverse order.
@@ -296,6 +289,20 @@ func (c *Controller) releaseResources(ctx context.Context) error {
296289
}
297290
}
298291

292+
// After layer overlay has been removed, we can safely delete the
293+
// bundle path inside the UVM for the container. Therefore, delete
294+
// the guest-side container state if supported.
295+
if !c.isContainerStateDeleted && c.guest.Capabilities().IsDeleteContainerStateSupported() {
296+
// GCS bridge evicts the container from its host-state map even if the inner Delete fails,
297+
// so retries will always return not-found.
298+
if err := c.guest.DeleteContainerState(ctx, c.gcsContainerID); err != nil && !hcs.IsNotExist(err) {
299+
return fmt.Errorf("delete container state: %w", err)
300+
}
301+
302+
// Set isContainerStateDeleted to true so that we do not retry this post successful delete.
303+
c.isContainerStateDeleted = true
304+
}
305+
299306
return nil
300307
}
301308

@@ -345,12 +352,7 @@ func (c *Controller) handleInitProcessExit(ctx context.Context, initProcess *pro
345352

346353
c.mu.Lock()
347354
c.state = StateStopped
348-
if err := c.closeContainer(ctx); err != nil {
349-
// Leave state as StateStopped so DeleteProcess can retry the
350-
// teardown. The exit event below still informs the caller that
351-
// the init process is gone.
352-
log.G(ctx).WithError(err).Error("failed to close container after init exit")
353-
}
355+
c.closeContainer()
354356
c.mu.Unlock()
355357

356358
// Publish the exit event after teardown is complete.
@@ -549,7 +551,7 @@ func (c *Controller) KillProcess(ctx context.Context, execID string, signal uint
549551
// When "all" is requested, deliver the signal to every additional exec
550552
// on a best-effort basis. Errors are logged but do not prevent the
551553
// target process from being signaled.
552-
if all {
554+
if all || execID == "" {
553555
for eid, proc := range c.processes {
554556
if eid == "" {
555557
// The init process is signaled as the explicit target below.
@@ -616,12 +618,11 @@ func (c *Controller) DeleteProcess(ctx context.Context, execID string) (*task.St
616618
// For containers that were created but never started, handleInitProcessExit
617619
// was never launched, so closeContainer was never called. Perform full
618620
// teardown now. closeContainer is retriable.
619-
if err = c.closeContainer(ctx); err != nil {
620-
return nil, fmt.Errorf("close container %s: %w", c.containerID, err)
621-
}
622621
if err = c.releaseResources(ctx); err != nil {
623622
return nil, fmt.Errorf("releasing resources for container %s: %w", c.containerID, err)
624623
}
624+
// Close container handle after the resources are released.
625+
c.closeContainer()
625626
}
626627

627628
// Remove the process entry only after all fallible operations have

0 commit comments

Comments
 (0)