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

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 {

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
+}

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)
+}

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"))
+		})
 	})
 })
 

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)
+}

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")
 	}

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"
 )

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())
 	})
 })