cherry-pick(#1136) fix revision enable workload check and prevent GC if replica>0 (#1141)

* fix revision enable workload check and prevent GC if replica>0

* fix ci and vet

cmd/core/main.go

@@ -173,10 +173,10 @@ func main() {
 	case "", "false", string(oamcontroller.ApplyOnceOnlyOff):
 		controllerArgs.ApplyMode = oamcontroller.ApplyOnceOnlyOff
 		setupLog.Info("ApplyOnceOnly is disabled")
-	case "true", oamcontroller.ApplyOnceOnlyOn:
+	case "true", string(oamcontroller.ApplyOnceOnlyOn):
 		controllerArgs.ApplyMode = oamcontroller.ApplyOnceOnlyOn
 		setupLog.Info("ApplyOnceOnly is enabled, that means workload or trait only apply once if no spec change even they are changed by others")
-	case oamcontroller.ApplyOnceOnlyForce:
+	case string(oamcontroller.ApplyOnceOnlyForce):
 		controllerArgs.ApplyMode = oamcontroller.ApplyOnceOnlyForce
 		setupLog.Info("ApplyOnceOnlyForce is enabled, that means workload or trait only apply once if no spec change even they are changed or deleted by others")
 	default:

pkg/controller/core.oam.dev/oamruntime_controller.go

@@ -26,12 +26,12 @@ const (
 
 	// ApplyOnceOnlyOn indicates workloads and traits should not be affected
 	// if no spec change is made in the ApplicationConfiguration.
-	ApplyOnceOnlyOn = "on"
+	ApplyOnceOnlyOn ApplyOnceOnlyMode = "on"
 
 	// ApplyOnceOnlyForce is a more strong case for ApplyOnceOnly, the workload
 	// and traits won't be affected if no spec change is made in the ApplicationConfiguration,
 	// even if the workload or trait has been deleted from cluster.
-	ApplyOnceOnlyForce = "force"
+	ApplyOnceOnlyForce ApplyOnceOnlyMode = "force"
 )
 
 // Args args used by controller

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go

@@ -37,6 +37,7 @@ import (
 
 	"github.com/crossplane/crossplane-runtime/apis/core/v1alpha1"
 	"github.com/crossplane/crossplane-runtime/pkg/event"
+	"github.com/crossplane/crossplane-runtime/pkg/fieldpath"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
 	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
@@ -316,6 +317,13 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco
 		log := log.WithValues("kind", e.GetKind(), "name", e.GetName())
 		record := r.record.WithAnnotations("kind", e.GetKind(), "name", e.GetName())
 
+		err := r.confirmDeleteOnApplyOnceMode(ctx, ac.GetNamespace(), &e)
+		if err != nil {
+			log.Debug("confirm component can't be garbage collected", "error", err)
+			record.Event(ac, event.Warning(reasonCannotGGComponents, err))
+			ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errGCComponent)))
+			return reconcile.Result{}, errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus)
+		}
 		if err := r.client.Delete(ctx, &e); resource.IgnoreNotFound(err) != nil {
 			log.Debug("Cannot garbage collect component", "error", err)
 			record.Event(ac, event.Warning(reasonCannotGGComponents, err))
@@ -340,6 +348,41 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (result reco
 	return reconcile.Result{RequeueAfter: waitTime}, nil
 }
 
+// confirmDeleteOnApplyOnceMode will confirm whether the workload can be delete or not in apply once only enabled mode
+// currently only workload replicas with 0 can be delete
+func (r *OAMApplicationReconciler) confirmDeleteOnApplyOnceMode(ctx context.Context, namespace string, u *unstructured.Unstructured) error {
+	if r.applyOnceOnlyMode == core.ApplyOnceOnlyOff {
+		return nil
+	}
+	getU := u.DeepCopy()
+	err := r.client.Get(ctx, client.ObjectKey{Name: u.GetName(), Namespace: namespace}, getU)
+	if err != nil {
+		// no need to check if workload not found
+		return resource.IgnoreNotFound(err)
+	}
+	// only check for workload
+	if labels := getU.GetLabels(); labels == nil || labels[oam.LabelOAMResourceType] != oam.ResourceTypeWorkload {
+		return nil
+	}
+	paved := fieldpath.Pave(getU.Object)
+
+	// TODO: add more kinds of workload replica check here if needed
+	// "spec.replicas" maybe not accurate for all kinds of workload, but it work for most of them(including Deployment/StatefulSet/CloneSet).
+	// For workload which don't align with the `spec.replicas` schema, the check won't work
+	replicas, err := paved.GetInteger("spec.replicas")
+	if err != nil {
+		// it's possible for workload without the `spec.replicas`, it's omitempty
+		if strings.Contains(err.Error(), "no such field") {
+			return nil
+		}
+		return errors.WithMessage(err, "fail to get 'spec.replicas' from workload")
+	}
+	if replicas > 0 {
+		return errors.Errorf("can't delete workload with replicas %d in apply once only mode", replicas)
+	}
+	return nil
+}
+
 // UpdateStatus updates v1alpha2.ApplicationConfiguration's Status with retry.RetryOnConflict
 func (r *OAMApplicationReconciler) UpdateStatus(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, opts ...client.UpdateOption) error {
 	status := ac.DeepCopy().Status
@@ -552,9 +595,20 @@ func (fn GarbageCollectorFn) Eligible(namespace string, ws []v1alpha2.WorkloadSt
 }
 
 // IsRevisionWorkload check is a workload is an old revision Workload which shouldn't be garbage collected.
-// TODO(wonderflow): Do we have a better way to recognise it's a revisionWorkload which can't be garbage collected by AppConfig?
-func IsRevisionWorkload(status v1alpha2.WorkloadStatus) bool {
-	return strings.HasPrefix(status.Reference.Name, status.ComponentName+"-")
+func IsRevisionWorkload(status v1alpha2.WorkloadStatus, w []Workload) bool {
+	if strings.HasPrefix(status.Reference.Name, status.ComponentName+"-") {
+		// for compatibility, keep the old way
+		return true
+	}
+
+	// check all workload, with same componentName
+	for _, wr := range w {
+		if wr.ComponentName == status.ComponentName {
+			return wr.RevisionEnabled
+		}
+	}
+	// component not found, should be deleted
+	return false
 }
 
 func eligible(namespace string, ws []v1alpha2.WorkloadStatus, w []Workload) []unstructured.Unstructured {
@@ -578,7 +632,7 @@ func eligible(namespace string, ws []v1alpha2.WorkloadStatus, w []Workload) []un
 	eligible := make([]unstructured.Unstructured, 0)
 	for _, s := range ws {
 
-		if !applied[s.Reference] && !IsRevisionWorkload(s) {
+		if !applied[s.Reference] && !IsRevisionWorkload(s, w) {
 			w := &unstructured.Unstructured{}
 			w.SetAPIVersion(s.Reference.APIVersion)
 			w.SetKind(s.Reference.Kind)

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go

@@ -852,9 +852,18 @@ func TestEligible(t *testing.T) {
 }
 
 func TestIsRevisionWorkload(t *testing.T) {
-	if true != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "compName-rev1"}}) {
+	if true != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "compName-rev1"}}, nil) {
 		t.Error("workloadName has componentName as prefix is revisionWorkload")
 	}
+	if true != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "speciedName"}}, []Workload{{ComponentName: "compName", RevisionEnabled: true}}) {
+		t.Error("workloadName has componentName same and revisionEnabled is revisionWorkload")
+	}
+	if false != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "speciedName"}}, []Workload{{ComponentName: "compName", RevisionEnabled: false}}) {
+		t.Error("workloadName has componentName same and revisionEnabled is false")
+	}
+	if false != IsRevisionWorkload(v1alpha2.WorkloadStatus{ComponentName: "compName", Reference: runtimev1alpha1.TypedReference{Name: "speciedName"}}, []Workload{{ComponentName: "compName-notmatch", RevisionEnabled: true}}) {
+		t.Error("workload with no prefix and no componentName match is not revisionEnabled ")
+	}
 }
 
 func TestDependency(t *testing.T) {

pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go

@@ -32,6 +32,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/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -824,3 +825,342 @@ var _ = Describe("Component Revision Enabled with apply once only force", func()
 	})
 
 })
