From 97b69a06c103d26624730e1b036cc139143bccc0 Mon Sep 17 00:00:00 2001 From: Hongchao Deng Date: Mon, 28 Sep 2020 11:51:39 -0700 Subject: [PATCH 1/5] update yaml to add more scenarios Signed-off-by: Hongchao Deng --- examples/dependency/definition.yaml | 2 +- examples/dependency/demo.yaml | 40 +++++++++++++++++++++-------- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/examples/dependency/definition.yaml b/examples/dependency/definition.yaml index acf5fc47..3bc3e0c5 100644 --- a/examples/dependency/definition.yaml +++ b/examples/dependency/definition.yaml @@ -24,7 +24,7 @@ spec: apiVersion: core.oam.dev/v1alpha2 kind: TraitDefinition metadata: - name: foo.example.com + name: foos.example.com spec: definitionRef: name: foo.example.com diff --git a/examples/dependency/demo.yaml b/examples/dependency/demo.yaml index 8fc7ff1c..72a126d9 100644 --- a/examples/dependency/demo.yaml +++ b/examples/dependency/demo.yaml @@ -9,14 +9,16 @@ spec: metadata: name: source # Uncomment the following and apply again will make dependency satisfied. - # status: - # key: test status: - key: - - name: a - value: aa - - name: b - value: bb + key: test + + # Uncommnet below to test append array + # status: + # key: + # - name: a + # value: aa + # - name: b + # value: bb --- apiVersion: core.oam.dev/v1alpha2 kind: Component @@ -28,10 +30,12 @@ spec: kind: Foo metadata: name: sink - spec: - key: - - name: exist - value: existtt + + # Uncommnet below to test append array + # spec: + # key: + # - name: exist + # value: existtt --- apiVersion: core.oam.dev/v1alpha2 kind: ApplicationConfiguration @@ -49,3 +53,17 @@ spec: dataOutputName: example-key toFieldPaths: - "spec.key" + + # Uncomment to test trait + # traits: + # - trait: + # apiVersion: example.com/v1 + # kind: Foo + # metadata: + # name: source + # # Uncomment the following and apply again will make dependency satisfied. + # # status: + # # key: test + # dataOutputs: + # - name: example-key + # fieldPath: status.key From 450760ef97f27ff5104c71b77a6ca84c4117d42b Mon Sep 17 00:00:00 2001 From: Hongchao Deng Date: Mon, 28 Sep 2020 11:47:03 -0700 Subject: [PATCH 2/5] Use AppConfig's generation to get latest dependency only - pass AC's generation into child objects' labels - check generation in getting data input Signed-off-by: Hongchao Deng --- .../v1alpha2/applicationconfiguration/render.go | 17 +++++++++++++++-- pkg/oam/labels.go | 2 ++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/pkg/controller/v1alpha2/applicationconfiguration/render.go b/pkg/controller/v1alpha2/applicationconfiguration/render.go index 9de78df9..c76cfa3f 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/render.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "reflect" + "strconv" runtimev1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/crossplane/crossplane-runtime/pkg/fieldpath" @@ -136,6 +137,7 @@ func (r *components) renderComponent(ctx context.Context, acc v1alpha2.Applicati oam.LabelAppComponent: acc.ComponentName, oam.LabelAppComponentRevision: componentRevisionName, oam.LabelOAMResourceType: oam.ResourceTypeWorkload, + oam.LabelAppGeneration: strconv.FormatInt(ac.Generation, 10), } util.AddLabels(w, compInfoLabels) @@ -494,12 +496,23 @@ func (r *components) getDataInput(ctx context.Context, s *dagSource, ac *unstruc u.SetGroupVersionKind(obj.GroupVersionKind()) err := r.client.Get(ctx, key, u) if err != nil { - reason := fmt.Sprintf("failed to get object (%s)", key.String()) + reason := fmt.Sprintf("failed to get object (%s): %v", key.String(), err) return nil, false, reason, errors.Wrap(resource.IgnoreNotFound(err), reason) } - paved := fieldpath.Pave(u.UnstructuredContent()) + pavedAC := fieldpath.Pave(ac.UnstructuredContent()) + var acGeneration int + if err := pavedAC.GetValueInto("metadata.generation", &acGeneration); err != nil { + return nil, false, err.Error(), err + } + // The source object's app generation should match current AC. Otherwise it is from an old AC and we should wait until + // it is updated then reconcile again. + if g1, g2 := u.GetLabels()[oam.LabelAppGeneration], strconv.Itoa(acGeneration); g1 != g2 { + return nil, false, fmt.Sprintf("generation not match: %s, %s", g1, g2), nil + } + + paved := fieldpath.Pave(u.UnstructuredContent()) rawval, err := paved.GetValue(obj.FieldPath) if err != nil { if fieldpath.IsNotFound(err) { diff --git a/pkg/oam/labels.go b/pkg/oam/labels.go index 97b72447..3377553b 100644 --- a/pkg/oam/labels.go +++ b/pkg/oam/labels.go @@ -25,6 +25,8 @@ const ( LabelAppComponent = "app.oam.dev/component" // LabelAppComponentRevision records the revision name of Component LabelAppComponentRevision = "app.oam.dev/revision" + // LabelAppGeneration records the generation of AppConfig + LabelAppGeneration = "app.oam.dev/generation" // LabelOAMResourceType whether a CR is workload or trait LabelOAMResourceType = "app.oam.dev/resourceType" ) From b3b7bfd9957d8c6d3a6a94e0d9bd7f89c0fb3995 Mon Sep 17 00:00:00 2001 From: Hongchao Deng Date: Mon, 28 Sep 2020 14:46:42 -0700 Subject: [PATCH 3/5] fix test Signed-off-by: Hongchao Deng --- .../applicationconfiguration_test.go | 5 +++-- pkg/controller/v1alpha2/applicationconfiguration/render.go | 2 +- .../v1alpha2/applicationconfiguration/render_test.go | 6 ++++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go b/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go index b7461f8d..7b5c8a48 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go @@ -1333,8 +1333,9 @@ func TestDependency(t *testing.T) { ac := &v1alpha2.ApplicationConfiguration{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-app", - Namespace: "test-ns", + Name: "test-app", + Namespace: "test-ns", + Generation: 0, }, Spec: v1alpha2.ApplicationConfigurationSpec{ Components: tc.args.components, diff --git a/pkg/controller/v1alpha2/applicationconfiguration/render.go b/pkg/controller/v1alpha2/applicationconfiguration/render.go index c76cfa3f..1e79e282 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/render.go @@ -502,7 +502,7 @@ func (r *components) getDataInput(ctx context.Context, s *dagSource, ac *unstruc pavedAC := fieldpath.Pave(ac.UnstructuredContent()) var acGeneration int - if err := pavedAC.GetValueInto("metadata.generation", &acGeneration); err != nil { + if err := pavedAC.GetValueInto("metadata.generation", &acGeneration); err != nil && !fieldpath.IsNotFound(err) { return nil, false, err.Error(), err } diff --git a/pkg/controller/v1alpha2/applicationconfiguration/render_test.go b/pkg/controller/v1alpha2/applicationconfiguration/render_test.go index 9437e29e..d8d3e4b4 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/render_test.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/render_test.go @@ -200,6 +200,7 @@ func TestRenderComponents(t *testing.T) { oam.LabelAppName: acName, oam.LabelAppComponentRevision: "", oam.LabelOAMResourceType: oam.ResourceTypeWorkload, + oam.LabelAppGeneration: "0", }) return w }(), @@ -214,6 +215,7 @@ func TestRenderComponents(t *testing.T) { oam.LabelAppName: acName, oam.LabelAppComponentRevision: "", oam.LabelOAMResourceType: oam.ResourceTypeTrait, + oam.LabelAppGeneration: "0", }) return &Trait{Object: *t} }(), @@ -275,6 +277,7 @@ func TestRenderComponents(t *testing.T) { oam.LabelAppName: acName, oam.LabelAppComponentRevision: revisionName, oam.LabelOAMResourceType: oam.ResourceTypeWorkload, + oam.LabelAppGeneration: "0", }) return w }(), @@ -289,6 +292,7 @@ func TestRenderComponents(t *testing.T) { oam.LabelAppName: acName, oam.LabelAppComponentRevision: revisionName, oam.LabelOAMResourceType: oam.ResourceTypeTrait, + oam.LabelAppGeneration: "0", }) return &Trait{Object: *t} }(), @@ -341,6 +345,7 @@ func TestRenderComponents(t *testing.T) { oam.LabelAppName: acName, oam.LabelAppComponentRevision: revisionName2, oam.LabelOAMResourceType: oam.ResourceTypeWorkload, + oam.LabelAppGeneration: "0", }) return w }(), @@ -355,6 +360,7 @@ func TestRenderComponents(t *testing.T) { oam.LabelAppName: acName, oam.LabelAppComponentRevision: revisionName2, oam.LabelOAMResourceType: oam.ResourceTypeTrait, + oam.LabelAppGeneration: "0", }) return &Trait{Object: *t} }(), From 05f65802ea83c533d9473808ef8d229ff2b2855d Mon Sep 17 00:00:00 2001 From: Hongchao Deng Date: Mon, 28 Sep 2020 15:15:25 -0700 Subject: [PATCH 4/5] add unit test to cover generation check path Signed-off-by: Hongchao Deng --- .../applicationconfiguration_test.go | 99 ++++++++++++++++++- .../applicationconfiguration/render.go | 2 +- test/e2e-test/appconfig_dependency_test.go | 2 +- 3 files changed, 98 insertions(+), 5 deletions(-) diff --git a/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go b/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go index 7b5c8a48..fef05ed0 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/applicationconfiguration_test.go @@ -41,6 +41,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/crossplane/oam-kubernetes-runtime/apis/core/v1alpha2" + "github.com/crossplane/oam-kubernetes-runtime/pkg/oam" "github.com/crossplane/oam-kubernetes-runtime/pkg/oam/mock" ) @@ -856,6 +857,7 @@ func TestDependency(t *testing.T) { unreadyWorkload.SetKind("Workload") unreadyWorkload.SetNamespace("test-ns") unreadyWorkload.SetName("unready-workload") + unreadyWorkload.SetLabels(map[string]string{oam.LabelAppGeneration: "0"}) readyWorkload := unreadyWorkload.DeepCopy() readyWorkload.SetName("ready-workload") @@ -879,6 +881,7 @@ func TestDependency(t *testing.T) { unreadyTrait.SetKind("Trait") unreadyTrait.SetNamespace("test-ns") unreadyTrait.SetName("unready-trait") + unreadyTrait.SetLabels(map[string]string{oam.LabelAppGeneration: "0"}) readyTrait := unreadyTrait.DeepCopy() readyTrait.SetName("ready-trait") @@ -887,10 +890,14 @@ func TestDependency(t *testing.T) { t.Fatal(err) } + readyTraitNewGen := readyTrait.DeepCopy() + readyTraitNewGen.SetLabels(map[string]string{oam.LabelAppGeneration: "1"}) + type args struct { components []v1alpha2.ApplicationConfigurationComponent wl *unstructured.Unstructured trait *unstructured.Unstructured + generation int64 } type want struct { err error @@ -1292,6 +1299,92 @@ func TestDependency(t *testing.T) { }, depStatus: &v1alpha2.DependencyStatus{}}, }, + "DataOutput from old generation should be regarded as not ready": { + args: args{ + components: []v1alpha2.ApplicationConfigurationComponent{{ + ComponentName: "test-component-sink", + DataInputs: []v1alpha2.DataInput{{ + ValueFrom: v1alpha2.DataInputValueFrom{DataOutputName: "test-output"}, + ToFieldPaths: []string{"spec.key"}, + }}, + Traits: []v1alpha2.ComponentTrait{{ + Trait: runtime.RawExtension{}, + DataOutputs: []v1alpha2.DataOutput{{ + Name: "test-output", + FieldPath: "status.key", + }}, + }}, + }}, + wl: unreadyWorkload.DeepCopy(), + trait: readyTrait.DeepCopy(), + generation: 1, + }, + want: want{ + verifyWorkloads: func(ws []Workload) { + if !ws[0].HasDep { + t.Error("Workload should be unready to apply") + } + }, + depStatus: &v1alpha2.DependencyStatus{ + Unsatisfied: []v1alpha2.UnstaifiedDependency{{ + Reason: "generation not match: obj (0), ac (1)", + From: v1alpha2.DependencyFromObject{ + TypedReference: runtimev1alpha1.TypedReference{ + APIVersion: readyTrait.GetAPIVersion(), + Kind: readyTrait.GetKind(), + Name: readyTrait.GetName(), + }, + FieldPath: "status.key", + }, + To: v1alpha2.DependencyToObject{ + TypedReference: runtimev1alpha1.TypedReference{ + APIVersion: unreadyWorkload.GetAPIVersion(), + Kind: unreadyWorkload.GetKind(), + Name: unreadyWorkload.GetName(), + }, + FieldPaths: []string{"spec.key"}, + }, + }}, + }, + }, + }, + "DataOutput from the same generation should be ready": { + args: args{ + components: []v1alpha2.ApplicationConfigurationComponent{{ + ComponentName: "test-component-sink", + DataInputs: []v1alpha2.DataInput{{ + ValueFrom: v1alpha2.DataInputValueFrom{DataOutputName: "test-output"}, + ToFieldPaths: []string{"spec.key"}, + }}, + Traits: []v1alpha2.ComponentTrait{{ + Trait: runtime.RawExtension{}, + DataOutputs: []v1alpha2.DataOutput{{ + Name: "test-output", + FieldPath: "status.key", + }}, + }}, + }}, + wl: unreadyWorkload.DeepCopy(), + trait: readyTraitNewGen.DeepCopy(), + generation: 1, + }, + want: want{ + verifyWorkloads: func(ws []Workload) { + if ws[0].HasDep { + t.Error("Workload should be ready to apply") + } + + s, _, err := unstructured.NestedString(ws[0].Workload.UnstructuredContent(), "spec", "key") + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(s, "test"); diff != "" { + t.Fatal(diff) + } + }, + depStatus: &v1alpha2.DependencyStatus{}, + }, + }, } for name, tc := range cases { @@ -1324,10 +1417,10 @@ func TestDependency(t *testing.T) { }, params: ParameterResolveFn(resolve), workload: ResourceRenderFn(func(data []byte, p ...Parameter) (*unstructured.Unstructured, error) { - return tc.args.wl, nil + return tc.args.wl.DeepCopy(), nil }), trait: ResourceRenderFn(func(data []byte, p ...Parameter) (*unstructured.Unstructured, error) { - return tc.args.trait, nil + return tc.args.trait.DeepCopy(), nil }), } @@ -1335,7 +1428,7 @@ func TestDependency(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "test-app", Namespace: "test-ns", - Generation: 0, + Generation: tc.args.generation, }, Spec: v1alpha2.ApplicationConfigurationSpec{ Components: tc.args.components, diff --git a/pkg/controller/v1alpha2/applicationconfiguration/render.go b/pkg/controller/v1alpha2/applicationconfiguration/render.go index 1e79e282..047f3de0 100644 --- a/pkg/controller/v1alpha2/applicationconfiguration/render.go +++ b/pkg/controller/v1alpha2/applicationconfiguration/render.go @@ -509,7 +509,7 @@ func (r *components) getDataInput(ctx context.Context, s *dagSource, ac *unstruc // The source object's app generation should match current AC. Otherwise it is from an old AC and we should wait until // it is updated then reconcile again. if g1, g2 := u.GetLabels()[oam.LabelAppGeneration], strconv.Itoa(acGeneration); g1 != g2 { - return nil, false, fmt.Sprintf("generation not match: %s, %s", g1, g2), nil + return nil, false, fmt.Sprintf("generation not match: obj (%s), ac (%s)", g1, g2), nil } paved := fieldpath.Pave(u.UnstructuredContent()) diff --git a/test/e2e-test/appconfig_dependency_test.go b/test/e2e-test/appconfig_dependency_test.go index 7dc290bc..0a3e509d 100644 --- a/test/e2e-test/appconfig_dependency_test.go +++ b/test/e2e-test/appconfig_dependency_test.go @@ -214,7 +214,7 @@ var _ = Describe("Resource Dependency in an ApplicationConfiguration", func() { k8sClient.Get(ctx, appconfigKey, appconfig) return appconfig.Status.Dependency.Unsatisfied }, - time.Second*80, time.Second*2).Should(BeNil()) + time.Second*180, time.Second*5).Should(BeNil()) } It("trait depends on another trait", func() { From 7f467b8e3457dc392bc8424c99e8b30b7b6e44ac Mon Sep 17 00:00:00 2001 From: Hongchao Deng Date: Mon, 28 Sep 2020 19:41:38 -0700 Subject: [PATCH 5/5] Update test/e2e-test/appconfig_dependency_test.go Co-authored-by: Jianbo Sun --- test/e2e-test/appconfig_dependency_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e-test/appconfig_dependency_test.go b/test/e2e-test/appconfig_dependency_test.go index 0a3e509d..636b3edc 100644 --- a/test/e2e-test/appconfig_dependency_test.go +++ b/test/e2e-test/appconfig_dependency_test.go @@ -214,7 +214,7 @@ var _ = Describe("Resource Dependency in an ApplicationConfiguration", func() { k8sClient.Get(ctx, appconfigKey, appconfig) return appconfig.Status.Dependency.Unsatisfied }, - time.Second*180, time.Second*5).Should(BeNil()) + time.Second*120, time.Second*2).Should(BeNil()) } It("trait depends on another trait", func() {