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 <[email protected]> * Add test case for application component race conditions Signed-off-by: thepetk <[email protected]> * Fix spiapi dep Signed-off-by: thepetk <[email protected]> * Retry in case application of component is to be deleted Signed-off-by: thepetk <[email protected]> * Run deletion of test application in background Signed-off-by: thepetk <[email protected]> * Return emtpy error for component deletion for under deletion apps Signed-off-by: thepetk <[email protected]> * Ensure that finalizer is removed Signed-off-by: thepetk <[email protected]> --------- Signed-off-by: thepetk <[email protected]>
redhat-appstudio · Mar 25, 2024 · f00d9c3 · f00d9c3
1 parent ac595a8
commit f00d9c3
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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
+}