Delete shim workloads tasks in pod.

This commit supports restarting containers and pods using CRI. Delete task request now removes tasks from `pod`'s `workloadTasks` map, and added `DeleteTask` function to `shimPod` interface so new tasks can use the same ID (ie, so a task can be restarted in a running pod). Signed-off-by: Hamza El-Saawy <[email protected]>
microsoft · Jan 10, 2022 · 2b7d978 · 2b7d978
1 parent 77c0270
commit 2b7d978
Show file tree

Hide file tree

Showing 3 changed files with 206 additions and 3 deletions.
diff --git a/cmd/containerd-shim-runhcs-v1/pod.go b/cmd/containerd-shim-runhcs-v1/pod.go
@@ -51,6 +51,14 @@ type shimPod interface {
 	// the `shimExecStateRunning, shimExecStateExited` states. If the exec is
 	// 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 task's init exec (eid == "") must be either `shimExecStateCreated` or
+	// `shimExecStateExited`.  If the exec is not in this state this pod MUST
+	// return `errdefs.ErrFailedPrecondition`. Deleting the pod's sandbox task
+	// is a no-op.
+	DeleteTask(ctx context.Context, tid string) error
 }
 
 func createPod(ctx context.Context, events publisher, req *task.CreateTaskRequest, s *specs.Spec) (shimPod, error) {
@@ -390,3 +398,25 @@ func (p *pod) KillTask(ctx context.Context, tid, eid string, signal uint32, all
 	})
 	return eg.Wait()
 }
+
+func (p *pod) DeleteTask(ctx context.Context, tid string) error {
+	t, err := p.GetTask(tid)
+	if err != nil {
+		return errors.Wrapf(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.Wrapf(err, "could not get initial exec")
+	}
+	if e.State() == shimExecStateRunning {
+		return errors.Wrapf(errdefs.ErrFailedPrecondition, "cannot delete task with running exec")
+	}
+
+	if p.id != tid {
+		p.workloadTasks.Delete(tid)
+	}
+
+	return nil
+}
diff --git a/cmd/containerd-shim-runhcs-v1/pod_test.go b/cmd/containerd-shim-runhcs-v1/pod_test.go
@@ -44,6 +44,28 @@ func (tsp *testShimPod) KillTask(ctx context.Context, tid, eid string, signal ui
 	return s.KillExec(ctx, eid, signal, all)
 }
 
+func (tsp *testShimPod) DeleteTask(ctx context.Context, tid string) error {
+	t, err := tsp.GetTask(tid)
+	if err != nil {
+		return err
+	}
+
+	e, err := t.GetExec("")
+	if err != nil {
+		return err
+	}
+	switch e.State() {
+	case shimExecStateRunning:
+		return errdefs.ErrFailedPrecondition
+	default:
+	}
+
+	if tid != tsp.ID() {
+		tsp.tasks.Delete(tid)
+	}
+	return nil
+}
+
 // Pod tests
 
 func setupTestPodWithFakes(t *testing.T) (*pod, *testShimTask) {
@@ -111,6 +133,8 @@ func Test_pod_GetTask_WorkloadID_Created_Success(t *testing.T) {
 	}
 }
 
+// kill tests
+
 func Test_pod_KillTask_UnknownTaskID_Error(t *testing.T) {
 	p, _ := setupTestPodWithFakes(t)
 	err := p.KillTask(context.TODO(), "thisshouldnotmatch", "", 0xf, false)
@@ -205,3 +229,140 @@ func Test_pod_KillTask_WorkloadID_2ndExecID_All_Error(t *testing.T) {
 		verifyExpectedError(t, nil, err, errdefs.ErrFailedPrecondition)
 	}
 }
+
+// delete tests
+
+func Test_pod_DeleteTask_SandboxID(t *testing.T) {
+	p, st := setupTestPodWithFakes(t)
+
+	err := p.KillTask(context.Background(), st.ID(), "", 0xf, true)
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	// it should not be possible to delete the sandbox task
+	_, err = p.GetTask(t.Name())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+}
+
+func Test_pod_DeleteTask_SandboxID_Running(t *testing.T) {
+	p, st := setupTestPodWithFakes(t)
+
+	// start the task
+	e, err := st.GetExec("")
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+	err = e.Start(context.Background())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	verifyExpectedError(t, nil, err, errdefs.ErrFailedPrecondition)
+}
+
+func Test_pod_DeleteTask_SandboxID_Repeated(t *testing.T) {
+	p, st := setupTestPodWithFakes(t)
+
+	err := p.KillTask(context.Background(), st.ID(), "", 0xf, true)
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+}
+
+func Test_pod_DeleteTask_TaskID(t *testing.T) {
+	p, _ := setupTestPodWithFakes(t)
+	st := setupTestTaskInPod(t, p)
+
+	err := p.KillTask(context.Background(), st.ID(), "", 0xf, true)
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	_, err = p.GetTask(st.ID())
+	verifyExpectedError(t, nil, err, errdefs.ErrNotFound)
+}
+
+func Test_pod_DeleteTask_TaskID_Running(t *testing.T) {
+	p, _ := setupTestPodWithFakes(t)
+	st := setupTestTaskInPod(t, p)
+
+	// start the task
+	e, err := st.GetExec("")
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+	err = e.Start(context.Background())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	verifyExpectedError(t, nil, err, errdefs.ErrFailedPrecondition)
+
+	// should not actually delete the sandbox task
+	_, err = p.GetTask(t.Name())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	stp, err := p.GetTask(st.ID())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	if stp != st {
+		t.Fatalf("task should not have changed: %v != %v", st, stp)
+	}
+}
+
+func Test_pod_DeleteTask_TaskID_Repeated(t *testing.T) {
+	p, _ := setupTestPodWithFakes(t)
+	st := setupTestTaskInPod(t, p)
+
+	err := p.KillTask(context.Background(), st.ID(), "", 0xf, true)
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	if err != nil {
+		t.Fatalf("should not have failed, got: %v", err)
+	}
+
+	err = p.DeleteTask(context.Background(), st.ID())
+	verifyExpectedError(t, nil, err, errdefs.ErrNotFound)
+}
+
+func Test_pod_DeleteTask_TaskID_Not_Created(t *testing.T) {
+	p, _ := setupTestPodWithFakes(t)
+	// Add two workload tasks
+	setupTestTaskInPod(t, p)
+	setupTestTaskInPod(t, p)
+
+	err := p.KillTask(context.Background(), strconv.Itoa(rand.Int()), "", 0xf, true)
+	verifyExpectedError(t, nil, err, errdefs.ErrNotFound)
+}
diff --git a/cmd/containerd-shim-runhcs-v1/service_internal.go b/cmd/containerd-shim-runhcs-v1/service_internal.go
@@ -205,17 +205,29 @@ func (s *service) startInternal(ctx context.Context, req *task.StartRequest) (*t
 }
 
 func (s *service) deleteInternal(ctx context.Context, req *task.DeleteRequest) (*task.DeleteResponse, error) {
-	// TODO: JTERRY75 we need to send this to the POD for isSandbox
-
 	t, err := s.getTask(req.ID)
 	if err != nil {
 		return nil, err
 	}
+
 	pid, exitStatus, exitedAt, err := t.DeleteExec(ctx, req.ExecID)
 	if err != nil {
 		return nil, err
 	}
-	// TODO: We should be removing the task after this right?
+
+	// 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)
+		}
+		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)
+		}
+	}
+
 	return &task.DeleteResponse{
 		Pid:        uint32(pid),
 		ExitStatus: exitStatus,