From f00d9c392bf901acbb78b1e9c672d8958589331b Mon Sep 17 00:00:00 2001 From: Theofanis Petkos Date: Mon, 25 Mar 2024 12:10:30 +0000 Subject: [PATCH] Avoid race coditions from component deletion for applications already marked to be deleted (#451) * Check if application is under deletion before component deletion Signed-off-by: thepetk * Add test case for application component race conditions Signed-off-by: thepetk * Fix spiapi dep Signed-off-by: thepetk * Retry in case application of component is to be deleted Signed-off-by: thepetk * Run deletion of test application in background Signed-off-by: thepetk * Return emtpy error for component deletion for under deletion apps Signed-off-by: thepetk * Ensure that finalizer is removed Signed-off-by: thepetk --------- Signed-off-by: thepetk --- controllers/component_controller.go | 7 +- controllers/component_controller_test.go | 89 ++++++++++++++++++++++-- pkg/metrics/component.go | 12 ++++ 3 files changed, 101 insertions(+), 7 deletions(-) diff --git a/controllers/component_controller.go b/controllers/component_controller.go index c32302c5..71c49c33 100644 --- a/controllers/component_controller.go +++ b/controllers/component_controller.go @@ -188,8 +188,8 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( log.Info(fmt.Sprintf("added the finalizer %v", req.NamespacedName)) } } else { - if hasApplication.Status.Devfile != "" && (forceGenerateGitopsResource || len(component.Status.Conditions) > 0 && component.Status.Conditions[len(component.Status.Conditions)-1].Status == metav1.ConditionTrue && containsString(component.GetFinalizers(), compFinalizerName)) { - // only attempt to finalize and update the gitops repo if an Application is present & the previous Component status is good + if hasApplication.Status.Devfile != "" && hasApplication.ObjectMeta.DeletionTimestamp.IsZero() && (forceGenerateGitopsResource || len(component.Status.Conditions) > 0 && component.Status.Conditions[len(component.Status.Conditions)-1].Status == metav1.ConditionTrue && containsString(component.GetFinalizers(), compFinalizerName)) { + // only attempt to finalize and update the gitops repo if an Application is present & not under deletion & the previous Component status is good // A finalizer is present for the Component CR, so make sure we do the necessary cleanup steps metrics.ComponentDeletionTotalReqs.Inc() if err := r.Finalize(ctx, &component, &hasApplication, ghClient, gitToken); err != nil { @@ -212,6 +212,9 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( controllerutil.RemoveFinalizer(&component, compFinalizerName) if err := r.Update(ctx, &component); err != nil { return ctrl.Result{}, err + } else if !hasApplication.ObjectMeta.DeletionTimestamp.IsZero() { + log.Info("application %v is under deletion. Skipping deletion for component %v", hasApplication.Name, component.Name) + return ctrl.Result{}, nil } else { metrics.ComponentDeletionSucceeded.Inc() } diff --git a/controllers/component_controller_test.go b/controllers/component_controller_test.go index f638cb46..c792b84b 100644 --- a/controllers/component_controller_test.go +++ b/controllers/component_controller_test.go @@ -24,22 +24,22 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/testutil" + cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" "github.com/redhat-appstudio/application-service/pkg/metrics" - - "github.com/devfile/library/v2/pkg/devfile/parser" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/yaml" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/api/v2/pkg/attributes" + "github.com/devfile/library/v2/pkg/devfile/parser" data "github.com/devfile/library/v2/pkg/devfile/parser/data" "github.com/devfile/library/v2/pkg/devfile/parser/data/v2/common" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" appstudiov1alpha1 "github.com/redhat-appstudio/application-api/api/v1alpha1" - cdqanalysis "github.com/redhat-appstudio/application-service/cdq-analysis/pkg" devfilePkg "github.com/redhat-appstudio/application-service/pkg/devfile" - spiapi "github.com/redhat-appstudio/service-provider-integration-operator/api/v1beta1" - "sigs.k8s.io/yaml" + spiapi "github.com/redhat-appstudio/service-provider-integration-operator/api/v1beta1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -2078,6 +2078,7 @@ var _ = Describe("Component controller", func() { deleteHASAppCR(hasAppLookupKey) }) }) + Context("Component with valid GitOps repository", func() { It("Should successfully update CR conditions and status", func() { beforeCreateTotalReqs := testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) @@ -2181,6 +2182,7 @@ var _ = Describe("Component controller", func() { deleteHASAppCR(hasAppLookupKey) }) }) + Context("force generate gitops resource", func() { It("Should successfully update CR conditions and status", func() { beforeCreateTotalReqs := testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) @@ -2958,6 +2960,83 @@ var _ = Describe("Component controller", func() { deleteHASAppCR(hasAppLookupKey) }) }) + Context("Component with application marked to be deleted", func() { + It("Should not increase the deletion metrics", func() { + beforeDeleteFailedReqs := testutil.ToFloat64(metrics.GetComponentDeletionFailed()) + beforeDeleteSucceedReqs := testutil.ToFloat64(metrics.GetComponentDeletionSucceeded()) + beforeDeleteTotalReqs := testutil.ToFloat64(metrics.GetComponentDeletionTotalReqs()) + + ctx := context.Background() + + applicationName := HASAppName + "29" + componentName := HASCompName + "29" + + createAndFetchSimpleApp(applicationName, HASAppNamespace, DisplayName, Description) + + hasAppLookupKey := types.NamespacedName{Name: applicationName, Namespace: HASAppNamespace} + fetchedHasApp := &appstudiov1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{}, + } + Eventually(func() bool { + k8sClient.Get(context.Background(), hasAppLookupKey, fetchedHasApp) + return fetchedHasApp.Status.Devfile != "" + }, timeout, interval).Should(BeTrue()) + + hasComp := &appstudiov1alpha1.Component{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "appstudio.redhat.com/v1alpha1", + Kind: "Component", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: componentName, + Namespace: HASAppNamespace, + }, + Spec: appstudiov1alpha1.ComponentSpec{ + ComponentName: ComponentName, + Application: applicationName, + Source: appstudiov1alpha1.ComponentSource{ + ComponentSourceUnion: appstudiov1alpha1.ComponentSourceUnion{ + GitSource: &appstudiov1alpha1.GitSource{ + URL: SampleRepoLink, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, hasComp)).Should(Succeed()) + + // Look up the has app resource that was created. + // num(conditions) may still be < 1 on the first try, so retry until at least _some_ condition is set + hasCompLookupKey := types.NamespacedName{Name: componentName, Namespace: HASAppNamespace} + createdHasComp := &appstudiov1alpha1.Component{} + Eventually(func() bool { + k8sClient.Get(context.Background(), hasCompLookupKey, createdHasComp) + return len(createdHasComp.Status.Conditions) > 0 + }, timeout, interval).Should(BeTrue()) + + Expect(k8sClient.Update(ctx, createdHasComp)).Should(Succeed()) + + // Set deletion timestamp for application. + gracePeriodSeconds := int64(5) + opts := &client.DeleteOptions{GracePeriodSeconds: &gracePeriodSeconds} + + k8sClient.Delete(context.Background(), fetchedHasApp, opts) + + // Check that the deletion timestamp has been set for component's application + fetchedHasApp = &appstudiov1alpha1.Application{} + Eventually(func() bool { + k8sClient.Get(context.Background(), hasAppLookupKey, fetchedHasApp) + return !fetchedHasApp.ObjectMeta.DeletionTimestamp.IsZero() + }, timeout, interval).Should(BeTrue()) + + // Try to delete the component. It should be ignored as the application is under deletion + k8sClient.Delete(ctx, createdHasComp) + + Expect(testutil.ToFloat64(metrics.GetComponentDeletionFailed()) == beforeDeleteFailedReqs).To(BeTrue()) + Expect(testutil.ToFloat64(metrics.GetComponentDeletionSucceeded()) == beforeDeleteSucceedReqs).To(BeTrue()) + Expect(testutil.ToFloat64(metrics.GetComponentDeletionTotalReqs()) == beforeDeleteTotalReqs).To(BeTrue()) + }) + }) Context("Create component having git source from gitlab", func() { It("Should not increase the component failure metrics", func() { beforeCreateTotalReqs := testutil.ToFloat64(metrics.GetComponentCreationTotalReqs()) diff --git a/pkg/metrics/component.go b/pkg/metrics/component.go index 21ce4bc5..9fb93285 100644 --- a/pkg/metrics/component.go +++ b/pkg/metrics/component.go @@ -81,6 +81,10 @@ func GetComponentCreationFailed() prometheus.Counter { return componentCreationFailed } +func GetComponentDeletionFailed() prometheus.Counter { + return ComponentDeletionFailed +} + // IncrementComponentCreationSucceeded increments the component creation succeeded metric. func IncrementComponentCreationSucceeded(oldError, newError string) { if oldError == "" || newError == "" || !strings.Contains(oldError, newError) { @@ -98,6 +102,14 @@ func GetComponentCreationSucceeded() prometheus.Counter { return componentCreationSucceeded } +func GetComponentDeletionSucceeded() prometheus.Counter { + return ComponentDeletionSucceeded +} + func GetComponentCreationTotalReqs() prometheus.Counter { return componentCreationTotalReqs } + +func GetComponentDeletionTotalReqs() prometheus.Counter { + return ComponentDeletionTotalReqs +}