From ae3163eecae0087192274445a04c5aeccdbbb7b8 Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Sat, 7 Feb 2026 13:46:43 +0100 Subject: [PATCH 1/3] Fix issue 136525: Handle NotFound errors in garbage collector When objects are deleted externally (e.g., pods deleted directly after job deletion), the garbage collector should not log errors. This change adds explicit NotFound error handling in attemptToDeleteItem to enqueue virtual delete events when deleteObject returns NotFound, treating external deletion as a successful outcome. This follows the same pattern already used when getObject returns NotFound, ensuring consistency across the function. --- .../garbagecollector/garbagecollector.go | 16 ++++++++++++++-- pkg/controller/garbagecollector/operations.go | 4 ++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/pkg/controller/garbagecollector/garbagecollector.go b/pkg/controller/garbagecollector/garbagecollector.go index 24044f8706d..643c0b1476f 100644 --- a/pkg/controller/garbagecollector/garbagecollector.go +++ b/pkg/controller/garbagecollector/garbagecollector.go @@ -625,7 +625,13 @@ 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) { + // the item was deleted externally, enqueue a virtual delete event + 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 +652,13 @@ 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) { + // the item was deleted externally, enqueue a virtual delete event + gc.dependencyGraphBuilder.enqueueVirtualDeleteEvent(item.identity) + return enqueuedVirtualDeleteEventErr + } + return err } } diff --git a/pkg/controller/garbagecollector/operations.go b/pkg/controller/garbagecollector/operations.go index a3000252728..40acfdef578 100644 --- a/pkg/controller/garbagecollector/operations.go +++ b/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. From 4810a2aae7b352559194f6ff19aa908de4df99c2 Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Thu, 12 Feb 2026 19:39:23 +0100 Subject: [PATCH 2/3] fix: remove duplicate SSH execution in masterExec function --- test/e2e/apimachinery/etcd_failure.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/e2e/apimachinery/etcd_failure.go b/test/e2e/apimachinery/etcd_failure.go index 5d0b39b13cf..8c26abcc9da 100644 --- a/test/e2e/apimachinery/etcd_failure.go +++ b/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) From 8ed40e7ae71a3484b969f5de5d83e44594606b32 Mon Sep 17 00:00:00 2001 From: Alessio Attilio Date: Tue, 10 Mar 2026 19:11:31 +0100 Subject: [PATCH 3/3] test: add unit tests for deleteObject NotFound handling in garbage collector When deleteObject returns a NotFound error (the object was externally deleted between the GET and the DELETE), attemptToDeleteItem should enqueue a virtual delete event and return enqueuedVirtualDeleteEventErr. Cover both code paths: - default (background propagation): item with dangling owner - waitingForDependentsDeletion: item whose owner is foreground-deleting --- .../garbagecollector/garbagecollector.go | 2 - .../garbagecollector/garbagecollector_test.go | 121 ++++++++++++++++++ 2 files changed, 121 insertions(+), 2 deletions(-) diff --git a/pkg/controller/garbagecollector/garbagecollector.go b/pkg/controller/garbagecollector/garbagecollector.go index 643c0b1476f..f43578073e8 100644 --- a/pkg/controller/garbagecollector/garbagecollector.go +++ b/pkg/controller/garbagecollector/garbagecollector.go @@ -627,7 +627,6 @@ func (gc *GarbageCollector) attemptToDeleteItem(ctx context.Context, item *node) policy := metav1.DeletePropagationForeground err := gc.deleteObject(item.identity, latest.ResourceVersion, latest.OwnerReferences, &policy) if errors.IsNotFound(err) { - // the item was deleted externally, enqueue a virtual delete event gc.dependencyGraphBuilder.enqueueVirtualDeleteEvent(item.identity) return enqueuedVirtualDeleteEventErr } @@ -654,7 +653,6 @@ func (gc *GarbageCollector) attemptToDeleteItem(ctx context.Context, item *node) ) err := gc.deleteObject(item.identity, latest.ResourceVersion, latest.OwnerReferences, &policy) if errors.IsNotFound(err) { - // the item was deleted externally, enqueue a virtual delete event gc.dependencyGraphBuilder.enqueueVirtualDeleteEvent(item.identity) return enqueuedVirtualDeleteEventErr } diff --git a/pkg/controller/garbagecollector/garbagecollector_test.go b/pkg/controller/garbagecollector/garbagecollector_test.go index 8663e6c24a8..2b164fd17cf 100644 --- a/pkg/controller/garbagecollector/garbagecollector_test.go +++ b/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{