Repository: virtual-kubelet/virtual-kubelet (go)

Issue:
Delete Pod Doesn't Handle Errors
Context From Slack:
```
smithx10 [9:17 PM]
I am testing deletepod,  and noticed that if I return an error to deletepod, virtual kubelete will show the pod deleted. (edited) 

It runs the delete  pod twice but thats it...

smithx10 [9:37 PM]
HmMmm looks like https://github.com/virtual-kubelet/virtual-kubelet/blob/master/vkubelet/pod.go#L95
vkubelet/pod.go:95

    if delErr = s.provider.DeletePod(ctx, pod); delErr != nil && errors.IsNotFound(delErr) {
virtual-kubelet/virtual-kubelet 
    Added by GitHub

I'm returning the following error to test this, "1.266s delErr=&errors.errorString{s:"not implemented by Triton provider"}"  but I guess  "&& errors.IsNotFound(delErr) "   isn't returning true,  so I never actually return an error for DeletePod and instead return nil
Do I need to return a particular Error to DeletePod?
```

```
cpuguy83 [11:41 AM]
That error check seems both backwards and not properly implemented.
Because we don’t expect providers to return k8s API errors.
And it should be `!IsNotFound` anyway.
```
```
smithx10 [11:45 AM]
@cpuguy83 yeah I couldn't really reason about the checks
```
```
cpuguy83 [11:46 AM]
So, I think this should change to `!strongerrors.IsNotFound(delErr)`
And if a provider wants to let us know that thing doesn’t exist they can return a `strongerrors.ErrNotFound`
```

Pull request:
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.

Fixes #521

Patch:
diff --git a/Makefile b/Makefile
index 64c4b992..32acd877 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 f685d2b9..bac2fad1 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
 }


Test patch:
diff --git a/node/pod_test.go b/node/pod_test.go
index 88fdde95..10153200 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)
+			}
+		})
+	}
+}