From 66177ddd132db54b4e394c33cfbce214079c6ca0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=A4=A9=E5=85=83?= Date: Mon, 1 Mar 2021 15:03:29 +0800 Subject: [PATCH] update CRD and fix workload not create first time --- apis/core.oam.dev/v1alpha2/core_types.go | 18 +- ...ore.oam.dev_applicationconfigurations.yaml | 14 +- ...ore.oam.dev_applicationconfigurations.yaml | 14 +- .../applicationconfiguration.go | 64 +++--- .../apply_once_only_test.go | 182 +++++++++++++++++- 5 files changed, 247 insertions(+), 45 deletions(-) diff --git a/apis/core.oam.dev/v1alpha2/core_types.go b/apis/core.oam.dev/v1alpha2/core_types.go index e12125133..b03d5ce07 100644 --- a/apis/core.oam.dev/v1alpha2/core_types.go +++ b/apis/core.oam.dev/v1alpha2/core_types.go @@ -413,6 +413,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. @@ -443,12 +453,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"` diff --git a/charts/vela-core/crds/core.oam.dev_applicationconfigurations.yaml b/charts/vela-core/crds/core.oam.dev_applicationconfigurations.yaml index 738f0c7b7..f8efd98b3 100644 --- a/charts/vela-core/crds/core.oam.dev_applicationconfigurations.yaml +++ b/charts/vela-core/crds/core.oam.dev_applicationconfigurations.yaml @@ -748,10 +748,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 @@ -801,6 +800,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 diff --git a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationconfigurations.yaml b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationconfigurations.yaml index 6e7371f46..e72bcc91b 100644 --- a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationconfigurations.yaml +++ b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationconfigurations.yaml @@ -748,10 +748,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 @@ -801,6 +800,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 diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go index c93dd6fbd..66fbe1cb1 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go @@ -408,8 +408,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 + } } } } @@ -532,6 +537,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{ @@ -650,47 +656,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 diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_once_only_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_once_only_test.go index bd6e5b525..3c0171820 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/apply_once_only_test.go +++ b/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