From d83fa47741931d06bd94504cfdb8d97891641a98 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 12 Nov 2021 11:47:24 +0800 Subject: [PATCH] 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 7fb0c2ad139c197f7c87abea2b4e5222e2d366f1) Co-authored-by: wangyike --- .../v1alpha2/application/dispatch/dispatch.go | 2 +- .../dispatch/dispatch_suite_test.go | 7 ++- .../application/dispatch/dispatch_test.go | 31 +++++++++++++ .../v1alpha2/application/dispatch/gc.go | 22 +++++++++- test/e2e-test/rollout_trait_test.go | 43 ++++++++++++++++++- .../rollout/deployment/multi_comp_app.yaml | 27 ++++++++++++ 6 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 test/e2e-test/testdata/rollout/deployment/multi_comp_app.yaml diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch.go b/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch.go index 6ff29d2f8..07555df4a 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch.go +++ b/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), } } diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_suite_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_suite_test.go index 8eabf6398..16f31b140 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_suite_test.go +++ b/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}}) diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_test.go index 0f83b7caf..8a233e6ec 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/dispatch_test.go +++ b/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) + } + } +} diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/gc.go b/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/gc.go index 389ee3f46..7ab6b38fe 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/gc.go +++ 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 +} diff --git a/test/e2e-test/rollout_trait_test.go b/test/e2e-test/rollout_trait_test.go index 4db9cb26b..b58f69eb2 100644 --- a/test/e2e-test/rollout_trait_test.go +++ b/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 ( diff --git a/test/e2e-test/testdata/rollout/deployment/multi_comp_app.yaml b/test/e2e-test/testdata/rollout/deployment/multi_comp_app.yaml new file mode 100644 index 000000000..549067878 --- /dev/null +++ b/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 \ No newline at end of file