propagate AppRevision Into context.AppRevision (#1323)

* refactor: calculate revision first before render AC and Components

* fix flaky e2e case

* add e2e test

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

@@ -102,6 +102,16 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 
 	app.Status.SetConditions(readyCondition("Parsed"))
 	handler.appfile = generatedAppfile
+
+	appRev, err := handler.GenerateAppRevision(ctx)
+	if err != nil {
+		applog.Error(err, "[Handle Calculate Revision]")
+		app.Status.SetConditions(errorCondition("Parsed", err))
+		return handler.handleErr(err)
+	}
+	// Record the revision so it can be used to render data in context.appRevision
+	generatedAppfile.RevisionName = appRev.Name
+
 	applog.Info("build template")
 	// build template to applicationconfig & component
 	ac, comps, err := appParser.GenerateApplicationConfiguration(generatedAppfile, app.Namespace)
@@ -110,14 +120,13 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 		app.Status.SetConditions(errorCondition("Built", err))
 		return handler.handleErr(err)
 	}
-
 	// pass the App label and annotation to ac except some app specific ones
 	oamutil.PassLabelAndAnnotation(app, ac)
 
 	app.Status.SetConditions(readyCondition("Built"))
 	applog.Info("apply application revision & component to the cluster")
 	// apply application revision & component to the cluster
