From 8e1e77876490dc2e8698adc02e0a909a29b7d221 Mon Sep 17 00:00:00 2001
From: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Date: Mon, 7 Feb 2022 19:24:31 -0500
Subject: [PATCH] PR: errors messages and docs

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
---
 cmd/containerd-shim-runhcs-v1/pod.go          | 11 ++++++--
 cmd/containerd-shim-runhcs-v1/service.go      |  3 +-
 .../service_internal.go                       | 28 +++++++++++++------
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go
index df98fb8820..922f8cb3b2 100644
--- a/cmd/containerd-shim-runhcs-v1/pod.go
+++ b/cmd/containerd-shim-runhcs-v1/pod.go
@@ -52,7 +52,7 @@ type shimPod interface {
 	// not in this state this pod MUST return `errdefs.ErrFailedPrecondition`.
 	KillTask(ctx context.Context, tid, eid string, signal uint32, all bool) error
 	// DeleteTask removes a task from being tracked by this pod, and cleans up
-	// and resources the shim allocated to the task.
+	// the resources the shim allocated for the task.
 	//
 	// The task's init exec (eid == "") must be either `shimExecStateCreated` or
 	// `shimExecStateExited`.  If the exec is not in this state this pod MUST
@@ -400,12 +400,17 @@ func (p *pod) KillTask(ctx context.Context, tid, eid string, signal uint32, all
 }
 
 func (p *pod) DeleteTask(ctx context.Context, tid string) error {
+	// Deleting the sandbox task is a no-op, since the service should delete its
+	// reference to the sandbox task or pod, and `p.sandboxTask != nil` is an
+	// invariant that is relied on elsewhere.
+	// However, still get the init exec for all tasks to ensure that they have
+	// been properly stopped.
+
 	t, err := p.GetTask(tid)
 	if err != nil {
-		return errors.Wrap(err, "could not delete task")
+		return errors.Wrap(err, "could not find task to delete")
 	}
 
-	// although deleting the sandbox task is a no-op, still check that it is not running
 	e, err := t.GetExec("")
 	if err != nil {
 		return errors.Wrap(err, "could not get initial exec")
diff --git a/cmd/containerd-shim-runhcs-v1/service.go b/cmd/containerd-shim-runhcs-v1/service.go
index cc25d87ae1..1bcb30932e 100644
--- a/cmd/containerd-shim-runhcs-v1/service.go
+++ b/cmd/containerd-shim-runhcs-v1/service.go
@@ -37,7 +37,8 @@ type service struct {
 
 	// taskOrPod is either the `pod` this shim is tracking if `isSandbox ==
 	// true` or it is the `task` this shim is tracking. If no call to `Create`
-	// has taken place yet `taskOrPod.Load()` MUST return `nil`.
+	// has taken place yet, or `Delete` was called on the init task id (`tid`),
+	// then `taskOrPod.Load()` MUST return `nil`.
 	taskOrPod atomic.Value
 
 	// cl is the create lock. Since each shim MUST only track a single task or
diff --git a/cmd/containerd-shim-runhcs-v1/service_internal.go b/cmd/containerd-shim-runhcs-v1/service_internal.go
index 9824a7d8a6..666a3f8b08 100644
--- a/cmd/containerd-shim-runhcs-v1/service_internal.go
+++ b/cmd/containerd-shim-runhcs-v1/service_internal.go
@@ -3,9 +3,11 @@ package main
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
+	"sync/atomic"
 
 	runhcsopts "github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/options"
 	"github.com/Microsoft/hcsshim/internal/oci"
@@ -215,16 +217,24 @@ func (s *service) deleteInternal(ctx context.Context, req *task.DeleteRequest) (
 		return nil, err
 	}
 
-	// remove the pod sandbox's reference to the task, if the delete is for a
-	// task and not an exec
-	if s.isSandbox && req.ExecID == "" {
-		p, err := s.getPod()
-		if err != nil {
-			return nil, errors.Wrapf(err, "could not get pod %q to delete task %q", s.tid, req.ID)
+	if req.ExecID == "" {
+		// delete is for a task, and not an exec
+		if s.isSandbox {
+			// remove the pod sandbox's reference to the task
+			p, err := s.getPod()
+			if err != nil {
+				return nil, errors.Wrapf(err, "could not get pod %q to delete task %q", s.tid, req.ID)
+			}
+			err = p.DeleteTask(ctx, req.ID)
+			if err != nil {
+				return nil, fmt.Errorf("could not delete task %q in pod %q: %w", req.ID, s.tid, err)
+
+			}
 		}
-		err = p.DeleteTask(ctx, req.ID)
-		if err != nil {
-			return nil, errors.Wrapf(err, "could not delete task %q in pod %q", req.ID, s.tid)
+		if req.ID == s.tid {
+			// deleting the init task, so delete the internal reference to it, cleaning up
+			// any final resources the service had for it
+			s.taskOrPod = atomic.Value{}
 		}
 	}