+
+var _ = Describe("Component Revision Enabled with workloadName set and apply once only force", func() {
+	const (
+		namespace         = "revision-and-workload-name-specified"
+		appName           = "revision-apply-once2"
+		compName          = "revision-apply-once-comp2"
+		specifiedNameBase = "specified-name-base"
+		specifiedNameV1   = "specified-name-v1"
+		specifiedNameV2   = "specified-name-v2"
+	)
+	var (
+		ctx          = context.Background()
+		wr           v1.Deployment
+		component    v1alpha2.Component
+		appConfig    v1alpha2.ApplicationConfiguration
+		appConfigKey = client.ObjectKey{
+			Name:      appName,
+			Namespace: namespace,
+		}
+		req = reconcile.Request{NamespacedName: appConfigKey}
+		ns  = corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: namespace,
+			},
+		}
+	)
+
+	BeforeEach(func() {})
+
+	AfterEach(func() {
+		// delete the namespace with all its resources
+		Expect(k8sClient.Delete(ctx, &ns, client.PropagationPolicy(metav1.DeletePropagationForeground))).
+			Should(SatisfyAny(BeNil(), &util.NotFoundMatcher{}))
+	})
+
+	It("revision enabled should create workload with specified name protect delete with replicas larger than 0", func() {
+
+		getDeploy := func(image, name string) *v1.Deployment {
+			return &v1.Deployment{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Deployment",
+					APIVersion: "apps/v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: namespace,
+					Name:      name,
+				},
+				Spec: v1.DeploymentSpec{
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{
+						"app": compName,
+					}},
+					Template: corev1.PodTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{
+							"app": compName,
+						}},
+						Spec: corev1.PodSpec{Containers: []corev1.Container{{
+							Name:  "wordpress",
+							Image: image,
+							Ports: []corev1.ContainerPort{
+								{
+									Name:          "wordpress",
+									ContainerPort: 80,
+								},
+							},
+						},
+						}}},
+				},
+			}
+		}
+		component = v1alpha2.Component{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "core.oam.dev/v1alpha2",
+				Kind:       "Component",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      compName,
+				Namespace: namespace,
+			},
+			Spec: v1alpha2.ComponentSpec{
+				Workload: runtime.RawExtension{
+					Object: getDeploy("wordpress:4.6.1-apache", specifiedNameBase),
+				},
+			},
+		}
+		appConfig = v1alpha2.ApplicationConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      appName,
+				Namespace: namespace,
+			},
+		}
+		By("Create namespace")
+		Eventually(
+			func() error {
+				return k8sClient.Create(ctx, &ns)
+			},
+			time.Second*3, time.Millisecond*300).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{}))
+
+		By("Create Component")
+		Expect(k8sClient.Create(ctx, &component)).Should(Succeed())
+		cmpV1 := &v1alpha2.Component{}
+		Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV1)).Should(Succeed())
+
+		By("component handler will automatically create controller revision")
+		Expect(func() bool {
+			_, ok := componentHandler.createControllerRevision(cmpV1, cmpV1)
+			return ok
+		}()).Should(BeTrue())
+		var crList v1.ControllerRevisionList
+		By("Check controller revision created successfully")
+		Eventually(func() error {
+			labels := &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					ControllerRevisionComponentLabel: compName,
+				},
+			}
+			selector, err := metav1.LabelSelectorAsSelector(labels)
+			if err != nil {
+				return err
+			}
+			err = k8sClient.List(ctx, &crList, &client.ListOptions{
+				LabelSelector: selector,
+			})
+			if err != nil {
+				return err
+			}
+			if len(crList.Items) != 1 {
+				return fmt.Errorf("want only 1 revision created but got %d", len(crList.Items))
+			}
+			return nil
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+		By("Create an ApplicationConfiguration")
+		appConfig = v1alpha2.ApplicationConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      appName,
+				Namespace: namespace,
+			},
+			Spec: v1alpha2.ApplicationConfigurationSpec{Components: []v1alpha2.ApplicationConfigurationComponent{
+				{
+					RevisionName: compName + "-v1",
+				},
+			}},
+		}
+
+		By("Creat appConfig & check successfully")
+		Expect(k8sClient.Create(ctx, &appConfig)).Should(Succeed())
+		Eventually(func() error {
+			return k8sClient.Get(ctx, appConfigKey, &appConfig)
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+		By("Reconcile")
+		reconciler.applyOnceOnlyMode = "force"
+		reconcileRetry(reconciler, req)
+
+		By("Check workload created successfully")
+		var workloadKey1 = client.ObjectKey{Namespace: namespace, Name: specifiedNameBase}
+		Eventually(func() error {
+			reconcileRetry(reconciler, req)
+			return k8sClient.Get(ctx, workloadKey1, &wr)
+		}, 3*time.Second, 300*time.Millisecond).Should(BeNil())
+		By("Check workload should only have 1 generation")
+		Expect(wr.GetGeneration()).Should(BeEquivalentTo(1))
+
+		By("Check reconcile again and no error will happen")
+		reconcileRetry(reconciler, req)
+
+		Expect(k8sClient.Get(ctx, appConfigKey, &appConfig)).Should(BeNil())
+
+		By("===================================== Start to Upgrade revision of component =========================================")
+		cmpV2 := &v1alpha2.Component{}
+		Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV2)).Should(Succeed())
+		cmpV2.Spec.Workload = runtime.RawExtension{
+			Object: getDeploy("wordpress:v2", specifiedNameV1),
+		}
+		By("Update Component")
+		Expect(k8sClient.Update(ctx, cmpV2)).Should(Succeed())
+		By("component handler will automatically create a ne controller revision")
+		Expect(func() bool { _, ok := componentHandler.createControllerRevision(cmpV2, cmpV2); return ok }()).Should(BeTrue())
+		By("Check controller revision created successfully")
+		Eventually(func() error {
+			labels := &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					ControllerRevisionComponentLabel: compName,
+				},
+			}
+			selector, err := metav1.LabelSelectorAsSelector(labels)
+			if err != nil {
+				return err
+			}
+			err = k8sClient.List(ctx, &crList, &client.ListOptions{
+				LabelSelector: selector,
+			})
+			if err != nil {
+				return err
+			}
+			if len(crList.Items) != 2 {
+				return fmt.Errorf("there should be exactly 2 revision created but got %d", len(crList.Items))
+			}
+			return nil
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+		By("Update appConfig & check successfully")
+		appConfig.Spec.Components[0].RevisionName = compName + "-v2"
+
+		Expect(k8sClient.Update(ctx, &appConfig)).Should(Succeed())
+		Eventually(func() error {
+			return k8sClient.Get(ctx, appConfigKey, &appConfig)
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+		By("Reconcile for new revision")
+		reconcileRetry(reconciler, req)
+
+		By("Check new revision workload created successfully")
+		Eventually(func() error {
+			reconcileRetry(reconciler, req)
+			var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameV1}
+			return k8sClient.Get(ctx, workloadKey, &wr)
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+		By("Check the new workload should only have 1 generation")
+		Expect(wr.GetGeneration()).Should(BeEquivalentTo(1))
+		Expect(wr.Spec.Template.Spec.Containers[0].Image).Should(BeEquivalentTo("wordpress:v2"))
+
+		By("Check the new workload should only have 1 generation")
+		Expect(wr.GetGeneration()).Should(BeEquivalentTo(1))
+
+		By("Check reconcile again")
+		reconcileRetry(reconciler, req)
+
+		By("Check appconfig condition should have error")
+		Eventually(func() string {
+			reconcileRetry(reconciler, req)
+			err := k8sClient.Get(ctx, appConfigKey, &appConfig)
+			if err != nil {
+				return err.Error()
+			}
+			if len(appConfig.Status.Conditions) != 1 {
+				return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions))
+			}
+			By(fmt.Sprintf("Reconcile with condition %v", appConfig.Status.Conditions[0]))
+			return string(appConfig.Status.Conditions[0].Reason)
+		}, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileError"))
+
+		By("Check the old workload still there")
+		Eventually(func() error {
+			reconcileRetry(reconciler, req)
+			var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameBase}
+			return k8sClient.Get(ctx, workloadKey, &wr)
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+		By("Check the old workload should only have 1 generation")
+		Expect(wr.GetGeneration()).Should(BeEquivalentTo(1))
+		Expect(wr.Spec.Template.Spec.Containers[0].Image).Should(BeEquivalentTo("wordpress:4.6.1-apache"))
+
+		wr.Spec.Replicas = pointer.Int32Ptr(0)
+		Expect(k8sClient.Update(ctx, &wr)).Should(Succeed())
+
+		By("Reconcile Again and appconfig condition should not have error")
+		Eventually(func() string {
+			By("Once more Reconcile and should not have error")
+			reconcileRetry(reconciler, req)
+			err := k8sClient.Get(ctx, appConfigKey, &appConfig)
+			if err != nil {
+				return err.Error()
+			}
+			if len(appConfig.Status.Conditions) != 1 {
+				return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions))
+			}
+			return string(appConfig.Status.Conditions[0].Reason)
+		}, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileSuccess"))
+		Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: specifiedNameBase}, &wr)).Should(SatisfyAny(util.NotFoundMatcher{}))
+
+		By("===================================== Start to Upgrade revision of component again =========================================")
+		cmpV3 := &v1alpha2.Component{}
+		Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compName}, cmpV3)).Should(Succeed())
+		cmpV3.Spec.Workload = runtime.RawExtension{
+			Object: getDeploy("wordpress:v3", specifiedNameV2),
+		}
+		By("Update Component")
+		Expect(k8sClient.Update(ctx, cmpV3)).Should(Succeed())
+		By("component handler will automatically create a ne controller revision")
+		Expect(func() bool { _, ok := componentHandler.createControllerRevision(cmpV3, cmpV3); return ok }()).Should(BeTrue())
+		By("Update the AC and add the revisionEnabled Trait")
+		appConfig.Spec.Components[0].RevisionName = compName + "-v3"
+		appConfig.Spec.Components[0].Traits = []v1alpha2.ComponentTrait{
+			{Trait: runtime.RawExtension{Object: &unstructured.Unstructured{Object: map[string]interface{}{
+				"apiVersion": "example.com/v1",
+				"kind":       "Foo",
+				"metadata": map[string]interface{}{
+					"labels": map[string]interface{}{
+						"trait.oam.dev/type": "rollout-revision",
+					},
+				},
+				"spec": map[string]interface{}{
+					"key": "test3",
+				},
+			}}}},
+		}
+		Expect(k8sClient.Update(ctx, &appConfig)).Should(Succeed())
+		Eventually(func() error {
+			return k8sClient.Get(ctx, appConfigKey, &appConfig)
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+		Expect(len(appConfig.Spec.Components[0].Traits)).Should(BeEquivalentTo(1))
+
+		By("Check reconcile again and no error will happen, revisionEnabled will skip delete")
+		reconcileRetry(reconciler, req)
+		By("Check appconfig condition should not have error")
+		Eventually(func() string {
+			By("Once more Reconcile and should not have error")
+			reconcileRetry(reconciler, req)
+			err := k8sClient.Get(ctx, appConfigKey, &appConfig)
+			if err != nil {
+				return err.Error()
+			}
+			if len(appConfig.Status.Conditions) != 1 {
+				return "condition len should be 1 but now is " + strconv.Itoa(len(appConfig.Status.Conditions))
+			}
+			return string(appConfig.Status.Conditions[0].Reason)
+		}, 3*time.Second, 300*time.Millisecond).Should(BeEquivalentTo("ReconcileSuccess"))
+
+		By("Check new revision workload created successfully")
+		Eventually(func() error {
+			reconcileRetry(reconciler, req)
+			var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameV2}
+			return k8sClient.Get(ctx, workloadKey, &wr)
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+		By("Check the new workload should only have 1 generation")
+		Expect(wr.GetGeneration()).Should(BeEquivalentTo(1))
+		Expect(wr.Spec.Template.Spec.Containers[0].Image).Should(BeEquivalentTo("wordpress:v3"))
+		By("Check the new workload should only have 1 generation")
+		Expect(wr.GetGeneration()).Should(BeEquivalentTo(1))
+		By("Check the old workload still there")
+		Eventually(func() error {
+			reconcileRetry(reconciler, req)
+			var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameV1}
+			return k8sClient.Get(ctx, workloadKey, &wr)
+		}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+		reconciler.applyOnceOnlyMode = "off"
+	})
+
+})