Merge pull request #136817 from kairosci/fix-gc-notfound-136525

Handle NotFound errors in garbage collector

pkg/controller/garbagecollector/garbagecollector.go

@@ -625,7 +625,12 @@ func (gc *GarbageCollector) attemptToDeleteItem(ctx context.Context, item *node)
 		// FinalizerDeletingDependents from the item, resulting in the final
 		// deletion of the item.
 		policy := metav1.DeletePropagationForeground
-		return gc.deleteObject(item.identity, latest.ResourceVersion, latest.OwnerReferences, &policy)
+		err := gc.deleteObject(item.identity, latest.ResourceVersion, latest.OwnerReferences, &policy)
+		if errors.IsNotFound(err) {
+			gc.dependencyGraphBuilder.enqueueVirtualDeleteEvent(item.identity)
+			return enqueuedVirtualDeleteEventErr
+		}
+		return err
 	default:
 		// item doesn't have any solid owner, so it needs to be garbage
 		// collected. Also, none of item's owners is waiting for the deletion of
@@ -646,7 +651,12 @@ func (gc *GarbageCollector) attemptToDeleteItem(ctx context.Context, item *node)
 			"item", item.identity,
 			"propagationPolicy", policy,
 		)
-		return gc.deleteObject(item.identity, latest.ResourceVersion, latest.OwnerReferences, &policy)
+		err := gc.deleteObject(item.identity, latest.ResourceVersion, latest.OwnerReferences, &policy)
+		if errors.IsNotFound(err) {
+			gc.dependencyGraphBuilder.enqueueVirtualDeleteEvent(item.identity)
+			return enqueuedVirtualDeleteEventErr
+		}
+		return err
 	}
 }
 

pkg/controller/garbagecollector/garbagecollector_test.go

@@ -18,6 +18,7 @@ package garbagecollector
 
 import (
 	"context"
+	goerrors "errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -255,6 +256,126 @@ func serilizeOrDie(t *testing.T, object interface{}) []byte {
 	return data
 }
 
