Fix: fix delete a component from application not delete workload (#2690)

lint

Fix: error test

Fix: fix e2e rollout

Fix comment

(cherry picked from commit 7fb0c2ad13)

Co-authored-by: wangyike <wangyike_wyk@163.com>

pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch.go

@@ -43,7 +43,7 @@ func NewAppManifestsDispatcher(c client.Client, appRev *v1beta1.ApplicationRevis
 		c:          c,
 		applicator: apply.NewAPIApplicator(c),
 		appRev:     appRev,
-		gcHandler:  NewGCHandler(c, appRev.Namespace),
+		gcHandler:  NewGCHandler(c, appRev.Namespace, *appRev),
 	}
 }
 

pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_suite_test.go

@@ -26,6 +26,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/common"
+
 	"github.com/crossplane/crossplane-runtime/pkg/test"
 
 	. "github.com/onsi/ginkgo"
@@ -419,7 +421,9 @@ var _ = Describe("Test handleSkipGC func", func() {
 	})
 
 	It("Test GC skip func ", func() {
-		handler := GCHandler{c: k8sClient}
+		handler := GCHandler{c: k8sClient, appRev: v1beta1.ApplicationRevision{Spec: v1beta1.ApplicationRevisionSpec{
+			Application: v1beta1.Application{Spec: v1beta1.ApplicationSpec{Components: []common.ApplicationComponent{{Name: "mywebservice"}}}},
+		}}}
 		wlName := "test-workload"
 		resourceTracker := v1beta1.ResourceTracker{
 			ObjectMeta: metav1.ObjectMeta{
@@ -430,6 +434,7 @@ var _ = Describe("Test handleSkipGC func", func() {
 		skipWorkload := &appsv1.Deployment{TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1", Kind: "Deployment"}}
 		skipWorkload.SetNamespace(namespaceName)
 		skipWorkload.SetName(wlName)
+		skipWorkload.SetLabels(map[string]string{oam.LabelAppComponent: "mywebservice"})
 		skipWorkload.SetOwnerReferences([]metav1.OwnerReference{*metav1.NewControllerRef(
 			&resourceTracker, v1beta1.ResourceTrackerKindVersionKind),
 			metav1.OwnerReference{UID: "app-uid", Name: "test-app", APIVersion: v1beta1.SchemeGroupVersion.String(), Kind: v1beta1.ApplicationKind}})

pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_test.go

@@ -22,6 +22,9 @@ import (
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/common"
+	"github.com/oam-dev/kubevela/pkg/oam"
 )
 
 func TestSetOAMOwner(t *testing.T) {
@@ -107,3 +110,31 @@ func TestSetOAMOwner(t *testing.T) {
 		assert.Equal(t, ti.ExpOwner, ti.OO.GetOwnerReferences(), name)
 	}
 }
+
+func TestCheckComponentDeleted(t *testing.T) {
+	wl_1 := unstructured.Unstructured{}
+	wl_1.SetLabels(map[string]string{oam.LabelAppComponent: "comp-1"})
+
+	wl_2 := unstructured.Unstructured{}
+
+	wl_3 := unstructured.Unstructured{}
+	wl_3.SetLabels(map[string]string{oam.LabelAppComponent: "comp-3"})
+
+	components := []common.ApplicationComponent{{Name: "comp-1"}}
+
+	testCase := map[string]struct {
+		u   unstructured.Unstructured
+		res bool
+	}{
+		"exsit comp":       {wl_1, false},
+		"no label deleted": {wl_2, true},
+		"not exsit comp":   {wl_3, true},
+	}
+
+	for caseName, s := range testCase {
+		b := checkResourceRelatedCompDeleted(s.u, components)
+		if b != s.res {
+			t.Errorf("check comp deleted func meet error: %s want %v got %v", caseName, s.res, b)
+		}
+	}
+}

pkg/controller/core.oam.dev/v1alpha2/application/dispatch/gc.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/common"
 	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
 	"github.com/oam-dev/kubevela/pkg/oam"
 )
@@ -38,8 +39,8 @@ type GarbageCollector interface {
 }
 
 // NewGCHandler create a GCHandler
-func NewGCHandler(c client.Client, ns string) *GCHandler {
-	return &GCHandler{c, ns, nil, nil}
+func NewGCHandler(c client.Client, ns string, appRev v1beta1.ApplicationRevision) *GCHandler {
+	return &GCHandler{c, ns, nil, nil, appRev}
 }
 
 // GCHandler implement GarbageCollector interface
@@ -49,6 +50,8 @@ type GCHandler struct {
 
 	oldRT *v1beta1.ResourceTracker
 	newRT *v1beta1.ResourceTracker
+
+	appRev v1beta1.ApplicationRevision
 }
 
 // GarbageCollect delete the old resources that are no longer in the new resource tracker
@@ -137,6 +140,10 @@ func (h *GCHandler) handleResourceSkipGC(ctx context.Context, u *unstructured.Un
 	if _, exist := res.GetAnnotations()[oam.AnnotationSkipGC]; !exist {
 		return false, nil
 	}
+	// if the component have been deleted don't skipGC
+	if checkResourceRelatedCompDeleted(*res, h.appRev.Spec.Application.Spec.Components) {
+		return false, nil
+	}
 	var owners []metav1.OwnerReference
 	for _, ownerReference := range res.GetOwnerReferences() {
 		if ownerReference.UID == oldRt.GetUID() {
@@ -152,3 +159,14 @@ func (h *GCHandler) handleResourceSkipGC(ctx context.Context, u *unstructured.Un
 	klog.InfoS("succeed to handle a skipGC res kind ", res.GetKind(), "namespace", res.GetNamespace(), "name", res.GetName())
 	return true, nil
 }
+
+func checkResourceRelatedCompDeleted(res unstructured.Unstructured, comps []common.ApplicationComponent) bool {
+	compName := res.GetLabels()[oam.LabelAppComponent]
+	deleted := true
+	for _, comp := range comps {
+		if compName == comp.Name {
+			deleted = false
+		}
+	}
+	return deleted
+}

test/e2e-test/rollout_trait_test.go

@@ -30,6 +30,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/yaml"
 
+	common2 "github.com/oam-dev/kubevela/apis/core.oam.dev/common"
 	"github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1"
 	"github.com/oam-dev/kubevela/pkg/oam"
 	"github.com/oam-dev/kubevela/pkg/oam/util"
@@ -105,6 +106,7 @@ var _ = Describe("rollout related e2e-test,rollout trait test", func() {
 		By("check rollout status have succeed")
 		Eventually(func() error {
 			rolloutKey := types.NamespacedName{Namespace: namespaceName, Name: componentName}
+			rollout = v1alpha1.Rollout{}
 			if err := k8sClient.Get(ctx, rolloutKey, &rollout); err != nil {
 				return err
 			}
@@ -150,7 +152,7 @@ var _ = Describe("rollout related e2e-test,rollout trait test", func() {
 			}
 			deployKey = types.NamespacedName{Namespace: namespaceName, Name: rollout.Status.LastSourceRevision}
 			if err := k8sClient.Get(ctx, deployKey, &sourceDeploy); err == nil || !apierrors.IsNotFound(err) {
-				return fmt.Errorf("source deploy still exist")
+				return fmt.Errorf("source deploy still exist namespace %s deployName %s", namespaceName, rollout.Status.LastSourceRevision)
 			}
 			return nil
 		}, time.Second*60, 300*time.Millisecond).Should(BeNil())
@@ -321,7 +323,7 @@ var _ = Describe("rollout related e2e-test,rollout trait test", func() {
 		}, 30*time.Second, 300*time.Millisecond).Should(BeNil())
 	})
 
-	It("rollout scale up adnd down without rollout batches", func() {
+	It("rollout scale up and down without rollout batches", func() {
 		By("first scale operation")
 		Expect(common.ReadYamlToObject("testdata/rollout/deployment/application.yaml", &app)).Should(BeNil())
 		app.Namespace = namespaceName
@@ -409,6 +411,43 @@ var _ = Describe("rollout related e2e-test,rollout trait test", func() {
 		}, 30*time.Second, 300*time.Millisecond).Should(BeNil())
 		verifySuccess("express-server-v2")
 	})
+
+	It("Delete a component with rollout trait from an application should delete this workload", func() {
+		By("first scale operation")
+		Expect(common.ReadYamlToObject("testdata/rollout/deployment/multi_comp_app.yaml", &app)).Should(BeNil())
+		app.Namespace = namespaceName
+		Expect(k8sClient.Create(ctx, &app)).Should(BeNil())
+		verifySuccess("express-server-v1")
+		componentName = "express-server-another"
+		verifySuccess("express-server-another-v1")
+		By("delete a component")
+		Eventually(func() error {
+			checkApp := &v1beta1.Application{}
+			if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: namespaceName, Name: app.Name}, checkApp); err != nil {
+				return err
+			}
+			checkApp.Spec.Components = []common2.ApplicationComponent{checkApp.Spec.Components[0]}
+			if err := k8sClient.Update(ctx, checkApp); err != nil {
+				return err
+			}
+			return nil
+		}, 30*time.Second, 300*time.Millisecond).Should(BeNil())
+		By("check deployment have been gc")
+		Eventually(func() error {
+			checkApp := &v1beta1.Application{}
+			if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: namespaceName, Name: app.Name}, checkApp); err != nil {
+				return err
+			}
+			if len(checkApp.Spec.Components) != 1 || checkApp.Spec.Components[0].Name != "express-server" {
+				return fmt.Errorf("app hasn't update yet")
+			}
+			deploy := v1.Deployment{}
+			if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: namespaceName, Name: "express-server-another-v1"}, &deploy); err == nil || !apierrors.IsNotFound(err) {
+				return fmt.Errorf("another deployment haven't been delete")
+			}
+			return nil
+		}, 30*time.Second, 300*time.Millisecond).Should(BeNil())
+	})
 })
 
 const (

test/e2e-test/testdata/rollout/deployment/multi_comp_app.yaml

@@ -0,0 +1,27 @@
+apiVersion: core.oam.dev/v1beta1
+kind: Application
+metadata:
+  name: rollout-trait-test
+spec:
+  components:
+    - name: express-server
+      type: webservice
+      properties:
+        image: stefanprodan/podinfo:4.0.3
+      traits:
+        - type: rollout
+          properties:
+            targetSize: 2
+            firstBatchReplicas: 1
+            secondBatchReplicas: 1
+
+    - name: express-server-another
+      type: webservice
+      properties:
+        image: stefanprodan/podinfo:4.0.3
+      traits:
+        - type: rollout
+          properties:
+            targetSize: 2
+            firstBatchReplicas: 1
+            secondBatchReplicas: 1