Merge pull request #1114 from wonderflow/fix1

update CRD and fix workload not create first time

apis/core.oam.dev/v1alpha2/core_types.go

@@ -412,6 +412,16 @@ type WorkloadTrait struct {
 
 	// Message will allow controller to leave some additional information for this trait
 	Message string `json:"message,omitempty"`
+
+	// AppliedGeneration indicates the generation observed by the appConfig controller.
+	// The same field is also recorded in the annotations of traits.
+	// A trait is possible to be deleted from cluster after created.
+	// This field is useful to track the observed generation of traits after they are
+	// deleted.
+	AppliedGeneration int64 `json:"appliedGeneration,omitempty"`
+
+	// DependencyUnsatisfied notify does the trait has dependency unsatisfied
+	DependencyUnsatisfied bool `json:"dependencyUnsatisfied,omitempty"`
 }
 
 // A ScopeStatus represents the state of a scope.
@@ -442,12 +452,8 @@ type WorkloadStatus struct {
 	// DependencyUnsatisfied notify does the workload has dependency unsatisfied
 	DependencyUnsatisfied bool `json:"dependencyUnsatisfied,omitempty"`
 
-	// AppliedGeneration indicates the generation observed by the appConfig controller.
-	// The same field is also recorded in the annotations of workloads.
-	// A workload is possible to be deleted from cluster after created.
-	// This field is useful to track the observed generation of workloads after they are
-	// deleted.
-	AppliedGeneration int64 `json:"appliedGeneration,omitempty"`
+	// AppliedComponentRevision indicates the applied component revision name of this workload
+	AppliedComponentRevision string `json:"appliedComponentRevision,omitempty"`
 
 	// Reference to a workload created by an ApplicationConfiguration.
 	Reference runtimev1alpha1.TypedReference `json:"workloadRef,omitempty"`

charts/vela-core/crds/core.oam.dev_applicationconfigurations.yaml

@@ -380,10 +380,9 @@ spec:
                 items:
                   description: A WorkloadStatus represents the status of a workload.
                   properties:
-                    appliedGeneration:
-                      description: AppliedGeneration indicates the generation observed by the appConfig controller. The same field is also recorded in the annotations of workloads. A workload is possible to be deleted from cluster after created. This field is useful to track the observed generation of workloads after they are deleted.
-                      format: int64
-                      type: integer
+                    appliedComponentRevision:
+                      description: AppliedComponentRevision indicates the applied component revision name of this workload
+                      type: string
                     componentName:
                       description: ComponentName that produced this workload.
                       type: string
@@ -433,6 +432,13 @@ spec:
                       items:
                         description: A WorkloadTrait represents a trait associated with a workload and its status
                         properties:
+                          appliedGeneration:
+                            description: AppliedGeneration indicates the generation observed by the appConfig controller. The same field is also recorded in the annotations of traits. A trait is possible to be deleted from cluster after created. This field is useful to track the observed generation of traits after they are deleted.
+                            format: int64
+                            type: integer
+                          dependencyUnsatisfied:
+                            description: DependencyUnsatisfied notify does the trait has dependency unsatisfied
+                            type: boolean
                           message:
                             description: Message will allow controller to leave some additional information for this trait
                             type: string

legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationconfigurations.yaml

@@ -380,10 +380,9 @@ spec:
               items:
                 description: A WorkloadStatus represents the status of a workload.
                 properties:
-                  appliedGeneration:
-                    description: AppliedGeneration indicates the generation observed by the appConfig controller. The same field is also recorded in the annotations of workloads. A workload is possible to be deleted from cluster after created. This field is useful to track the observed generation of workloads after they are deleted.
-                    format: int64
-                    type: integer
+                  appliedComponentRevision:
+                    description: AppliedComponentRevision indicates the applied component revision name of this workload
+                    type: string
                   componentName:
                     description: ComponentName that produced this workload.
                     type: string
@@ -433,6 +432,13 @@ spec:
                     items:
                       description: A WorkloadTrait represents a trait associated with a workload and its status
                       properties:
+                        appliedGeneration:
+                          description: AppliedGeneration indicates the generation observed by the appConfig controller. The same field is also recorded in the annotations of traits. A trait is possible to be deleted from cluster after created. This field is useful to track the observed generation of traits after they are deleted.
+                          format: int64
+                          type: integer
+                        dependencyUnsatisfied:
+                          description: DependencyUnsatisfied notify does the trait has dependency unsatisfied
+                          type: boolean
                         message:
                           description: Message will allow controller to leave some additional information for this trait
                           type: string

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

@@ -392,8 +392,13 @@ func updateObservedGeneration(ac *v1alpha2.ApplicationConfiguration) {
 	}
 	for i, w := range ac.Status.Workloads {
 		// only the workload meet all dependency can mean the generation applied successfully
-		if w.AppliedGeneration != ac.Generation && !w.DependencyUnsatisfied {
-			ac.Status.Workloads[i].AppliedGeneration = ac.GetGeneration()
+		if w.AppliedComponentRevision != w.ComponentRevisionName && !w.DependencyUnsatisfied {
+			ac.Status.Workloads[i].AppliedComponentRevision = w.ComponentRevisionName
+		}
+		for j, t := range w.Traits {
+			if t.AppliedGeneration != ac.Generation && !t.DependencyUnsatisfied {
+				ac.Status.Workloads[i].Traits[j].AppliedGeneration = ac.Generation
+			}
 		}
 	}
 }
@@ -501,6 +506,7 @@ func (w Workload) Status() v1alpha2.WorkloadStatus {
 			Kind:       w.Traits[i].Object.GetKind(),
 			Name:       w.Traits[i].Object.GetName(),
 		}
+		acw.Traits[i].DependencyUnsatisfied = tr.HasDep
 	}
 	for i, s := range w.Scopes {
 		acw.Scopes[i].Reference = v1alpha1.TypedReference{
@@ -619,47 +625,49 @@ func applyOnceOnly(ac *v1alpha2.ApplicationConfiguration, mode core.ApplyOnceOnl
 			}
 
 			createdBefore := false
+			var appliedRevision, appliedGeneration string
 			for _, w := range ac.Status.Workloads {
 				// traverse recorded workloads to find the one matching applied resource
 				if w.Reference.GetObjectKind().GroupVersionKind() == desired.GetObjectKind().GroupVersionKind() &&
 					w.Reference.Name == d.GetName() {
 					// the workload matches applied resource
 					createdBefore = true
-
 					// for workload, when revision enabled, only when revision changed that can trigger to create a new one
-					if dLabels[oam.LabelOAMResourceType] == oam.ResourceTypeWorkload && w.ComponentRevisionName == dLabels[oam.LabelAppComponentRevision] {
+					if dLabels[oam.LabelOAMResourceType] == oam.ResourceTypeWorkload && w.AppliedComponentRevision == dLabels[oam.LabelAppComponentRevision] {
 						// the revision is not changed, so return an error to abort creating it
 						return &GenerationUnchanged{}
 					}
+					appliedRevision = w.AppliedComponentRevision
+					break
 				}
-				if !createdBefore {
-					// the workload is not matched, then traverse its traits to find matching one
-					for _, t := range w.Traits {
-						if t.Reference.GetObjectKind().GroupVersionKind() == desired.GetObjectKind().GroupVersionKind() &&
-							t.Reference.Name == d.GetName() {
-							// the trait matches applied resource
-							createdBefore = true
+				// the workload is not matched, then traverse its traits to find matching one
+				for _, t := range w.Traits {
+					if t.Reference.GetObjectKind().GroupVersionKind() == desired.GetObjectKind().GroupVersionKind() &&
+						t.Reference.Name == d.GetName() {
+						// the trait matches applied resource
+						createdBefore = true
+						// the resource was created before and appConfig status recorded the resource version applied
+						// if recorded AppliedGeneration and ComponentRevisionName both equal to the applied resource's,
+						// that means its spec is not changed
+						if dLabels[oam.LabelOAMResourceType] == oam.ResourceTypeTrait &&
+							w.ComponentRevisionName == dLabels[oam.LabelAppComponentRevision] &&
+							strconv.FormatInt(t.AppliedGeneration, 10) == dAnnots[oam.AnnotationAppGeneration] {
+							// the revision is not changed, so return an error to abort creating it
+							return &GenerationUnchanged{}
 						}
+						appliedGeneration = strconv.FormatInt(t.AppliedGeneration, 10)
+						break
 					}
 				}
-				// don't use if-else here because it will miss the case that the resource is a trait
-				if createdBefore {
-					// the resource was created before and appConfig status recorded the resource version applied
-					// if recorded AppliedGeneration and ComponentRevisionName both equal to the applied resource's,
-					// that means its spec is not changed
-					if (strconv.Itoa(int(w.AppliedGeneration)) != dAnnots[oam.AnnotationAppGeneration]) ||
-						(w.ComponentRevisionName != dLabels[oam.LabelAppComponentRevision]) {
-						log.Info("apply only once with mode: "+string(mode)+", but condition not meet, will create new",
-							oam.AnnotationAppGeneration, strconv.Itoa(int(w.AppliedGeneration))+"/"+dAnnots[oam.AnnotationAppGeneration],
-							oam.LabelAppComponentRevision, w.ComponentRevisionName+"/"+dLabels[oam.LabelAppComponentRevision])
-						// its spec is changed, so re-create the resource
-						return nil
-					}
-					// its spec is not changed, so return an error to abort creating it
-					return &GenerationUnchanged{}
-				}
 			}
-			log.Info("apply only once with mode: force, but resource not created before, will create new", "appConfig", ac.Name)
+			var message = "apply only once with mode: force, but resource not created before, will create new"
+			if createdBefore {
+				message = "apply only once with mode: force, but resource updated, will create new"
+			}
+			log.Info(message, "appConfig", ac.Name, "gvk", desired.GetObjectKind().GroupVersionKind(), "name", d.GetName(),
+				"resourceType", dLabels[oam.LabelOAMResourceType], "appliedCompRevision", appliedRevision, "labeledCompRevision", dLabels[oam.LabelAppComponentRevision],
+				"appliedGeneration", appliedGeneration, "labeledGeneration", dAnnots[oam.AnnotationAppGeneration])
+
 			// no recorded workloads nor traits matches the applied resource
 			// that means the resource is not created before, so create it
 			return nil

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

@@ -4,6 +4,9 @@ import (
 	"context"
 	"time"
 
+	"github.com/crossplane/crossplane-runtime/pkg/logging"
+	ctrl "sigs.k8s.io/controller-runtime"
+
 	"k8s.io/apimachinery/pkg/types"
 
 	. "github.com/onsi/ginkgo"
@@ -292,6 +295,8 @@ var _ = Describe("Test apply (workloads/traits) once only", func() {
 	When("ApplyOnceOnlyForce is enabled", func() {
 		It("tests the situation where workload is not applied at the first because of unsatisfied dependency",
 			func() {
+				componentHandler := &ComponentHandler{Client: k8sClient, RevisionLimit: 100, Logger: logging.NewLogrLogger(ctrl.Log.WithName("component-handler"))}
+
 				By("Enable ApplyOnceOnlyForce")
 				reconciler.applyOnceOnlyMode = core.ApplyOnceOnlyForce
 
@@ -360,6 +365,13 @@ var _ = Describe("Test apply (workloads/traits) once only", func() {
 				cmp := &v1alpha2.Component{}
 				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compInName}, cmp)).Should(Succeed())
 
+				cmpV1 := cmp.DeepCopy()
+				By("component handler will automatically create controller revision")
+				Expect(func() bool {
+					_, ok := componentHandler.createControllerRevision(cmpV1, cmpV1)
+					return ok
+				}()).Should(BeTrue())
+
 				By("Creat appConfig & check successfully")
 				Expect(k8sClient.Create(ctx, &acWithDep)).Should(Succeed())
 				Eventually(func() error {
@@ -390,21 +402,37 @@ var _ = Describe("Test apply (workloads/traits) once only", func() {
 				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: outName}, outputTrait)).Should(Succeed())
 				err := unstructured.SetNestedField(outputTrait.Object, "test", "status", "key")
 				Expect(err).Should(BeNil())
+				Expect(k8sClient.Status().Update(ctx, outputTrait)).Should(Succeed())
+				Eventually(func() string {
+					k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: outName}, outputTrait)
+					data, _, _ := unstructured.NestedString(outputTrait.Object, "status", "key")
+					return data
+				}, 3*time.Second, time.Second).Should(Equal("test"))
+
+				By("Reconcile & check ac is satisfied")
+				Eventually(func() []v1alpha2.UnstaifiedDependency {
+					reconcileRetry(reconciler, reqDep)
+					acWithDep = v1alpha2.ApplicationConfiguration{}
+					if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: acWithDepName}, &acWithDep); err != nil {
+						return []v1alpha2.UnstaifiedDependency{{Reason: err.Error()}}
+					}
+					return acWithDep.Status.Dependency.Unsatisfied
+				}, time.Second, 300*time.Millisecond).Should(BeNil())
 
 				By("Reconcile & check workload is created")
 				Eventually(func() error {
 					reconcileRetry(reconciler, reqDep)
 					// the workload is created now because its dependency is satisfied
 					workloadIn := tempFoo.DeepCopy()
-					return k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: outName}, workloadIn)
+					return k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: inName}, workloadIn)
 				}, time.Second, 300*time.Millisecond).Should(BeNil())
 
 				By("Delete the workload")
 				recreatedWL := tempFoo.DeepCopy()
-				recreatedWL.SetName(outName)
+				recreatedWL.SetName(inName)
 				Expect(k8sClient.Delete(ctx, recreatedWL)).Should(Succeed())
 				outputTrait = tempFoo.DeepCopy()
-				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: outName}, outputTrait)).Should(util.NotFoundMatcher{})
+				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: inName}, outputTrait)).Should(util.NotFoundMatcher{})
 
 				By("Reconcile")
 				Expect(func() error { _, err := reconciler.Reconcile(req); return err }()).Should(BeNil())