+func TestAttemptToDeleteItemDeleteObjectNotFound(t *testing.T) {
+	pod := getPod("ExternallyDeletedPod", []metav1.OwnerReference{
+		{
+			Kind:       "ReplicationController",
+			Name:       "owner1",
+			UID:        "123",
+			APIVersion: "v1",
+		},
+	})
+	testHandler := &fakeActionHandler{
+		response: map[string]FakeResponse{
+			"GET" + "/api/v1/namespaces/ns1/replicationcontrollers/owner1": {
+				404,
+				[]byte{},
+			},
+			"GET" + "/api/v1/namespaces/ns1/pods/ExternallyDeletedPod": {
+				200,
+				serilizeOrDie(t, pod),
+			},
+			"DELETE" + "/api/v1/namespaces/ns1/pods/ExternallyDeletedPod": {
+				404,
+				[]byte{},
+			},
+		},
+	}
+	srv, clientConfig := testServerAndClientConfig(testHandler.ServeHTTP)
+	defer srv.Close()
+
+	gc := setupGC(t, clientConfig)
+	defer close(gc.stop)
+
+	item := &node{
+		identity: objectReference{
+			OwnerReference: metav1.OwnerReference{
+				Kind:       pod.Kind,
+				APIVersion: pod.APIVersion,
+				Name:       pod.Name,
+				UID:        pod.UID,
+			},
+			Namespace: pod.Namespace,
+		},
+		owners: nil,
+	}
+
+	err := gc.attemptToDeleteItem(context.TODO(), item)
+	if !goerrors.Is(err, enqueuedVirtualDeleteEventErr) {
+		t.Errorf("expected enqueuedVirtualDeleteEventErr, got: %v", err)
+	}
+	if gc.dependencyGraphBuilder.graphChanges.Len() == 0 {
+		t.Errorf("expected a virtual delete event to be enqueued in graphChanges, but the queue is empty")
+	}
+}
+
+func TestAttemptToDeleteItemDeleteObjectNotFoundWaitingForDependents(t *testing.T) {
+	pod := getPod("ExternallyDeletedPodFG", []metav1.OwnerReference{
+		{
+			Kind:               "ReplicationController",
+			Name:               "owner1",
+			UID:                "123",
+			APIVersion:         "v1",
+			BlockOwnerDeletion: func() *bool { b := true; return &b }(),
+		},
+	})
+	owner := &v1.ReplicationController{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "ReplicationController",
+			APIVersion: "v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              "owner1",
+			Namespace:         "ns1",
+			UID:               "123",
+			DeletionTimestamp: func() *metav1.Time { t := metav1.Now(); return &t }(),
+			Finalizers:        []string{metav1.FinalizerDeleteDependents},
+		},
+	}
+	testHandler := &fakeActionHandler{
+		response: map[string]FakeResponse{
+			"GET" + "/api/v1/namespaces/ns1/replicationcontrollers/owner1": {
+				200,
+				serilizeOrDie(t, owner),
+			},
+			"GET" + "/api/v1/namespaces/ns1/pods/ExternallyDeletedPodFG": {
+				200,
+				serilizeOrDie(t, pod),
+			},
+			"DELETE" + "/api/v1/namespaces/ns1/pods/ExternallyDeletedPodFG": {
+				404,
+				[]byte{},
+			},
+		},
+	}
+	srv, clientConfig := testServerAndClientConfig(testHandler.ServeHTTP)
+	defer srv.Close()
+
+	gc := setupGC(t, clientConfig)
+	defer close(gc.stop)
+
+	item := &node{
+		identity: objectReference{
+			OwnerReference: metav1.OwnerReference{
+				Kind:       pod.Kind,
+				APIVersion: pod.APIVersion,
+				Name:       pod.Name,
+				UID:        pod.UID,
+			},
+			Namespace: pod.Namespace,
+		},
+		owners: nil,
+	}
+
+	err := gc.attemptToDeleteItem(context.TODO(), item)
+	if !goerrors.Is(err, enqueuedVirtualDeleteEventErr) {
+		t.Errorf("expected enqueuedVirtualDeleteEventErr, got: %v", err)
+	}
+	if gc.dependencyGraphBuilder.graphChanges.Len() == 0 {
+		t.Errorf("expected a virtual delete event to be enqueued in graphChanges, but the queue is empty")
+	}
+}
+
 // test the attemptToDeleteItem function making the expected actions.
 func TestAttemptToDeleteItem(t *testing.T) {
 	pod := getPod("ToBeDeletedPod", []metav1.OwnerReference{

pkg/controller/garbagecollector/operations.go

@@ -67,8 +67,8 @@ func (gc *GarbageCollector) deleteObject(item objectReference, resourceVersion s
 		// check if the ownerReferences changed
 		liveObject, liveErr := resourceClient.Get(context.TODO(), item.Name, metav1.GetOptions{})
 		if errors.IsNotFound(liveErr) {
-			// object we wanted to delete is gone, success!
-			return nil
+			// object we wanted to delete is gone, return NotFound so caller can handle it consistently
+			return liveErr
 		}
 		if liveErr == nil && liveObject.UID == item.UID && liveObject.ResourceVersion != resourceVersion && reflect.DeepEqual(liveObject.OwnerReferences, ownersAtResourceVersion) {
 			// object changed, causing a conflict error, but ownerReferences did not change.

test/e2e/apimachinery/etcd_failure.go

@@ -115,10 +115,6 @@ func masterExec(ctx context.Context, f *framework.Framework, cmd string) {
 
 	host := ips[0] + ":22"
 	result, err := e2essh.SSH(ctx, cmd, host, framework.TestContext.Provider)
-	framework.ExpectNoError(err)
-	e2essh.LogResult(result)
-
-	result, err = e2essh.SSH(ctx, cmd, host, framework.TestContext.Provider)
 	framework.ExpectNoError(err, "failed to SSH to host %s on provider %s and run command: %q", host, framework.TestContext.Provider, cmd)
 	if result.Code != 0 {
 		e2essh.LogResult(result)