From 3f326f06e42bf6ba8b99137ae4dba56914df3b4a Mon Sep 17 00:00:00 2001 From: wyike <77846369+wangyikewxgm@users.noreply.github.com> Date: Thu, 5 Aug 2021 17:15:57 +0800 Subject: [PATCH] application skip gc resource and rollout set workload ownerReference (#2024) * finish main logic and test * fix import order * rollout isn't created by applciation * fix comments * fix compatility test * mock error test --- .../dispatch/dispatch_suite_test.go | 59 +++++++++++++++++++ .../v1alpha2/application/dispatch/gc.go | 45 ++++++++++++++ .../v1alpha1/rollout/handler.go | 59 +++++++++++++++++++ .../v1alpha1/rollout/handler_suit_test.go | 40 +++++++++++++ .../v1alpha1/rollout/handler_test.go | 29 +++++++++ .../v1alpha1/rollout/suite_test.go | 2 +- pkg/oam/labels.go | 3 + test/e2e-test/rollout_trait_test.go | 22 +++++++ 8 files changed, 258 insertions(+), 1 deletion(-) 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 7dcfca4e2..b9a17f6f0 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/crossplane/crossplane-runtime/pkg/test" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" @@ -34,6 +36,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/pointer" @@ -41,6 +44,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" + "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/util" ) @@ -406,6 +410,61 @@ var _ = Describe("Test AppManifestsDispatcher", func() { }) }) +var _ = Describe("Test handleSkipGC func", func() { + var namespaceName string + ctx := context.Background() + BeforeEach(func() { + namespaceName = fmt.Sprintf("%s-%s", "dispatch-gc-skip-test", strconv.FormatInt(rand.Int63(), 16)) + Expect(k8sClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespaceName}})) + }) + + It("Test GC skip func ", func() { + handler := GCHandler{c: k8sClient} + wlName := "test-workload" + resourceTracker := v1beta1.ResourceTracker{ + ObjectMeta: metav1.ObjectMeta{ + Name: wlName, + UID: "test-uid", + }, + } + skipWorkload := &appsv1.Deployment{TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1", Kind: "Deployment"}} + skipWorkload.SetNamespace(namespaceName) + skipWorkload.SetName(wlName) + skipWorkload.SetOwnerReferences([]metav1.OwnerReference{*metav1.NewControllerRef( + &resourceTracker, v1beta1.ResourceTrackerKindVersionKind), + metav1.OwnerReference{UID: "app-uid", Name: "test-app", APIVersion: v1beta1.SchemeGroupVersion.String(), Kind: v1beta1.ApplicationKind}}) + skipWorkload.SetAnnotations(map[string]string{ + oam.AnnotationSkipGC: "true", + }) + skipWorkload.Spec.Selector = &metav1.LabelSelector{MatchLabels: map[string]string{"component": "mywebservice"}} + skipWorkload.Spec.Template = corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"component": "mywebservice"}}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{ + Name: "nginx", + Image: "nginx: 1.14.2", + Ports: []corev1.ContainerPort{{Name: "nginx", ContainerPort: int32(8080)}}}}}} + u, err := util.Object2Unstructured(skipWorkload) + Expect(err).Should(BeNil()) + Expect(k8sClient.Create(ctx, skipWorkload)).Should(BeNil()) + skipGC, err := handler.handleResourceSkipGC(ctx, u, &resourceTracker) + Expect(err).Should(BeNil()) + Expect(skipGC).Should(BeTrue()) + + checkWl := skipWorkload.DeepCopy() + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: checkWl.GetNamespace(), Name: checkWl.GetName()}, checkWl)).Should(BeNil()) + Expect(len(checkWl.GetOwnerReferences())).Should(BeEquivalentTo(1)) + Expect(checkWl.GetOwnerReferences()[0].UID).Should(BeEquivalentTo("app-uid")) + }) + + It("Test GC skip func, mock client return error", func() { + handler := GCHandler{c: &test.MockClient{ + MockGet: test.NewMockGetFn(fmt.Errorf("this isn't a not found error")), + }} + isSkip, err := handler.handleResourceSkipGC(ctx, &unstructured.Unstructured{}, &v1beta1.ResourceTracker{}) + Expect(err).ShouldNot(BeNil()) + Expect(isSkip).Should(BeEquivalentTo(false)) + }) +}) + // in envtest, no gc controller can delete PersistentVolume because of its finalizer // so we just use deletion timestamp to verify its deletion func persistentVolumeIsDeleted(pv *corev1.PersistentVolume) bool { 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 90d5b3d4f..095b95fcb 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/gc.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/dispatch/gc.go @@ -22,11 +22,14 @@ import ( "github.com/pkg/errors" kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + types "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" + "github.com/oam-dev/kubevela/pkg/oam" ) // GarbageCollector do GC according two resource trackers @@ -71,6 +74,17 @@ func (h *GCHandler) GarbageCollect(ctx context.Context, oldRT, newRT *v1beta1.Re toBeDeleted.SetKind(oldRsc.Kind) toBeDeleted.SetNamespace(oldRsc.Namespace) toBeDeleted.SetName(oldRsc.Name) + + isSkip := false + var err error + if isSkip, err = h.handleResourceSkipGC(ctx, toBeDeleted, oldRT); err != nil { + return errors.Wrap(err, "cannot handle resource skipResourceGC") + } + if isSkip { + // the resource have skipGC annotation, will not delete the resource + continue + } + if err := h.c.Delete(ctx, toBeDeleted); err != nil && !kerrors.IsNotFound(err) { klog.ErrorS(err, "Failed to delete a resource", "name", oldRsc.Name, "apiVersion", oldRsc.APIVersion, "kind", oldRsc.Kind) return errors.Wrapf(err, "cannot delete resource %q", oldRsc) @@ -107,3 +121,34 @@ func (h *GCHandler) validate() error { } return errors.Errorf("two resource trackers must come from the same application") } + +// handleResourceSkipGC will check resource have skipGC annotation,if yes patch the resource to orphan the resource and return true +func (h *GCHandler) handleResourceSkipGC(ctx context.Context, u *unstructured.Unstructured, oldRt *v1beta1.ResourceTracker) (bool, error) { + // deepCopy avoid modify origin resource + res := u.DeepCopy() + if err := h.c.Get(ctx, types.NamespacedName{Namespace: res.GetNamespace(), Name: res.GetName()}, res); err != nil { + if !kerrors.IsNotFound(err) { + klog.ErrorS(err, "handleResourceSkipGC faied cannot get res kind ", res.GetKind(), "namespace", res.GetNamespace(), "name", res.GetName()) + return false, err + } + // resource have gone, skip delete it + return true, nil + } + if _, exist := res.GetAnnotations()[oam.AnnotationSkipGC]; !exist { + return false, nil + } + var owners []metav1.OwnerReference + for _, ownerReference := range res.GetOwnerReferences() { + if ownerReference.UID == oldRt.GetUID() { + continue + } + owners = append(owners, ownerReference) + } + res.SetOwnerReferences(owners) + if err := h.c.Update(ctx, res); err != nil { + klog.ErrorS(err, "handleResourceSkipGC failed cannot orphan a res kind ", res.GetKind(), "namespace", res.GetNamespace(), "name", res.GetName()) + return false, err + } + klog.InfoS("succeed to handle a skipGC res kind ", res.GetKind(), "namespace", res.GetNamespace(), "name", res.GetName()) + return true, nil +} diff --git a/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler.go b/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler.go index fed4a5169..46ec7cb07 100644 --- a/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler.go +++ b/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler.go @@ -20,14 +20,20 @@ import ( "context" "fmt" + "github.com/pkg/errors" + v1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/klog/v2" + "k8s.io/utils/pointer" "github.com/crossplane/crossplane-runtime/pkg/event" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" "github.com/oam-dev/kubevela/pkg/controller/core.oam.dev/v1alpha2/application/assemble" "github.com/oam-dev/kubevela/pkg/controller/core.oam.dev/v1alpha2/applicationrollout" @@ -106,6 +112,7 @@ func (h *handler) extractWorkload(ctx context.Context, namespace, revisionName s } // applyTargetWorkload check the target workload whether exist. if not create it. +// and recode workload in resourceTracker func (h *handler) applyTargetWorkload(ctx context.Context) error { if h.targetWorkload == nil { return fmt.Errorf("cannot find target workload to template") @@ -115,6 +122,11 @@ func (h *handler) applyTargetWorkload(ctx context.Context) error { "rollout", h.rollout.Name, "targetWorkload", h.targetWorkload.GetName()) return err } + + if err := h.recordWorkloadInResourceTracker(ctx); err != nil { + return errors.Wrap(err, "fail to add resourceTracker as owner for workload") + } + klog.InfoS("template rollout target workload", "namespace", h.rollout.Namespace, "rollout", h.rollout.Name, "targetWorkload", h.targetWorkload.GetName()) return nil @@ -166,8 +178,14 @@ func (h *handler) setWorkloadBaseInfo() { if h.sourceWorkload != nil && len(h.sourceWorkload.GetNamespace()) == 0 { h.sourceWorkload.SetNamespace(h.rollout.Namespace) } + h.targetWorkload.SetName(h.compName) util.AddLabels(h.targetWorkload, map[string]string{oam.LabelAppComponentRevision: h.targetRevName}) + util.AddAnnotations(h.targetWorkload, map[string]string{oam.AnnotationSkipGC: "true"}) + + // pass rollout's ownerReference to workload + h.passOwnerToTargetWorkload() + if h.sourceWorkload != nil { h.sourceWorkload.SetName(h.compName) util.AddLabels(h.sourceWorkload, map[string]string{oam.LabelAppComponentRevision: h.sourceRevName}) @@ -219,3 +237,44 @@ func (h *handler) isRolloutModified(rollout v1alpha1.Rollout) bool { (rollout.Spec.RolloutPlan.TargetSize != nil && rollout.Status.RolloutTargetSize != -1 && rollout.Status.RolloutTargetSize != *rollout.Spec.RolloutPlan.TargetSize)) } + +func (h *handler) recordWorkloadInResourceTracker(ctx context.Context) error { + var resourceTrackerName string + for _, reference := range h.rollout.OwnerReferences { + if reference.Kind == v1beta1.ResourceTrackerKind && reference.APIVersion == v1beta1.SchemeGroupVersion.String() { + resourceTrackerName = reference.Name + } + } + if len(resourceTrackerName) == 0 { + // rollout isn't created by application + return nil + } + rt := v1beta1.ResourceTracker{} + if err := h.Get(ctx, types.NamespacedName{Name: resourceTrackerName}, &rt); err != nil { + klog.Errorf("fail to get resourceTracker to record workload rollout: namespace:%s, name: %s", h.rollout.Namespace, h.rollout.Name) + return err + } + recordedWorkload := corev1.ObjectReference{ + APIVersion: h.targetWorkload.GetAPIVersion(), + Kind: h.targetWorkload.GetKind(), + UID: h.targetWorkload.GetUID(), + Namespace: h.targetWorkload.GetNamespace(), + Name: h.targetWorkload.GetName(), + } + rt.Status.TrackedResources = append(rt.Status.TrackedResources, recordedWorkload) + if err := h.Status().Update(ctx, &rt); err != nil { + klog.Errorf("fail to update resourceTracker for rollout record workload namespace:%s, name: %s", h.rollout.Namespace, h.rollout.Name) + return err + } + klog.InfoS("succeed to record workload in resourceTracker rollout: namespace:%s, name: %s", h.rollout.Namespace, h.rollout.Name) + return nil +} + +func (h *handler) passOwnerToTargetWorkload() { + var owners []metav1.OwnerReference + for _, reference := range h.rollout.OwnerReferences { + reference.Controller = pointer.Bool(false) + owners = append(owners, reference) + } + h.targetWorkload.SetOwnerReferences(owners) +} diff --git a/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_suit_test.go b/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_suit_test.go index 1fc61e1fc..b251c61e0 100644 --- a/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_suit_test.go +++ b/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_suit_test.go @@ -30,6 +30,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/meta" "github.com/ghodss/yaml" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" "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" @@ -38,6 +39,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" ) var _ = Describe("Test rollout related handler func", func() { @@ -252,6 +254,44 @@ var _ = Describe("Test rollout related handler func", func() { Expect(len(rollout.Finalizers)).Should(BeEquivalentTo(1)) Expect(rollout.Status.RollingState).Should(BeEquivalentTo(v1alpha1.RolloutDeletingState)) }) + + It("Test recordeWorkloadInResourceTracker func", func() { + ctx := context.Background() + rtName := "resourcetracker-v1-test-namespace" + rt := v1beta1.ResourceTracker{ + ObjectMeta: metav1.ObjectMeta{ + Name: rtName, + }, + } + Expect(k8sClient.Create(ctx, &rt)).Should(BeNil()) + rollout := v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(&rt, v1beta1.ResourceTrackerKindVersionKind), + }, + }, + } + u := &unstructured.Unstructured{} + u.SetAPIVersion("apps/v1") + u.SetNamespace("test-namespace") + u.SetName("test-workload") + u.SetUID("test-uid") + u.SetKind("Deployment") + h := &handler{ + reconciler: &reconciler{ + Client: k8sClient, + record: event.NewNopRecorder(), + }, + rollout: &rollout, + targetWorkload: u, + } + Expect(h.recordWorkloadInResourceTracker(ctx)).Should(BeNil()) + checkRt := v1beta1.ResourceTracker{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: rtName}, &checkRt)).Should(BeNil()) + Expect(len(checkRt.Status.TrackedResources)).Should(BeEquivalentTo(1)) + Expect(checkRt.Status.TrackedResources[0].Name).Should(BeEquivalentTo("test-workload")) + Expect(checkRt.Status.TrackedResources[0].UID).Should(BeEquivalentTo("test-uid")) + }) }) }) diff --git a/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_test.go b/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_test.go index 8505d251a..131a957a6 100644 --- a/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_test.go +++ b/pkg/controller/standard.oam.dev/v1alpha1/rollout/handler_test.go @@ -19,8 +19,12 @@ package rollout import ( "testing" + "gotest.tools/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/utils/pointer" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" oamstandard "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" ) @@ -302,3 +306,28 @@ func Test_isRolloutModified(t *testing.T) { }) } } + +func TestPassOwnerReference(t *testing.T) { + rollout := oamstandard.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: v1beta1.SchemeGroupVersion.String(), + Kind: v1beta1.ResourceTrackerKind, + UID: "test-uid", + Controller: pointer.Bool(true), + Name: "test-resouceTracker", + }, + }, + }, + } + u := &unstructured.Unstructured{} + u.SetName("test-workload") + h := &handler{ + rollout: &rollout, + targetWorkload: u, + } + h.passOwnerToTargetWorkload() + assert.Assert(t, len(h.targetWorkload.GetOwnerReferences()) == 1) + assert.Assert(t, *h.targetWorkload.GetOwnerReferences()[0].Controller == false) +} diff --git a/pkg/controller/standard.oam.dev/v1alpha1/rollout/suite_test.go b/pkg/controller/standard.oam.dev/v1alpha1/rollout/suite_test.go index 407a10877..773808109 100644 --- a/pkg/controller/standard.oam.dev/v1alpha1/rollout/suite_test.go +++ b/pkg/controller/standard.oam.dev/v1alpha1/rollout/suite_test.go @@ -59,7 +59,7 @@ var _ = BeforeSuite(func(done Done) { By("bootstrapping test environment") var yamlPath string if _, set := os.LookupEnv("COMPATIBILITY_TEST"); set { - yamlPath = "../../../../../../test/compatibility-test/testdata" + yamlPath = "../../../../../test/compatibility-test/testdata" } else { yamlPath = filepath.Join("../../../../..", "charts", "vela-core", "crds") } diff --git a/pkg/oam/labels.go b/pkg/oam/labels.go index ee62ff25d..0724f0be3 100644 --- a/pkg/oam/labels.go +++ b/pkg/oam/labels.go @@ -108,4 +108,7 @@ const ( // AnnotationFilterLabelKeys is used to filter labels passed to workload and trait, split by comma AnnotationFilterLabelKeys = "filter.oam.dev/label-keys" + + // AnnotationSkipGC is used to tell application to skip gc workload/trait + AnnotationSkipGC = "app.oam.dev/skipGC" ) diff --git a/test/e2e-test/rollout_trait_test.go b/test/e2e-test/rollout_trait_test.go index 6a80f3549..6d486836c 100644 --- a/test/e2e-test/rollout_trait_test.go +++ b/test/e2e-test/rollout_trait_test.go @@ -123,6 +123,13 @@ var _ = Describe("rollout related e2e-test,rollout trait test", func() { if targerDeploy.Status.UpdatedReplicas != *targerDeploy.Spec.Replicas { return fmt.Errorf("update not finish") } + if len(targerDeploy.OwnerReferences) != 1 { + return fmt.Errorf("workload ownerReference missMatch") + } + if targerDeploy.OwnerReferences[0].Kind != rollout.OwnerReferences[0].Kind || + targerDeploy.OwnerReferences[0].Name != rollout.OwnerReferences[0].Name { + return fmt.Errorf("workload ownerReference missMatch") + } if rollout.Status.LastSourceRevision == "" { return nil } @@ -229,6 +236,21 @@ var _ = Describe("rollout related e2e-test,rollout trait test", func() { return nil }, 30*time.Second, 300*time.Millisecond).Should(BeNil()) verifySuccess("express-server-v4") + By("delete the application, check workload have been removed") + Expect(k8sClient.Delete(ctx, checkApp)).Should(BeNil()) + listOptions := []client.ListOption{ + client.InNamespace(namespaceName), + } + deployList := &v1.DeploymentList{} + Eventually(func() error { + if err := k8sClient.List(ctx, deployList, listOptions...); err != nil { + return err + } + if len(deployList.Items) != 0 { + return fmt.Errorf("workload have not been removed") + } + return nil + }, 30*time.Second, 300*time.Millisecond).Should(BeNil()) }) })