@@ -415,6 +443,154 @@ var _ = Describe("Test apply (workloads/traits) once only", func() {
 				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: inName}, inputWorkload)).Should(util.NotFoundMatcher{})
 			})
 
+		It("tests the situation where workload is not applied at the first because of unsatisfied dependency and revision specified",
+			func() {
+				componentHandler := &ComponentHandler{Client: k8sClient, RevisionLimit: 100, Logger: logging.NewLogrLogger(ctrl.Log.WithName("component-handler"))}
+
+				By("Enable ApplyOnceOnlyForce")
+				reconciler.applyOnceOnlyMode = core.ApplyOnceOnlyForce
+
+				tempFoo := &unstructured.Unstructured{}
+				tempFoo.SetAPIVersion("example.com/v1")
+				tempFoo.SetKind("Foo")
+				tempFoo.SetNamespace(namespace)
+
+				inputWorkload := &unstructured.Unstructured{}
+				inputWorkload.SetAPIVersion("example.com/v1")
+				inputWorkload.SetKind("Foo")
+				inputWorkload.SetNamespace(namespace)
+
+				compInName := "comp-in-revision"
+				compIn := v1alpha2.Component{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      compInName,
+						Namespace: namespace,
+					},
+					Spec: v1alpha2.ComponentSpec{
+						Workload: runtime.RawExtension{
+							Object: inputWorkload,
+						},
+					},
+				}
+
+				outName := "data-output"
+				outputTrait := tempFoo.DeepCopy()
+				outputTrait.SetName(outName)
+
+				acWithDepName := "ac-dep"
+				acWithDep := v1alpha2.ApplicationConfiguration{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      acWithDepName,
+						Namespace: namespace,
+					},
+					Spec: v1alpha2.ApplicationConfigurationSpec{
+						Components: []v1alpha2.ApplicationConfigurationComponent{
+							{
+								RevisionName: compInName + "-v1",
+								DataInputs: []v1alpha2.DataInput{
+									{
+										ValueFrom: v1alpha2.DataInputValueFrom{
+											DataOutputName: "trait-output",
+										},
+										ToFieldPaths: []string{"spec.key"},
+									},
+								},
+								Traits: []v1alpha2.ComponentTrait{{
+									Trait: runtime.RawExtension{Object: outputTrait},
+									DataOutputs: []v1alpha2.DataOutput{{
+										Name:      "trait-output",
+										FieldPath: "status.key",
+									}},
+								},
+								},
+							},
+						},
+					},
+				}
+
+				By("Create Component")
+				Expect(k8sClient.Create(ctx, &compIn)).Should(Succeed())
+				cmp := &v1alpha2.Component{}
+				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compInName}, cmp)).Should(Succeed())
+
+				cmpV1 := cmp.DeepCopy()
+				By("component handler will automatically create controller revision")
+				Expect(func() bool {
+					_, ok := componentHandler.createControllerRevision(cmpV1, cmpV1)
+					return ok
+				}()).Should(BeTrue())
+
+				By("Creat appConfig & check successfully")
+				Expect(k8sClient.Create(ctx, &acWithDep)).Should(Succeed())
+				Eventually(func() error {
+					return k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: acWithDepName}, &acWithDep)
+				}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+				By("Reconcile & check successfully")
+
+				reqDep := reconcile.Request{
+					NamespacedName: client.ObjectKey{Namespace: namespace, Name: acWithDepName},
+				}
+				Eventually(func() bool {
+					reconcileRetry(reconciler, reqDep)
+					acWithDep = v1alpha2.ApplicationConfiguration{}
+					if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: acWithDepName}, &acWithDep); err != nil {
+						return false
+					}
+					return len(acWithDep.Status.Workloads) == 1
+				}, time.Second, 300*time.Millisecond).Should(BeTrue())
+
+				// because dependency is not satisfied so the workload should not be created
+				By("Check the workload is NOT created")
+				workloadIn := tempFoo.DeepCopy()
+				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compInName + "-v1"}, workloadIn)).Should(&util.NotFoundMatcher{})
+
+				// modify the trait to make it satisfy comp's dependency
+				outputTrait = tempFoo.DeepCopy()
+				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: outName}, outputTrait)).Should(Succeed())
+				err := unstructured.SetNestedField(outputTrait.Object, "test", "status", "key")
+				Expect(err).Should(BeNil())
+				Expect(k8sClient.Status().Update(ctx, outputTrait)).Should(Succeed())
+				Eventually(func() string {
+					k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: outName}, outputTrait)
+					data, _, _ := unstructured.NestedString(outputTrait.Object, "status", "key")
+					return data
+				}, 3*time.Second, time.Second).Should(Equal("test"))
+
+				By("Reconcile & check ac is satisfied")
+				Eventually(func() []v1alpha2.UnstaifiedDependency {
+					reconcileRetry(reconciler, reqDep)
+					acWithDep = v1alpha2.ApplicationConfiguration{}
+					if err := k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: acWithDepName}, &acWithDep); err != nil {
+						return []v1alpha2.UnstaifiedDependency{{Reason: err.Error()}}
+					}
+					return acWithDep.Status.Dependency.Unsatisfied
+				}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+				By("Reconcile & check workload is created")
+				Eventually(func() error {
+					reconcileRetry(reconciler, reqDep)
+					// the workload should be created now because its dependency is satisfied
+					workloadIn := tempFoo.DeepCopy()
+					return k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compInName}, workloadIn)
+				}, time.Second, 300*time.Millisecond).Should(BeNil())
+
+				By("Delete the workload")
+				recreatedWL := tempFoo.DeepCopy()
+				recreatedWL.SetName(compInName)
+				Expect(k8sClient.Delete(ctx, recreatedWL)).Should(Succeed())
+				inputWorkload2 := tempFoo.DeepCopy()
+				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compInName}, inputWorkload2)).Should(util.NotFoundMatcher{})
+
+				By("Reconcile")
+				Expect(func() error { _, err := reconciler.Reconcile(req); return err }()).Should(BeNil())
+				time.Sleep(3 * time.Second)
+
+				By("Check workload is not re-created by reconciliation")
+				inputWorkload = tempFoo.DeepCopy()
+				Expect(k8sClient.Get(ctx, client.ObjectKey{Namespace: namespace, Name: compInName}, inputWorkload)).Should(util.NotFoundMatcher{})
+			})
+
 		It("should normally create workload/trait resources at fist time", func() {
 			By("Enable ApplyOnceOnlyForce")
 			reconciler.applyOnceOnlyMode = core.ApplyOnceOnlyForce