From 2361ec93bdf16649c92c246b5675722c3d97acc7 Mon Sep 17 00:00:00 2001 From: Katarina Strenkova Date: Thu, 16 Jul 2026 08:38:02 -0400 Subject: [PATCH] Release test-operator-lock on CR deletion Previously, deleting a CR while its pod held the test-operator-lock left the lock stuck, blocking all other CR instances indefinitely. Finalizers are now added unconditionally so the lock is always released during deletion. --- internal/controller/ansibletest_controller.go | 1 - internal/controller/common_controller.go | 13 ++++++++----- internal/controller/horizontest_controller.go | 1 - internal/controller/tempest_controller.go | 1 - internal/controller/tobiko_controller.go | 1 - test/functional/ansibletest_controller_test.go | 6 ++++++ test/functional/horizontest_controller_test.go | 6 ++++++ test/functional/tempest_controller_test.go | 2 -- test/functional/tobiko_controller_test.go | 5 +++++ 9 files changed, 25 insertions(+), 11 deletions(-) diff --git a/internal/controller/ansibletest_controller.go b/internal/controller/ansibletest_controller.go index 67e9c80c..5762ef77 100644 --- a/internal/controller/ansibletest_controller.go +++ b/internal/controller/ansibletest_controller.go @@ -58,7 +58,6 @@ func (r *AnsibleTestReconciler) Reconcile(ctx context.Context, req ctrl.Request) ServiceName: ansibletest.ServiceName, NeedsNetworkAttachments: false, NeedsConfigMaps: false, - NeedsFinalizer: false, SupportsWorkflow: true, BuildPod: func(ctx context.Context, instance *testv1beta1.AnsibleTest, labels, annotations map[string]string, workflowStepIndex int, pvcIndex int) (*corev1.Pod, error) { diff --git a/internal/controller/common_controller.go b/internal/controller/common_controller.go index 41718f97..fc5ed338 100644 --- a/internal/controller/common_controller.go +++ b/internal/controller/common_controller.go @@ -51,9 +51,6 @@ type TestResourceConfig[T TestResource] struct { // NeedsConfigMaps indicates if ServiceConfigReadyCondition is needed NeedsConfigMaps bool - // NeedsFinalizer indicates if the controller needs finalizer handling - NeedsFinalizer bool - // SupportsWorkflow indicates if the controller supports workflow feature SupportsWorkflow bool @@ -154,14 +151,20 @@ func CommonReconcile[T TestResource]( // If we're not deleting this and the service object doesn't have our // finalizer, add it. - if config.NeedsFinalizer && instance.GetDeletionTimestamp().IsZero() && + if instance.GetDeletionTimestamp().IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) { return ctrl.Result{}, nil } // Handle service delete - if config.NeedsFinalizer && !instance.GetDeletionTimestamp().IsZero() { + if !instance.GetDeletionTimestamp().IsZero() { Log.Info("Reconciling Service delete") + + if lockReleased, err := r.ReleaseLock(ctx, instance); !lockReleased { + Log.Info(fmt.Sprintf(InfoCanNotReleaseLock, testOperatorLockName)) + return ctrl.Result{RequeueAfter: RequeueAfterValue}, err + } + controllerutil.RemoveFinalizer(instance, helper.GetFinalizer()) Log.Info("Reconciled Service delete successfully") return ctrl.Result{}, nil diff --git a/internal/controller/horizontest_controller.go b/internal/controller/horizontest_controller.go index 8cf48df9..fb1110a9 100644 --- a/internal/controller/horizontest_controller.go +++ b/internal/controller/horizontest_controller.go @@ -58,7 +58,6 @@ func (r *HorizonTestReconciler) Reconcile(ctx context.Context, req ctrl.Request) ServiceName: horizontest.ServiceName, NeedsNetworkAttachments: false, NeedsConfigMaps: true, - NeedsFinalizer: false, SupportsWorkflow: false, GenerateServiceConfigMaps: func(ctx context.Context, helper *helper.Helper, labels map[string]string, instance *testv1beta1.HorizonTest, _ int) error { diff --git a/internal/controller/tempest_controller.go b/internal/controller/tempest_controller.go index 0d70e4a6..bd1e34f2 100644 --- a/internal/controller/tempest_controller.go +++ b/internal/controller/tempest_controller.go @@ -61,7 +61,6 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct ServiceName: tempest.ServiceName, NeedsNetworkAttachments: true, NeedsConfigMaps: true, - NeedsFinalizer: true, SupportsWorkflow: true, GenerateServiceConfigMaps: func(ctx context.Context, helper *helper.Helper, _ map[string]string, instance *testv1beta1.Tempest, workflowStep int) error { diff --git a/internal/controller/tobiko_controller.go b/internal/controller/tobiko_controller.go index 5ca071c4..ee97de53 100644 --- a/internal/controller/tobiko_controller.go +++ b/internal/controller/tobiko_controller.go @@ -61,7 +61,6 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr ServiceName: tobiko.ServiceName, NeedsNetworkAttachments: true, NeedsConfigMaps: true, - NeedsFinalizer: false, SupportsWorkflow: true, GenerateServiceConfigMaps: func(ctx context.Context, helper *helper.Helper, labels map[string]string, instance *testv1beta1.Tobiko, workflowStepIndex int) error { diff --git a/test/functional/ansibletest_controller_test.go b/test/functional/ansibletest_controller_test.go index 83cbaf58..0c6cbca6 100644 --- a/test/functional/ansibletest_controller_test.go +++ b/test/functional/ansibletest_controller_test.go @@ -81,6 +81,12 @@ var _ = Describe("AnsibleTest controller", func() { Expect(ansibleTest.Spec.AnsibleGitRepo).ShouldNot(BeEmpty()) Expect(ansibleTest.Spec.AnsiblePlaybookPath).ShouldNot(BeEmpty()) }) + + It("should have a finalizer", func() { + Eventually(func() []string { + return GetAnsibleTest(ansibleTestName).Finalizers + }, timeout, interval).Should(ContainElement("openstack.org/ansibletest")) + }) }) When("All dependencies are ready", func() { diff --git a/test/functional/horizontest_controller_test.go b/test/functional/horizontest_controller_test.go index 4b94d433..573b84e9 100644 --- a/test/functional/horizontest_controller_test.go +++ b/test/functional/horizontest_controller_test.go @@ -83,6 +83,12 @@ var _ = Describe("HorizonTest controller", func() { Expect(horizonTest.Spec.DashboardUrl).ShouldNot(BeEmpty()) Expect(horizonTest.Spec.AuthUrl).ShouldNot(BeEmpty()) }) + + It("should have a finalizer", func() { + Eventually(func() []string { + return GetHorizonTest(horizonTestName).Finalizers + }, timeout, interval).Should(ContainElement("openstack.org/horizontest")) + }) }) When("All dependencies are ready", func() { diff --git a/test/functional/tempest_controller_test.go b/test/functional/tempest_controller_test.go index d0ee0533..db467c57 100644 --- a/test/functional/tempest_controller_test.go +++ b/test/functional/tempest_controller_test.go @@ -86,8 +86,6 @@ var _ = Describe("Tempest controller", func() { }) It("should have a finalizer", func() { - // the reconciler loop adds the finalizer so we have to wait for - // it to run Eventually(func() []string { return GetTempest(tempestName).Finalizers }, timeout, interval).Should(ContainElement("openstack.org/tempest")) diff --git a/test/functional/tobiko_controller_test.go b/test/functional/tobiko_controller_test.go index 08148082..6138b01e 100644 --- a/test/functional/tobiko_controller_test.go +++ b/test/functional/tobiko_controller_test.go @@ -82,6 +82,11 @@ var _ = Describe("Tobiko controller", func() { Expect(tobiko.Spec.Testenv).Should(Equal("sanity")) }) + It("should have a finalizer", func() { + Eventually(func() []string { + return GetTobiko(tobikoName).Finalizers + }, timeout, interval).Should(ContainElement("openstack.org/tobiko")) + }) }) When("All dependencies are ready", func() {