From b915cde1ae53e20a55f1ab4af7d9b29de3ce3378 Mon Sep 17 00:00:00 2001
From: Brian Goff <cpuguy83@gmail.com>
Date: Sat, 29 Jun 2019 00:07:24 -0700
Subject: [PATCH] Fix error handling for delete pod (#685)

* Fix error handling for delete pod

- Error handling was looking for a k8s error from the provider, but
  providers should be using errdefs.
- Error handling was returning early if pod was not found and deleting
  from k8s in all other cases.

* Don't run unit tests twice
---
 Makefile         |  6 ++---
 node/pod.go      | 12 ++++-----
 node/pod_test.go | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index 64c4b992a..32acd8776 100644
--- a/Makefile
+++ b/Makefile
@@ -83,11 +83,9 @@ else
 endif
 
 test:
-	@echo "Testing..."
-	$Q go test -mod=vendor $(if $V,-v) $(allpackages) # install -race libs to speed up next run
 ifndef CI
-	@echo "Testing Outside CI..."
-	$Q GODEBUG=cgocheck=2 go test -mod=vendor $(allpackages)
+	@echo "Testing..."
+	$Q go test -mod=vendor $(if $V,-v) $(allpackages)
 else
 	@echo "Testing in CI..."
 	$Q mkdir -p test
diff --git a/node/pod.go b/node/pod.go
index f685d2b91..bac2fad15 100644
--- a/node/pod.go
+++ b/node/pod.go
@@ -145,20 +145,18 @@ func (pc *PodController) deletePod(ctx context.Context, namespace, name string)
 	ctx = addPodAttributes(ctx, span, pod)
 
 	var delErr error
-	if delErr = pc.provider.DeletePod(ctx, pod); delErr != nil && errors.IsNotFound(delErr) {
+	if delErr = pc.provider.DeletePod(ctx, pod); delErr != nil && !errdefs.IsNotFound(delErr) {
 		span.SetStatus(delErr)
 		return delErr
 	}
 
 	log.G(ctx).Debug("Deleted pod from provider")
 
-	if !errors.IsNotFound(delErr) {
-		if err := pc.forceDeletePodResource(ctx, namespace, name); err != nil {
-			span.SetStatus(err)
-			return err
-		}
-		log.G(ctx).Info("Deleted pod from Kubernetes")
+	if err := pc.forceDeletePodResource(ctx, namespace, name); err != nil {
+		span.SetStatus(err)
+		return err
 	}
+	log.G(ctx).Info("Deleted pod from Kubernetes")
 
 	return nil
 }
diff --git a/node/pod_test.go b/node/pod_test.go
index 88fdde952..101532006 100644
--- a/node/pod_test.go
+++ b/node/pod_test.go
@@ -19,11 +19,14 @@ import (
 	"path"
 	"testing"
 
+	pkgerrors "github.com/pkg/errors"
 	"github.com/virtual-kubelet/virtual-kubelet/errdefs"
 	testutil "github.com/virtual-kubelet/virtual-kubelet/internal/test/util"
 	"gotest.tools/assert"
 	is "gotest.tools/assert/cmp"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
 )
 
@@ -33,6 +36,8 @@ type mockProvider struct {
 	creates int
 	updates int
 	deletes int
+
+	errorOnDelete error
 }
 
 func (m *mockProvider) CreatePod(ctx context.Context, pod *corev1.Pod) error {
@@ -64,6 +69,9 @@ func (m *mockProvider) GetPodStatus(ctx context.Context, namespace, name string)
 }
 
 func (m *mockProvider) DeletePod(ctx context.Context, p *corev1.Pod) error {
+	if m.errorOnDelete != nil {
+		return m.errorOnDelete
+	}
 	delete(m.pods, path.Join(p.GetNamespace(), p.GetName()))
 	m.deletes++
 	return nil
@@ -293,3 +301,63 @@ func TestPodNoSpecChange(t *testing.T) {
 	assert.Check(t, is.Equal(svr.mock.creates, 1))
 	assert.Check(t, is.Equal(svr.mock.updates, 0))
 }
+
+func TestPodDelete(t *testing.T) {
+	type testCase struct {
+		desc   string
+		delErr error
+	}
+
+	cases := []testCase{
+		{desc: "no error on delete", delErr: nil},
+		{desc: "not found error on delete", delErr: errdefs.NotFound("not found")},
+		{desc: "unknown error on delete", delErr: pkgerrors.New("random error")},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.desc, func(t *testing.T) {
+			c := newTestController()
+			c.mock.errorOnDelete = tc.delErr
+
+			pod := &corev1.Pod{}
+			pod.ObjectMeta.Namespace = "default"
+			pod.ObjectMeta.Name = "nginx"
+			pod.Spec = corev1.PodSpec{
+				Containers: []corev1.Container{
+					corev1.Container{
+						Name:  "nginx",
+						Image: "nginx:1.15.12",
+					},
+				},
+			}
+
+			pc := c.client.CoreV1().Pods("default")
+
+			p, err := pc.Create(pod)
+			assert.NilError(t, err)
+
+			ctx := context.Background()
+			err = c.createOrUpdatePod(ctx, p) // make sure it's actually created
+			assert.NilError(t, err)
+			assert.Check(t, is.Equal(c.mock.creates, 1))
+
+			err = c.deletePod(ctx, pod.Namespace, pod.Name)
+			assert.Equal(t, pkgerrors.Cause(err), err)
+
+			var expectDeletes int
+			if tc.delErr == nil {
+				expectDeletes = 1
+			}
+			assert.Check(t, is.Equal(c.mock.deletes, expectDeletes))
+
+			expectDeleted := tc.delErr == nil || errdefs.IsNotFound(tc.delErr)
+
+			_, err = pc.Get(pod.Name, metav1.GetOptions{})
+			if expectDeleted {
+				assert.Assert(t, errors.IsNotFound(err))
+			} else {
+				assert.NilError(t, err)
+			}
+		})
+	}
+}