Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Use AppConfig's generation to get latest dependency only #224

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/dependency/definition.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
40 changes: 29 additions & 11 deletions examples/dependency/demo.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -1324,17 +1417,18 @@ 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
}),
}

ac := &v1alpha2.ApplicationConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: "test-app",
Namespace: "test-ns",
Name: "test-app",
Namespace: "test-ns",
Generation: tc.args.generation,
},
Spec: v1alpha2.ApplicationConfigurationSpec{
Components: tc.args.components,
Expand Down
17 changes: 15 additions & 2 deletions pkg/controller/v1alpha2/applicationconfiguration/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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 && !fieldpath.IsNotFound(err) {
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: obj (%s), ac (%s)", g1, g2), nil
}
Comment on lines +511 to +513
Copy link
Member

@wonderflow wonderflow Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object can be workload or trait. The object will always have this label, it is meaningless to check. We should check status is updated to align this label.

And also, this is a break change, I don't think we need to make it as a force check.


paved := fieldpath.Pave(u.UnstructuredContent())
rawval, err := paved.GetValue(obj.FieldPath)
if err != nil {
if fieldpath.IsNotFound(err) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ func TestRenderComponents(t *testing.T) {
oam.LabelAppName: acName,
oam.LabelAppComponentRevision: "",
oam.LabelOAMResourceType: oam.ResourceTypeWorkload,
oam.LabelAppGeneration: "0",
})
return w
}(),
Expand All @@ -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}
}(),
Expand Down Expand Up @@ -275,6 +277,7 @@ func TestRenderComponents(t *testing.T) {
oam.LabelAppName: acName,
oam.LabelAppComponentRevision: revisionName,
oam.LabelOAMResourceType: oam.ResourceTypeWorkload,
oam.LabelAppGeneration: "0",
})
return w
}(),
Expand All @@ -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}
}(),
Expand Down Expand Up @@ -341,6 +345,7 @@ func TestRenderComponents(t *testing.T) {
oam.LabelAppName: acName,
oam.LabelAppComponentRevision: revisionName2,
oam.LabelOAMResourceType: oam.ResourceTypeWorkload,
oam.LabelAppGeneration: "0",
})
return w
}(),
Expand All @@ -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}
}(),
Expand Down
2 changes: 2 additions & 0 deletions pkg/oam/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down
2 changes: 1 addition & 1 deletion test/e2e-test/appconfig_dependency_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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*120, time.Second*2).Should(BeNil())
}

It("trait depends on another trait", func() {
Expand Down