From 1301bb5f28839fa44e9a1cb089ebc4414c32dd71 Mon Sep 17 00:00:00 2001 From: Arunesh Dwivedi Date: Fri, 10 Jul 2026 12:55:14 +0000 Subject: [PATCH] fix: treat NotFound from pod delete in rolling update as success Signed-off-by: Arunesh Dwivedi --- controllers/events_test.go | 84 +++++++++++++++++++++++++- controllers/solr_pod_lifecycle_util.go | 15 ++++- 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/controllers/events_test.go b/controllers/events_test.go index f2cdb56f..d2a5d1ec 100644 --- a/controllers/events_test.go +++ b/controllers/events_test.go @@ -28,9 +28,11 @@ import ( "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" @@ -253,7 +255,87 @@ func TestHandlePvcExpansionEmitsEventOnBadMetadata(t *testing.T) { requireEvent(t, rec, corev1.EventTypeWarning, "PVCExpansionError") } -// TestReconcileSolrCloudBackupEmitsCloudNotReadyEvent verifies a BackupCloudNotReady warning is +// TestDeletePodForUpdateTreatsNotFoundAsSuccess verifies that when the target +// pod is already gone, DeletePodForUpdate does not report a PodUpdateError +// warning. A NotFound from the delete is the desired postcondition (the pod is +// absent and will be recreated), so it should surface a normal PodUpdate event. +func TestDeletePodForUpdateTreatsNotFoundAsSuccess(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("could not add client-go types to scheme: %v", err) + } + if err := solrv1beta1.AddToScheme(scheme); err != nil { + t.Fatalf("could not add solr types to scheme: %v", err) + } + + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test", + UID: "test-uid", + }, + } + + // Force the fake client's Delete to return NotFound, simulating a pod that + // was already removed by a prior reconcile. + notFoundClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.DeleteOption) error { + return apierrors.NewNotFound(schema.GroupResource{Resource: "pods"}, "test-pod") + }, + }). + Build() + + rec := record.NewFakeRecorder(8) + r := &SolrCloudReconciler{Client: notFoundClient, Recorder: rec} + instance := &solrv1beta1.SolrCloud{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test"}} + + if _, _, err := DeletePodForUpdate(context.Background(), r, instance, pod, false, logr.Discard()); err == nil { + t.Error("expected DeletePodForUpdate to return the NotFound delete error, got nil") + } + // A NotFound delete must surface a normal PodUpdate event, never a PodUpdateError warning. + requireEvent(t, rec, corev1.EventTypeNormal, "PodUpdate") +} + +// TestDeletePodForUpdateReportsRealDeleteError verifies that a non-NotFound +// delete failure is still surfaced as a PodUpdateError warning. +func TestDeletePodForUpdateReportsRealDeleteError(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("could not add client-go types to scheme: %v", err) + } + if err := solrv1beta1.AddToScheme(scheme); err != nil { + t.Fatalf("could not add solr types to scheme: %v", err) + } + + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test", + UID: "test-uid", + }, + } + + realErrClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.DeleteOption) error { + return errors.New("boom") + }, + }). + Build() + + rec := record.NewFakeRecorder(8) + r := &SolrCloudReconciler{Client: realErrClient, Recorder: rec} + instance := &solrv1beta1.SolrCloud{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "test"}} + + if _, _, err := DeletePodForUpdate(context.Background(), r, instance, pod, false, logr.Discard()); err == nil { + t.Error("expected DeletePodForUpdate to return the delete error, got nil") + } + requireEvent(t, rec, corev1.EventTypeWarning, "PodUpdateError") +} + // emitted when a backup is attempted against a repository that the SolrCloud has not yet marked // available. A GCS repository is used so that EnsureDirectoryForBackup is a no-op (no pod exec). func TestReconcileSolrCloudBackupEmitsCloudNotReadyEvent(t *testing.T) { diff --git a/controllers/solr_pod_lifecycle_util.go b/controllers/solr_pod_lifecycle_util.go index c794f07b..9815dbd2 100644 --- a/controllers/solr_pod_lifecycle_util.go +++ b/controllers/solr_pod_lifecycle_util.go @@ -23,6 +23,7 @@ import ( "github.com/apache/solr-operator/controllers/util" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "time" @@ -106,9 +107,17 @@ func DeletePodForUpdate(ctx context.Context, r *SolrCloudReconciler, instance *s UID: &pod.UID, }) if err != nil { - logger.Error(err, "Error while killing solr pod for update", "pod", pod.Name) - r.Recorder.Eventf(instance, corev1.EventTypeWarning, "PodUpdateError", - "Error while deleting pod %s for an update: %v", pod.Name, err) + // A NotFound error means the pod is already gone, which is the + // desired postcondition for this update path. Treat it as success + // rather than reporting a spurious PodUpdateError. + if apierrors.IsNotFound(err) { + r.Recorder.Eventf(instance, corev1.EventTypeNormal, "PodUpdate", + "Pod %s already deleted; it will be recreated with the updated SolrCloud specification", pod.Name) + } else { + logger.Error(err, "Error while killing solr pod for update", "pod", pod.Name) + r.Recorder.Eventf(instance, corev1.EventTypeWarning, "PodUpdateError", + "Error while deleting pod %s for an update: %v", pod.Name, err) + } } else { r.Recorder.Eventf(instance, corev1.EventTypeNormal, "PodUpdate", "Deleting pod %s so that it can be recreated with the updated SolrCloud specification", pod.Name)