From fec8e08ab641b9e3f0c9121861a6d8cba020f3fe Mon Sep 17 00:00:00 2001
From: Amit Barve <ambarve@microsoft.com>
Date: Wed, 21 Apr 2021 16:51:19 -0700
Subject: [PATCH] Minor fixes to shim panic log & create task functions

The change to collect shim panic logs during shim delete command does not work in cases
when the delete command itself runs into some error. To avoid losing shim panic logs in
such cases we log the shim panic logs (if found) as first thing in the delete command.

CreateTask function had `wcl` mutex lock that wasn't really being used anywhere, this
change removes that. We also don't add a `nil` entry for a new task in the `workloadTasks`
map anymore to avoid the shim panic in some cases where a `GetTask` function might
be called while we are still in the process of creating the task and haven't updated the
nil entry with the actual task struct reference.
---
 cmd/containerd-shim-runhcs-v1/delete.go | 16 +++++++++-------
 cmd/containerd-shim-runhcs-v1/pod.go    | 15 ++-------------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/cmd/containerd-shim-runhcs-v1/delete.go b/cmd/containerd-shim-runhcs-v1/delete.go
index 9644c06b7b..ec673c4bec 100644
--- a/cmd/containerd-shim-runhcs-v1/delete.go
+++ b/cmd/containerd-shim-runhcs-v1/delete.go
@@ -40,6 +40,15 @@ The delete command will be executed in the container's bundle as its cwd.
 			return errors.New("bundle is required")
 		}
 
+		// hcsshim shim writes panic logs in the bundle directory in a file named "panic.log"
+		// log those messages (if any) on stderr so that it shows up in containerd's log.
+		// This should be done as the first thing so that we don't miss any panic logs even if
+		// something goes wrong during delete op.
+		logs, err := ioutil.ReadFile(filepath.Join(bundleFlag, "panic.log"))
+		if err == nil && len(logs) > 0 {
+			logrus.WithField("log", string(logs)).Error("found shim panic logs during delete")
+		}
+
 		// Attempt to find the hcssystem for this bundle and terminate it.
 		if sys, _ := hcs.OpenComputeSystem(ctx, idFlag); sys != nil {
 			defer sys.Close()
@@ -82,13 +91,6 @@ The delete command will be executed in the container's bundle as its cwd.
 			}
 		}
 
-		// hcsshim shim writes panic logs in the bundle directory in a file named "panic.log"
-		// log those messages (if any) on stderr so that it shows up in containerd's log.
-		logs, err := ioutil.ReadFile(filepath.Join(bundleFlag, "panic.log"))
-		if err == nil && len(logs) > 0 {
-			logrus.WithField("log", string(logs)).Error("found shim panic logs during delete")
-		}
-
 		// Remove the bundle on disk
 		if err := os.RemoveAll(bundleFlag); err != nil && !os.IsNotExist(err) {
 			return err
diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go
index 73a3247998..687dba1179 100644
--- a/cmd/containerd-shim-runhcs-v1/pod.go
+++ b/cmd/containerd-shim-runhcs-v1/pod.go
@@ -232,10 +232,6 @@ type pod struct {
 	// It MUST be treated as read only in the lifetime of the pod.
 	host *uvm.UtilityVM
 
-	// wcl is the workload create mutex. All calls to CreateTask must hold this
-	// lock while the ID reservation takes place. Once the ID is held it is safe
-	// to release the lock to allow concurrent creates.
-	wcl           sync.Mutex
 	workloadTasks sync.Map
 }
 
@@ -262,17 +258,10 @@ func (p *pod) CreateTask(ctx context.Context, req *task.CreateTaskRequest, s *sp
 		return nil, errors.Wrapf(errdefs.ErrFailedPrecondition, "task with id: '%s' cannot be created in pod: '%s' which is not running", req.ID, p.id)
 	}
 
-	p.wcl.Lock()
-	_, loaded := p.workloadTasks.LoadOrStore(req.ID, nil)
-	if loaded {
+	_, ok := p.workloadTasks.Load(req.ID)
+	if ok {
 		return nil, errors.Wrapf(errdefs.ErrAlreadyExists, "task with id: '%s' already exists id pod: '%s'", req.ID, p.id)
 	}
-	p.wcl.Unlock()
-	defer func() {
-		if err != nil {
-			p.workloadTasks.Delete(req.ID)
-		}
-	}()
 
 	ct, sid, err := oci.GetSandboxTypeAndID(s.Annotations)
 	if err != nil {