-	if err := handler.apply(ctx, ac, comps); err != nil {
+	if err := handler.apply(ctx, appRev, ac, comps); err != nil {
 		applog.Error(err, "[Handle apply]")
 		app.Status.SetConditions(errorCondition("Applied", err))
 		return handler.handleErr(err)

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

@@ -26,6 +26,8 @@ import (
 	"strconv"
 	"time"
 
+	common2 "github.com/oam-dev/kubevela/pkg/utils/common"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
@@ -1322,6 +1324,58 @@ var _ = Describe("Test Application Controller", func() {
 		By("Delete Application, clean the resource")
 		Expect(k8sClient.Delete(ctx, appImportPkg)).Should(BeNil())
 	})
+	It("revision should exist in created workload render by context.appRevision", func() {
+
+		expDeployment := getExpDeployment("myweb", "revision-app1")
+		expDeployment.Labels["workload.oam.dev/type"] = "cd1"
+		expDeployment.Spec.Template.Spec.Containers[0].Command = nil
+		expDeployment.Spec.Template.Labels["app.oam.dev/revision"] = "revision-app1-v1"
+		ns := &corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "vela-test-app-revisionname",
+			},
+		}
+		Expect(k8sClient.Create(ctx, ns)).Should(BeNil())
+
+		cd := &v1beta1.ComponentDefinition{}
+		Expect(common2.ReadYamlToObject("testdata/revision/cd1.yaml", cd)).Should(BeNil())
+		cd.SetNamespace(ns.Name)
+		Expect(k8sClient.Create(ctx, cd.DeepCopyObject())).Should(BeNil())
+
+		app := &v1beta1.Application{}
+		Expect(common2.ReadYamlToObject("testdata/revision/app1.yaml", app)).Should(BeNil())
+		app.SetNamespace(ns.Name)
+		Expect(k8sClient.Create(ctx, app.DeepCopyObject())).Should(BeNil())
+
+		appKey := client.ObjectKey{
+			Name:      app.Name,
+			Namespace: app.Namespace,
+		}
+		reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey})
+		By("Check Application Created with the correct revision")
+		curApp := &v1beta1.Application{}
+		Expect(k8sClient.Get(ctx, appKey, curApp)).Should(BeNil())
+		Expect(curApp.Status.Phase).Should(Equal(common.ApplicationRunning))
+		Expect(curApp.Status.LatestRevision).ShouldNot(BeNil())
+		Expect(curApp.Status.LatestRevision.Revision).Should(BeEquivalentTo(1))
+
+		By("Check Component Created with the expected workload spec")
+		var component v1alpha2.Component
+		Expect(k8sClient.Get(ctx, client.ObjectKey{
+			Namespace: app.Namespace,
+			Name:      "myweb",
+		}, &component)).Should(BeNil())
+		Expect(component.ObjectMeta.Labels).Should(BeEquivalentTo(map[string]string{oam.LabelAppName: app.Name}))
+		Expect(component.Status.LatestRevision).ShouldNot(BeNil())
+
+		// check the workload created should be the same as the raw data in the component
+		gotD := &v1.Deployment{}
+		Expect(json.Unmarshal(component.Spec.Workload.Raw, gotD)).Should(BeNil())
+		fmt.Println(cmp.Diff(expDeployment, gotD))
+		Expect(assert.ObjectsAreEqual(expDeployment, gotD)).Should(BeEquivalentTo(true))
+		By("Delete Application, clean the resource")
+		Expect(k8sClient.Delete(ctx, app)).Should(BeNil())
+	})
 })
 
 func reconcileRetry(r reconcile.Reconciler, req reconcile.Request) {

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

@@ -65,11 +65,13 @@ func readyCondition(tpy string) runtimev1alpha1.Condition {
 }
 
 type appHandler struct {
-	r       *Reconciler
-	app     *v1beta1.Application
-	appfile *appfile.Appfile
-	logger  logr.Logger
-	inplace bool
+	r             *Reconciler
+	app           *v1beta1.Application
+	appfile       *appfile.Appfile
+	logger        logr.Logger
+	inplace       bool
+	isNewRevision bool
+	revisionHash  string
 }
 
 // setInplace will mark if the application should upgrade the workload within the same instance(name never changed)
@@ -95,7 +97,7 @@ func (h *appHandler) handleErr(err error) (ctrl.Result, error) {
 // 2. update AC's components using the component revision name
 // 3. update or create the AC with new revision and remember it in the application status
 // 4. garbage collect unused components
-func (h *appHandler) apply(ctx context.Context, ac *v1alpha2.ApplicationConfiguration, comps []*v1alpha2.Component) error {
+func (h *appHandler) apply(ctx context.Context, appRev *v1beta1.ApplicationRevision, ac *v1alpha2.ApplicationConfiguration, comps []*v1alpha2.Component) error {
 	owners := []metav1.OwnerReference{{
 		APIVersion: v1beta1.SchemeGroupVersion.String(),
 		Kind:       v1beta1.ApplicationKind,
@@ -128,16 +130,22 @@ func (h *appHandler) apply(ctx context.Context, ac *v1alpha2.ApplicationConfigur
 		}
 	}
 	ac.SetOwnerReferences(owners)
-	isNewRevision, appRev, err := h.GenerateRevision(ctx, ac, comps)
-	if err != nil {
-		return errors.Wrap(err, "cannot generate a revision of the application")
-	}
-	if isNewRevision {
+	h.FinalizeAppRevision(appRev, ac, comps)
+
+	var err error
+	if h.isNewRevision {
+		var revisionNum int64
+		appRev.Name, revisionNum = utils.GetAppNextRevision(h.app)
+		// only new revision update the status
+		if err = h.UpdateRevisionStatus(ctx, appRev.Name, h.revisionHash, revisionNum); err != nil {
+			return err
+		}
 		if err = h.r.Create(ctx, appRev); err != nil {
 			return err
 		}
 	} else {
-		if err = h.r.Update(ctx, appRev); err != nil {
+		err = h.r.Update(ctx, appRev)
+		if err != nil {
 			return err
 		}
 	}

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

@@ -42,27 +42,30 @@ type AppRevisionHash struct {
 	ScopeDefinitionHash     map[string]string
 }
 
-// GenerateRevision will generate revision for an Application when created/updated
-func (h *appHandler) GenerateRevision(ctx context.Context, ac *v1alpha2.ApplicationConfiguration,
-	comps []*v1alpha2.Component) (bool, *v1beta1.ApplicationRevision, error) {
-	copiedApp := h.app.DeepCopy()
-	// We better to remove all object status in the appRevision
-	copiedApp.Status = common.AppStatus{}
-	appRev := &v1beta1.ApplicationRevision{
-		Spec: v1beta1.ApplicationRevisionSpec{
-			Application:              *copiedApp,
-			Components:               ConvertComponent2RawRevision(comps),
-			ApplicationConfiguration: util.Object2RawExtension(ac),
-			ComponentDefinitions:     make(map[string]v1beta1.ComponentDefinition),
-			WorkloadDefinitions:      make(map[string]v1beta1.WorkloadDefinition),
-			TraitDefinitions:         make(map[string]v1beta1.TraitDefinition),
-			ScopeDefinitions:         make(map[string]v1beta1.ScopeDefinition),
-		},
+// UpdateRevisionStatus will update the status of Application object mainly for update the revision part
+func (h *appHandler) UpdateRevisionStatus(ctx context.Context, revName, hash string, revision int64) error {
+	h.app.Status.LatestRevision = &common.Revision{
+		Name:         revName,
+		Revision:     revision,
+		RevisionHash: hash,
 	}
-	// appRev should have the same annotation/label as the app
+	// make sure that we persist the latest revision first
+	if err := h.r.UpdateStatus(ctx, h.app); err != nil {
+		h.logger.Error(err, "update the latest appConfig revision to status", "application name", h.app.GetName(),
+			"latest revision", revName)
+		return err
+	}
+	h.logger.Info("recorded the latest appConfig revision", "application name", h.app.GetName(),
+		"latest revision", revName)
+	return nil
+}
+
+// setRevisionMetadata will set the ApplicationRevision with the same annotation/label as the app
+func (h *appHandler) setRevisionMetadata(appRev *v1beta1.ApplicationRevision) {
 	appRev.Namespace = h.app.Namespace
 	appRev.SetAnnotations(h.app.GetAnnotations())
 	appRev.SetLabels(h.app.GetLabels())
+	util.AddLabels(appRev, map[string]string{oam.LabelAppRevisionHash: h.revisionHash})
 	appRev.SetOwnerReferences([]metav1.OwnerReference{{
 		APIVersion: v1beta1.SchemeGroupVersion.String(),
 		Kind:       v1beta1.ApplicationKind,
@@ -70,6 +73,31 @@ func (h *appHandler) GenerateRevision(ctx context.Context, ac *v1alpha2.Applicat
 		UID:        h.app.UID,
 		Controller: pointer.BoolPtr(false),
 	}})
+}
+
+// setRevisionWithRenderedResult will set the ApplicationRevision with the rendered result
+// it's ApplicationConfiguration and Component for now
+func (h *appHandler) setRevisionWithRenderedResult(appRev *v1beta1.ApplicationRevision, ac *v1alpha2.ApplicationConfiguration,
+	comps []*v1alpha2.Component) {
+	appRev.Spec.Components = ConvertComponent2RawRevision(comps)
+	appRev.Spec.ApplicationConfiguration = util.Object2RawExtension(ac)
+}
+
+// gatherRevisionSpec will gather all revision spec withouth metadata and rendered result.
+// the gathered Revision spec will be enough to calculate the hash and compare with the old revision
+func (h *appHandler) gatherRevisionSpec() (*v1beta1.ApplicationRevision, string, error) {
+	copiedApp := h.app.DeepCopy()
+	// We better to remove all object status in the appRevision
+	copiedApp.Status = common.AppStatus{}
+	appRev := &v1beta1.ApplicationRevision{
+		Spec: v1beta1.ApplicationRevisionSpec{
+			Application:          *copiedApp,
+			ComponentDefinitions: make(map[string]v1beta1.ComponentDefinition),
+			WorkloadDefinitions:  make(map[string]v1beta1.WorkloadDefinition),
+			TraitDefinitions:     make(map[string]v1beta1.TraitDefinition),
+			ScopeDefinitions:     make(map[string]v1beta1.ScopeDefinition),
+		},
+	}
 	for _, w := range h.appfile.Workloads {
 		if w == nil {
 			continue
@@ -94,43 +122,74 @@ func (h *appHandler) GenerateRevision(ctx context.Context, ac *v1alpha2.Applicat
 				appRev.Spec.TraitDefinitions[t.FullTemplate.TraitDefinition.Name] = *td
 			}
 		}
+		// TODO(wonderflow): take scope into the revision
 	}
 	appRevisionHash, err := ComputeAppRevisionHash(appRev)
 	if err != nil {
-		return false, nil, err
+		h.logger.Error(err, "compute hash of appRevision for application", "application name", h.app.GetName())
+		return appRev, "", err
 	}
-	util.AddLabels(appRev, map[string]string{oam.LabelAppRevisionHash: appRevisionHash})
+	return appRev, appRevisionHash, nil
+}
 
-	// check if the appRevision is different from the existing one
-	if h.app.Status.LatestRevision != nil && h.app.Status.LatestRevision.RevisionHash == appRevisionHash {
-		// get the last revision and double check
-		lastAppRevision := &v1beta1.ApplicationRevision{}
-		if err := h.r.Get(ctx, client.ObjectKey{Name: h.app.Status.LatestRevision.Name,
-			Namespace: h.app.Namespace}, lastAppRevision); err != nil {
-			return false, nil, errors.Wrapf(err, "fail to get applicationRevision %s", h.app.Status.LatestRevision.Name)
-		}
-		if DeepEqualRevision(lastAppRevision, appRev) {
-			// No difference on spec, will not create a new revision
-			appRev.Name = lastAppRevision.Name
-			appRev.ResourceVersion = lastAppRevision.ResourceVersion
-			return false, appRev, nil
-		}
+// compareWithLastRevisionSpec will get the last AppRevision from K8s and compare the Application and Definition's Spec
+func (h *appHandler) compareWithLastRevisionSpec(ctx context.Context, newAppRevisionHash string, newAppRevision *v1beta1.ApplicationRevision) (bool, error) {
+
+	// the last revision doesn't exist.
+	if h.app.Status.LatestRevision == nil {
+		return true, nil
 	}
-	// need to create a new appRev
-	var revision int64
-	appRev.Name, revision = utils.GetAppNextRevision(h.app)
-	h.app.Status.LatestRevision = &common.Revision{
-		Name:         appRev.Name,
-		Revision:     revision,
-		RevisionHash: appRevisionHash,
+	// the hash value doesn't align
+	if h.app.Status.LatestRevision.RevisionHash != newAppRevisionHash {
+		return true, nil
 	}
-	// make sure that we persist the latest revision first
-	if err = h.r.UpdateStatus(ctx, h.app); err != nil {
-		return false, nil, err
+
+	// check if the appRevision is deep equal in Spec level
+	// get the last revision from K8s and double check
+	lastAppRevision := &v1beta1.ApplicationRevision{}
+	if err := h.r.Get(ctx, client.ObjectKey{Name: h.app.Status.LatestRevision.Name,
+		Namespace: h.app.Namespace}, lastAppRevision); err != nil {
+		h.logger.Error(err, "get the last appRevision from K8s", "application name",
+			h.app.GetName(), "revision", h.app.Status.LatestRevision.Name)
+		return false, errors.Wrapf(err, "fail to get applicationRevision %s", h.app.Status.LatestRevision.Name)
 	}
-	h.logger.Info("recorded the latest appConfig revision", "application name", h.app.GetName(),
-		"latest revision", appRev.Name)
-	return true, appRev, nil
+	if DeepEqualRevision(lastAppRevision, newAppRevision) {
+		// No difference on spec, will not create a new revision
+		// align the name and resourceVersion
+		newAppRevision.Name = lastAppRevision.Name
+		newAppRevision.ResourceVersion = lastAppRevision.ResourceVersion
+		return false, nil
+	}
+	// if reach here, it's same hash but different spec
+	return true, nil
+}
+
+// GenerateAppRevision will generate a pure revision without metadata and rendered result
+// the generated revision will be compare with the last revision to see if there's any difference.
+func (h *appHandler) GenerateAppRevision(ctx context.Context) (*v1beta1.ApplicationRevision, error) {
+	appRev, appRevisionHash, err := h.gatherRevisionSpec()
+	if err != nil {
+		return nil, err
+	}
+	isNewRev, err := h.compareWithLastRevisionSpec(ctx, appRevisionHash, appRev)
+	if err != nil {
+		return appRev, err
+	}
+	if isNewRev {
+		appRev.Name, _ = utils.GetAppNextRevision(h.app)
+	}
+	h.isNewRevision = isNewRev
+	h.revisionHash = appRevisionHash
+	return appRev, nil
+}
+
+// FinalizeAppRevision will finalize the AppRevision with metadata and rendered result revision for an Application when created/updated
+func (h *appHandler) FinalizeAppRevision(appRev *v1beta1.ApplicationRevision,
+	ac *v1alpha2.ApplicationConfiguration, comps []*v1alpha2.Component) {
+
+	h.setRevisionMetadata(appRev)
+	h.setRevisionWithRenderedResult(appRev, ac, comps)
+
 }
 
 // ConvertComponent2RawRevision convert to ComponentMap
@@ -145,8 +204,9 @@ func ConvertComponent2RawRevision(comps []*v1alpha2.Component) []common.RawCompo
 	return objs
 }
 
-// DeepEqualRevision will check the Application and Definition to see if the Application is the same revision
-// AC and component are generated by the application and definitions
+// DeepEqualRevision will compare the spec of Application and Definition to see if the Application is the same revision
+// Spec of AC and Component will not be compared as they are generated by the application and definitions
+// Note the Spec compare can only work when the RawExtension are decoded well in the RawExtension.Object instead of in RawExtension.Raw(bytes)
 func DeepEqualRevision(old, new *v1beta1.ApplicationRevision) bool {
 	if len(old.Spec.WorkloadDefinitions) != len(new.Spec.WorkloadDefinitions) {
 		return false
@@ -184,6 +244,7 @@ func DeepEqualRevision(old, new *v1beta1.ApplicationRevision) bool {
 }
 
 // ComputeAppRevisionHash computes a single hash value for an appRevision object
+// Spec of Application/WorkloadDefinitions/ComponentDefinitions/TraitDefinitions/ScopeDefinitions will be taken into compute
 func ComputeAppRevisionHash(appRevision *v1beta1.ApplicationRevision) (string, error) {
 	// we first constructs a AppRevisionHash structure to store all the meaningful spec hashes
 	// and avoid computing the annotations. Those fields are all read from k8s already so their

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

@@ -230,7 +230,9 @@ var _ = Describe("test generate revision ", func() {
 		Expect(err).Should(Succeed())
 		handler.appfile = generatedAppfile
 		Expect(ac.Namespace).Should(Equal(app.Namespace))
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err := handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 
 		curApp := &v1beta1.Application{}
 		Eventually(
@@ -275,7 +277,9 @@ var _ = Describe("test generate revision ", func() {
 		annoKey2 := "testKey2"
 		app.SetAnnotations(map[string]string{annoKey2: "true"})
 		lastRevision := curApp.Status.LatestRevision.Name
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err = handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 		Eventually(
 			func() error {
 				return handler.r.Get(ctx,
@@ -322,7 +326,9 @@ var _ = Describe("test generate revision ", func() {
 		Expect(err).Should(Succeed())
 		handler.appfile = generatedAppfile
 		handler.app = &app
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err = handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 		Eventually(
 			func() error {
 				return handler.r.Get(ctx,
@@ -372,7 +378,9 @@ var _ = Describe("test generate revision ", func() {
 		Expect(err).Should(Succeed())
 		handler.appfile = generatedAppfile
 		Expect(ac.Namespace).Should(Equal(app.Namespace))
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err := handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 		curApp := &v1beta1.Application{}
 		Eventually(
 			func() error {
@@ -415,7 +423,9 @@ var _ = Describe("test generate revision ", func() {
 		annoKey2 := "testKey2"
 		app.SetAnnotations(map[string]string{annoKey2: "true"})
 		lastRevision := curApp.Status.LatestRevision.Name
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err = handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 		Eventually(
 			func() error {
 				return handler.r.Get(ctx,
@@ -463,7 +473,9 @@ var _ = Describe("test generate revision ", func() {
 		Expect(err).Should(Succeed())
 		handler.appfile = generatedAppfile
 		handler.app = &app
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err = handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 		Eventually(
 			func() error {
 				return handler.r.Get(ctx,
@@ -514,7 +526,9 @@ var _ = Describe("test generate revision ", func() {
 		Expect(err).Should(Succeed())
 		handler.appfile = generatedAppfile
 		Expect(ac.Namespace).Should(Equal(app.Namespace))
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err := handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 
 		curApp := &v1beta1.Application{}
 		Eventually(
@@ -543,7 +557,9 @@ var _ = Describe("test generate revision ", func() {
 		labelKey2 := "labelKey2"
 		app.SetLabels(map[string]string{labelKey2: "true"})
 		lastRevision := curApp.Status.LatestRevision.Name
-		Expect(handler.apply(context.Background(), ac, comps)).Should(Succeed())
+		appRev, err = handler.GenerateAppRevision(ctx)
+		Expect(err).Should(Succeed())
+		Expect(handler.apply(context.Background(), appRev, ac, comps)).Should(Succeed())
 		Eventually(
 			func() error {
 				return handler.r.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: app.Name}, curApp)

pkg/controller/core.oam.dev/v1alpha2/application/testdata/revision/app1.yaml

@@ -0,0 +1,10 @@
+apiVersion: core.oam.dev/v1beta1
+kind: Application
+metadata:
+  name: revision-app1
+spec:
+  components:
+    - name: myweb
+      type: cd1
+      properties:
+        image: busybox

pkg/controller/core.oam.dev/v1alpha2/application/testdata/revision/cd1.yaml

@@ -0,0 +1,39 @@
+apiVersion: core.oam.dev/v1beta1
+kind: ComponentDefinition
+metadata:
+  name: cd1
+  namespace: vela-system
+spec:
+  workload:
+    definition:
+      apiVersion: apps/v1
+      kind: Deployment
+  schematic:
+    cue:
+      template: |
+        output: {
+        	apiVersion: "apps/v1"
+        	kind:       "Deployment"
+        	spec: {
+        		selector: matchLabels: {
+        			"app.oam.dev/component": context.name
+        		}
+
+        		template: {
+        			metadata: labels: {
+        				"app.oam.dev/component": context.name
+                        "app.oam.dev/revision": context.appRevision
+        			}
+        			spec: {
+        				containers: [{
+        					name:  context.name
+        					image: parameter.image
+        				}]
+        			}
+        		}
+        	}
+        }
+
+        parameter: {
+        	image: string
+        }