From a5c7b4640f5e0cdc130d5f6a2302763b170a85bf Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Wed, 6 Nov 2024 18:17:50 +0000 Subject: [PATCH] Improve logic for cleaning up of service resources Leaked service instances and bindings have caused some problems to CI clusters, since they have finalizers and prevent the namespaces in which they are created to go away. Resolving this problem requres manual intervention. This commit makes the logic retry until no binding or instances are left for each service plan of the related broker --- tests/e2e/e2e_suite_test.go | 7 -- tests/e2e/service_bindings_test.go | 24 +++--- tests/e2e/service_brokers_test.go | 13 +-- tests/e2e/service_instances_test.go | 23 ++++-- tests/e2e/service_offerings_test.go | 3 +- tests/e2e/service_plans_test.go | 3 +- tests/helpers/broker/deleter.go | 120 +++++++++++++++++++--------- tests/smoke/catalog_test.go | 3 +- tests/smoke/services_test.go | 3 +- tests/smoke/suite_test.go | 7 -- 10 files changed, 126 insertions(+), 80 deletions(-) diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 5b98c5769..46cf93a60 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -16,7 +16,6 @@ import ( "time" "code.cloudfoundry.org/korifi/tests/helpers" - "code.cloudfoundry.org/korifi/tests/helpers/broker" "code.cloudfoundry.org/korifi/tests/helpers/fail_handler" "code.cloudfoundry.org/go-loggregator/v8/rpc/loggregator_v2" @@ -1229,12 +1228,6 @@ func createBroker(brokerURL string) string { return brokerGUID } -func cleanupBroker(brokerGUID string) { - GinkgoHelper() - - broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() -} - func expectJobCompletes(resp *resty.Response) { GinkgoHelper() diff --git a/tests/e2e/service_bindings_test.go b/tests/e2e/service_bindings_test.go index a0c5ea3ea..afd25c84f 100644 --- a/tests/e2e/service_bindings_test.go +++ b/tests/e2e/service_bindings_test.go @@ -3,6 +3,7 @@ package e2e_test import ( "net/http" + "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/go-resty/resty/v2" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -54,15 +55,17 @@ var _ = Describe("Service Bindings", func() { }) When("binding to a managed service instance", func() { - BeforeEach(func() { - brokerGUID := createBroker(serviceBrokerURL) - DeferCleanup(func() { - cleanupBroker(brokerGUID) - }) + var brokerGUID string + BeforeEach(func() { + brokerGUID = createBroker(serviceBrokerURL) instanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID) }) + AfterEach(func() { + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() + }) + It("succeeds with a job redirect", func() { Expect(httpError).NotTo(HaveOccurred()) Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted)) @@ -112,16 +115,19 @@ var _ = Describe("Service Bindings", func() { }) When("bound to a managed service instance", func() { + var brokerGUID string + BeforeEach(func() { - brokerGUID := createBroker(serviceBrokerURL) - DeferCleanup(func() { - cleanupBroker(brokerGUID) - }) + brokerGUID = createBroker(serviceBrokerURL) instanceGUID := createManagedServiceInstance(brokerGUID, spaceGUID) bindingGUID = createManagedServiceBinding(appGUID, instanceGUID, "") }) + AfterEach(func() { + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() + }) + It("succeeds with a job redirect", func() { Expect(httpError).NotTo(HaveOccurred()) Expect(httpResp).To(SatisfyAll( diff --git a/tests/e2e/service_brokers_test.go b/tests/e2e/service_brokers_test.go index 279eef817..2db084a1c 100644 --- a/tests/e2e/service_brokers_test.go +++ b/tests/e2e/service_brokers_test.go @@ -5,6 +5,7 @@ import ( "strings" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/go-resty/resty/v2" "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" @@ -50,7 +51,7 @@ var _ = Describe("Service Brokers", func() { jobURLSplit := strings.Split(jobURL, "~") Expect(jobURLSplit).To(HaveLen(2)) DeferCleanup(func() { - cleanupBroker(jobURLSplit[1]) + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(jobURLSplit[1]).Delete() }) }) }) @@ -63,10 +64,10 @@ var _ = Describe("Service Brokers", func() { BeforeEach(func() { brokerGUID = createBroker(serviceBrokerURL) + }) - DeferCleanup(func() { - cleanupBroker(brokerGUID) - }) + AfterEach(func() { + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() }) JustBeforeEach(func() { @@ -97,7 +98,7 @@ var _ = Describe("Service Brokers", func() { }) AfterEach(func() { - cleanupBroker(brokerGUID) + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() }) JustBeforeEach(func() { @@ -140,7 +141,7 @@ var _ = Describe("Service Brokers", func() { }) AfterEach(func() { - cleanupBroker(brokerGUID) + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() }) JustBeforeEach(func() { diff --git a/tests/e2e/service_instances_test.go b/tests/e2e/service_instances_test.go index 4dd607e7b..bb2a6ad43 100644 --- a/tests/e2e/service_instances_test.go +++ b/tests/e2e/service_instances_test.go @@ -4,6 +4,7 @@ import ( "net/http" "strings" + "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/go-resty/resty/v2" "github.com/google/uuid" . "github.com/onsi/ginkgo/v2" @@ -78,11 +79,10 @@ var _ = Describe("Service Instances", func() { }) When("creating a managed service instance", func() { + var brokerGUID string + BeforeEach(func() { - brokerGUID := createBroker(serviceBrokerURL) - DeferCleanup(func() { - cleanupBroker(brokerGUID) - }) + brokerGUID = createBroker(serviceBrokerURL) var plansResp resourceList[resource] catalogResp, err := adminClient.R().SetResult(&plansResp).Get("/v3/service_plans?service_broker_guids=" + brokerGUID) @@ -110,6 +110,10 @@ var _ = Describe("Service Instances", func() { } }) + AfterEach(func() { + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() + }) + It("succeeds with a job redirect", func() { Expect(httpError).NotTo(HaveOccurred()) Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted)) @@ -181,15 +185,18 @@ var _ = Describe("Service Instances", func() { }) When("deleting a managed service instance", func() { + var brokerGUID string + BeforeEach(func() { - brokerGUID := createBroker(serviceBrokerURL) - DeferCleanup(func() { - cleanupBroker(brokerGUID) - }) + brokerGUID = createBroker(serviceBrokerURL) serviceInstanceGUID = createManagedServiceInstance(brokerGUID, spaceGUID) }) + AfterEach(func() { + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() + }) + It("succeeds with a job redirect", func() { Expect(httpError).NotTo(HaveOccurred()) Expect(httpResp).To(HaveRestyStatusCode(http.StatusAccepted)) diff --git a/tests/e2e/service_offerings_test.go b/tests/e2e/service_offerings_test.go index 6089159e2..bceac25fc 100644 --- a/tests/e2e/service_offerings_test.go +++ b/tests/e2e/service_offerings_test.go @@ -3,6 +3,7 @@ package e2e_test import ( "net/http" + "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/go-resty/resty/v2" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -20,7 +21,7 @@ var _ = Describe("Service Offerings", func() { }) AfterEach(func() { - cleanupBroker(brokerGUID) + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() }) Describe("List", func() { diff --git a/tests/e2e/service_plans_test.go b/tests/e2e/service_plans_test.go index 96ae2f93d..3e156125f 100644 --- a/tests/e2e/service_plans_test.go +++ b/tests/e2e/service_plans_test.go @@ -5,6 +5,7 @@ import ( "net/http" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" + "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/BooleanCat/go-functional/v2/it/itx" "github.com/go-resty/resty/v2" . "github.com/onsi/ginkgo/v2" @@ -23,7 +24,7 @@ var _ = Describe("Service Plans", func() { }) AfterEach(func() { - cleanupBroker(brokerGUID) + broker.NewCatalogDeleter(rootNamespace).ForBrokerGUID(brokerGUID).Delete() }) Describe("List", func() { diff --git a/tests/helpers/broker/deleter.go b/tests/helpers/broker/deleter.go index 18ffb94ef..fc4e6e9d9 100644 --- a/tests/helpers/broker/deleter.go +++ b/tests/helpers/broker/deleter.go @@ -2,9 +2,12 @@ package broker import ( "context" + "fmt" + "time" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/tools/k8s" + "github.com/BooleanCat/go-functional/v2/it/itx" . "github.com/onsi/ginkgo/v2" //lint:ignore ST1001 this is a test file . "github.com/onsi/gomega" //lint:ignore ST1001 this is a test file @@ -15,12 +18,28 @@ import ( ) type CatalogDeleter struct { + ctx context.Context + k8sClient client.Client rootNamespace string catalogLabelSelector client.MatchingLabels + maxRetries int } func NewCatalogDeleter(rootNamespace string) *CatalogDeleter { - return &CatalogDeleter{rootNamespace: rootNamespace} + ctx := context.Background() + + config, err := controllerruntime.GetConfig() + Expect(err).NotTo(HaveOccurred()) + + k8sClient, err := client.New(config, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + + return &CatalogDeleter{ + ctx: ctx, + k8sClient: k8sClient, + rootNamespace: rootNamespace, + maxRetries: 50, + } } func (d *CatalogDeleter) ForBrokerGUID(brokerGUID string) *CatalogDeleter { @@ -42,42 +61,12 @@ func (d *CatalogDeleter) ForBrokerName(brokerName string) *CatalogDeleter { func (d *CatalogDeleter) Delete() { GinkgoHelper() - ctx := context.Background() - - config, err := controllerruntime.GetConfig() - Expect(err).NotTo(HaveOccurred()) - - k8sClient, err := client.New(config, client.Options{Scheme: scheme.Scheme}) - Expect(err).NotTo(HaveOccurred()) - servicePlans := &korifiv1alpha1.CFServicePlanList{} - Expect(k8sClient.List(ctx, servicePlans, client.InNamespace(d.rootNamespace), d.catalogLabelSelector)).To(Succeed()) + Expect(d.k8sClient.List(d.ctx, servicePlans, client.InNamespace(d.rootNamespace), d.catalogLabelSelector)).To(Succeed()) for _, plan := range servicePlans.Items { - serviceBindings := &korifiv1alpha1.CFServiceBindingList{} - Expect(k8sClient.List(ctx, serviceBindings, client.MatchingLabels{ - korifiv1alpha1.PlanGUIDLabelKey: plan.Name, - })).To(Succeed()) - for _, binding := range serviceBindings.Items { - Expect(k8s.PatchResource(ctx, k8sClient, &binding, func() { - binding.Finalizers = nil - })).To(Succeed()) - - Expect(k8sClient.Delete(ctx, &binding)).To(Succeed()) - } - - serviceInstances := &korifiv1alpha1.CFServiceInstanceList{} - Expect(k8sClient.List(ctx, serviceInstances)).To(Succeed()) - for _, instance := range serviceInstances.Items { - if instance.Spec.PlanGUID != plan.Name { - continue - } - Expect(k8s.Patch(ctx, k8sClient, &instance, func() { - instance.Finalizers = nil - })).To(Succeed()) - Expect(k8sClient.Delete(ctx, &instance)).To(Succeed()) - } - - Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, &korifiv1alpha1.CFServiceBroker{ + Expect(d.cleanupBindings(plan.Name)).To(Succeed()) + Expect(d.cleanupInsances(plan.Name)).To(Succeed()) + Expect(client.IgnoreNotFound(d.k8sClient.Delete(d.ctx, &korifiv1alpha1.CFServiceBroker{ ObjectMeta: metav1.ObjectMeta{ Namespace: d.rootNamespace, Name: plan.Labels[korifiv1alpha1.RelServiceBrokerGUIDLabel], @@ -85,17 +74,70 @@ func (d *CatalogDeleter) Delete() { }))).To(Succeed()) } - Expect(k8sClient.DeleteAllOf( - ctx, + Expect(d.k8sClient.DeleteAllOf( + d.ctx, &korifiv1alpha1.CFServiceOffering{}, client.InNamespace(d.rootNamespace), d.catalogLabelSelector, )).To(Succeed()) - Expect(k8sClient.DeleteAllOf( - ctx, + Expect(d.k8sClient.DeleteAllOf( + d.ctx, &korifiv1alpha1.CFServicePlan{}, client.InNamespace(d.rootNamespace), d.catalogLabelSelector, )).To(Succeed()) } + +func (d *CatalogDeleter) cleanupBindings(planName string) error { + for retries := 0; retries < d.maxRetries; retries++ { + serviceBindings := &korifiv1alpha1.CFServiceBindingList{} + Expect(d.k8sClient.List(d.ctx, serviceBindings, client.MatchingLabels{ + korifiv1alpha1.PlanGUIDLabelKey: planName, + })).To(Succeed()) + + if len(serviceBindings.Items) == 0 { + return nil + } + + for _, binding := range serviceBindings.Items { + forceDelete(d.ctx, d.k8sClient, &binding) + } + + time.Sleep(100 * time.Millisecond) + } + + return fmt.Errorf("failed to clean up service bindings for plan %q", planName) +} + +func (d *CatalogDeleter) cleanupInsances(planName string) error { + for retries := 0; retries < d.maxRetries; retries++ { + serviceInstances := &korifiv1alpha1.CFServiceInstanceList{} + Expect(d.k8sClient.List(d.ctx, serviceInstances)).To(Succeed()) + + instancesForPlan := itx.FromSlice(serviceInstances.Items). + Filter(func(instance korifiv1alpha1.CFServiceInstance) bool { + return instance.Spec.PlanGUID == planName + }).Collect() + + if len(instancesForPlan) == 0 { + return nil + } + + for _, instance := range instancesForPlan { + forceDelete(d.ctx, d.k8sClient, &instance) + } + + time.Sleep(100 * time.Millisecond) + } + + return fmt.Errorf("failed to clean up service bindings for plan %q", planName) +} + +func forceDelete[T any, PT k8s.ObjectWithDeepCopy[T]](ctx context.Context, k8sClient client.Client, obj PT) { + Expect(k8s.PatchResource(ctx, k8sClient, obj, func() { + obj.SetFinalizers(nil) + })).To(Succeed()) + + Expect(k8sClient.Delete(ctx, obj)).To(Succeed()) +} diff --git a/tests/smoke/catalog_test.go b/tests/smoke/catalog_test.go index de8091a1f..4f38ee3fc 100644 --- a/tests/smoke/catalog_test.go +++ b/tests/smoke/catalog_test.go @@ -2,6 +2,7 @@ package smoke_test import ( "code.cloudfoundry.org/korifi/tests/helpers" + "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/BooleanCat/go-functional/v2/it" "github.com/google/uuid" @@ -25,7 +26,7 @@ var _ = Describe("Service Catalog", func() { }) AfterEach(func() { - cleanupBroker(brokerName) + broker.NewCatalogDeleter(sharedData.RootNamespace).ForBrokerName(brokerName).Delete() }) Describe("cf service-brokers", func() { diff --git a/tests/smoke/services_test.go b/tests/smoke/services_test.go index 2f51f9656..418380884 100644 --- a/tests/smoke/services_test.go +++ b/tests/smoke/services_test.go @@ -2,6 +2,7 @@ package smoke_test import ( "code.cloudfoundry.org/korifi/tests/helpers" + "code.cloudfoundry.org/korifi/tests/helpers/broker" "github.com/BooleanCat/go-functional/v2/it" "github.com/google/uuid" @@ -26,7 +27,7 @@ var _ = Describe("Services", func() { }) AfterEach(func() { - cleanupBroker(brokerName) + broker.NewCatalogDeleter(sharedData.RootNamespace).ForBrokerName(brokerName).Delete() }) Describe("cf create-service", func() { diff --git a/tests/smoke/suite_test.go b/tests/smoke/suite_test.go index 800c8684c..8abf4ca1d 100644 --- a/tests/smoke/suite_test.go +++ b/tests/smoke/suite_test.go @@ -11,7 +11,6 @@ import ( korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/tests/helpers" - "code.cloudfoundry.org/korifi/tests/helpers/broker" "code.cloudfoundry.org/korifi/tests/helpers/fail_handler" "github.com/google/uuid" @@ -193,12 +192,6 @@ func getAppGUID(appName string) string { return string(session.Out.Contents()) } -func cleanupBroker(brokerName string) { - GinkgoHelper() - - broker.NewCatalogDeleter(sharedData.RootNamespace).ForBrokerName(brokerName).Delete() -} - func matchSubstrings(substrings ...string) types.GomegaMatcher { return MatchRegexp(strings.Join(substrings, ".*")) }