From 993994446ad595748371e57525c7478e1027bb24 Mon Sep 17 00:00:00 2001 From: hasheddan Date: Fri, 10 Apr 2020 07:34:30 -0500 Subject: [PATCH] Pass workload kind to trait reconciler Because we no longer include the UID of the workload in a trait's workloadRef it is necessary to pass the workload kind to the trait reconciler so that we can get the referenced workload on each reconcile and use its UID for ensuring the corresponding translation is controllable by it. Signed-off-by: hasheddan --- pkg/reconciler/oam/trait/reconciler.go | 31 +++++- pkg/reconciler/oam/trait/reconciler_test.go | 107 ++++++++++++++++++-- 2 files changed, 126 insertions(+), 12 deletions(-) diff --git a/pkg/reconciler/oam/trait/reconciler.go b/pkg/reconciler/oam/trait/reconciler.go index d09d1dd..bfe31bc 100644 --- a/pkg/reconciler/oam/trait/reconciler.go +++ b/pkg/reconciler/oam/trait/reconciler.go @@ -18,6 +18,7 @@ package trait import ( "context" + "fmt" "time" "github.com/pkg/errors" @@ -45,6 +46,7 @@ const ( errGetTrait = "cannot get trait" errUpdateTraitStatus = "cannot update trait status" errTraitModify = "cannot apply trait modification" + errGetWorkload = "cannot get workload referenced in trait" errGetTranslation = "cannot get translation for workload reference in trait" errApplyTraitModification = "cannot apply trait modification to workload translation" ) @@ -54,6 +56,7 @@ const ( reasonTraitWait = "WaitingForWorkloadTranslation" reasonTraitModify = "PackageModified" + reasonCannotGetWorkload = "CannotGetReferencedWorkload" reasonCannotGetTranslation = "CannotGetReferencedWorkloadTranslation" reasonCannotModifyTranslation = "CannotModifyTranslation" reasonCannotApplyModification = "CannotApplyModification" @@ -96,6 +99,7 @@ func WithApplicator(a resource.Applicator) ReconcilerOption { type Reconciler struct { client client.Client newTrait func() resource.Trait + newWorkload func() resource.Workload newTranslation func() resource.Object trait Modifier applicator resource.Applicator @@ -106,11 +110,13 @@ type Reconciler struct { // NewReconciler returns a Reconciler that reconciles OAM traits by fetching // their referenced workload's translation and applying modifications. -func NewReconciler(m ctrl.Manager, trait resource.TraitKind, trans resource.ObjectKind, o ...ReconcilerOption) *Reconciler { +func NewReconciler(m ctrl.Manager, trait resource.TraitKind, workload resource.WorkloadKind, trans resource.ObjectKind, o ...ReconcilerOption) *Reconciler { nt := func() resource.Trait { return resource.MustCreateObject(schema.GroupVersionKind(trait), m.GetScheme()).(resource.Trait) } - + nw := func() resource.Workload { + return resource.MustCreateObject(schema.GroupVersionKind(workload), m.GetScheme()).(resource.Workload) + } nr := func() resource.Object { return resource.MustCreateObject(schema.GroupVersionKind(trans), m.GetScheme()).(resource.Object) } @@ -118,6 +124,7 @@ func NewReconciler(m ctrl.Manager, trait resource.TraitKind, trans resource.Obje r := &Reconciler{ client: m.GetClient(), newTrait: nt, + newWorkload: nw, newTranslation: nr, trait: ModifyFn(NoopModifier), applicator: resource.NewAPIPatchingApplicator(m.GetClient()), @@ -149,6 +156,22 @@ func (r *Reconciler) Reconcile(req reconcile.Request) (reconcile.Result, error) log = log.WithValues("uid", trait.GetUID(), "version", trait.GetResourceVersion()) + workload := r.newWorkload() + err := r.client.Get(ctx, types.NamespacedName{Name: trait.GetWorkloadReference().Name, Namespace: trait.GetNamespace()}, workload) + if kerrors.IsNotFound(err) { + log.Debug("Waiting for referenced workload to exist", "kind", trait.GetObjectKind().GroupVersionKind().String()) + r.record.Event(trait, event.Normal(reasonTraitWait, "Waiting for workload to exist")) + trait.SetConditions(v1alpha1.ReconcileSuccess()) + return reconcile.Result{RequeueAfter: shortWait}, errors.Wrap(r.client.Status().Update(ctx, trait), errUpdateTraitStatus) + } + if err != nil { + fmt.Println("HEEERREE") + log.Debug("Cannot get referenced workload", "error", err, "requeue-after", time.Now().Add(shortWait)) + r.record.Event(trait, event.Warning(reasonCannotGetWorkload, err)) + trait.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errGetWorkload))) + return reconcile.Result{RequeueAfter: shortWait}, errors.Wrap(r.client.Status().Update(ctx, trait), errUpdateTraitStatus) + } + translation := r.newTranslation() // TODO(hasheddan): we make the assumption here that the workload @@ -156,7 +179,7 @@ func (r *Reconciler) Reconcile(req reconcile.Request) (reconcile.Result, error) // workload itself. This would not work if a translation produced multiple // objects of the same kind as they would not be permitted to have the same // name. - err := r.client.Get(ctx, types.NamespacedName{Name: trait.GetWorkloadReference().Name, Namespace: trait.GetNamespace()}, translation) + err = r.client.Get(ctx, types.NamespacedName{Name: workload.GetName(), Namespace: trait.GetNamespace()}, translation) if kerrors.IsNotFound(err) { log.Debug("Waiting for referenced workload's translation", "kind", trait.GetObjectKind().GroupVersionKind().String()) r.record.Event(trait, event.Normal(reasonTraitWait, "Waiting for workload translation to exist")) @@ -182,7 +205,7 @@ func (r *Reconciler) Reconcile(req reconcile.Request) (reconcile.Result, error) // object(s) already exists in the same namespace, with the same name, and // with a different controller before it is created, this wll guard against // modifying it. - if err := r.applicator.Apply(ctx, translation, resource.MustBeControllableBy(trait.GetWorkloadReference().UID)); err != nil { // nolint:staticcheck + if err := r.applicator.Apply(ctx, translation, resource.MustBeControllableBy(workload.GetUID())); err != nil { // nolint:staticcheck log.Debug("Cannot apply workload translation", "error", err, "requeue-after", time.Now().Add(shortWait)) r.record.Event(trait, event.Warning(reasonCannotApplyModification, err)) trait.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errApplyTraitModification))) diff --git a/pkg/reconciler/oam/trait/reconciler_test.go b/pkg/reconciler/oam/trait/reconciler_test.go index 7b1a788..2557cbc 100644 --- a/pkg/reconciler/oam/trait/reconciler_test.go +++ b/pkg/reconciler/oam/trait/reconciler_test.go @@ -41,6 +41,7 @@ func TestReconciler(t *testing.T) { type args struct { m manager.Manager t resource.TraitKind + w resource.WorkloadKind p resource.ObjectKind o []ReconcilerOption } @@ -66,9 +67,10 @@ func TestReconciler(t *testing.T) { args: args{ m: &fake.Manager{ Client: &test.MockClient{MockGet: test.NewMockGetFn(errBoom)}, - Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Object{}), + Scheme: fake.SchemeWith(&fake.Trait{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), }, want: want{err: errors.Wrap(errBoom, errGetTrait)}, @@ -81,10 +83,89 @@ func TestReconciler(t *testing.T) { Scheme: fake.SchemeWith(&fake.Trait{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), }, want: want{result: reconcile.Result{}}, }, + "WorkloadNotFound": { + reason: "Status should report successful reconcile and we should requeue after short wait if workload is not found.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: func(_ context.Context, key client.ObjectKey, obj runtime.Object) error { + if t, ok := obj.(resource.Trait); ok { + t.SetWorkloadReference(v1alpha1.TypedReference{ + APIVersion: workloadAPIVersion, + Kind: workloadKind, + Name: workloadName, + }) + return nil + } + if _, ok := obj.(resource.Workload); ok { + return kerrors.NewNotFound(schema.GroupResource{}, "") + } + return errBoom + }, + MockStatusUpdate: func(_ context.Context, obj runtime.Object, _ ...client.UpdateOption) error { + got := obj.(resource.Trait) + + if diff := cmp.Diff(v1alpha1.ReasonReconcileSuccess, got.GetCondition(v1alpha1.TypeSynced).Reason); diff != "" { + return errors.Errorf("MockStatusUpdate: -want, +got: %s", diff) + } + + return nil + }, + }, + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}), + }, + t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), + p: resource.ObjectKind(fake.GVK(&fake.Object{})), + }, + want: want{result: reconcile.Result{RequeueAfter: shortWait}}, + }, + "GetWorkloadError": { + reason: "Status should report reconcile error and we should requeue after short wait if we encounter an error getting the workload.", + args: args{ + m: &fake.Manager{ + Client: &test.MockClient{ + MockGet: func(_ context.Context, key client.ObjectKey, obj runtime.Object) error { + if t, ok := obj.(resource.Trait); ok { + t.SetWorkloadReference(v1alpha1.TypedReference{ + APIVersion: workloadAPIVersion, + Kind: workloadKind, + Name: workloadName, + }) + return nil + } + if _, ok := obj.(resource.Workload); ok { + return errBoom + } + return nil + }, + MockStatusUpdate: func(_ context.Context, obj runtime.Object, _ ...client.UpdateOption) error { + got := obj.(resource.Trait) + + if diff := cmp.Diff(v1alpha1.ReasonReconcileError, got.GetCondition(v1alpha1.TypeSynced).Reason); diff != "" { + return errors.Errorf("MockStatusUpdate: -want, +got: %s", diff) + } + + if diff := cmp.Diff(errors.Wrap(errBoom, errGetWorkload).Error(), got.GetCondition(v1alpha1.TypeSynced).Message); diff != "" { + return errors.Errorf("MockStatusUpdate: -want, +got: %s", diff) + } + + return nil + }, + }, + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}), + }, + t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), + p: resource.ObjectKind(fake.GVK(&fake.Object{})), + }, + want: want{result: reconcile.Result{RequeueAfter: shortWait}}, + }, "TranslationNotFound": { reason: "Status should report successful reconcile and we should requeue after short wait if translation is not found.", args: args{ @@ -111,9 +192,10 @@ func TestReconciler(t *testing.T) { return nil }, }, - Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Object{}), + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}, &fake.Object{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), }, want: want{result: reconcile.Result{RequeueAfter: shortWait}}, @@ -132,6 +214,10 @@ func TestReconciler(t *testing.T) { }) return nil } + if w, ok := obj.(resource.Workload); ok { + w.SetName(workloadName) + return nil + } return errBoom }, MockStatusUpdate: func(_ context.Context, obj runtime.Object, _ ...client.UpdateOption) error { @@ -148,9 +234,10 @@ func TestReconciler(t *testing.T) { return nil }, }, - Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Object{}), + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}, &fake.Object{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), }, want: want{result: reconcile.Result{RequeueAfter: shortWait}}, @@ -188,9 +275,10 @@ func TestReconciler(t *testing.T) { return nil }, }, - Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Object{}), + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}, &fake.Object{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), o: []ReconcilerOption{WithModifier(ModifyFn(func(_ context.Context, _ runtime.Object, _ resource.Trait) error { return errBoom @@ -231,9 +319,10 @@ func TestReconciler(t *testing.T) { return nil }, }, - Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Object{}), + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}, &fake.Object{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), o: []ReconcilerOption{WithApplicator(resource.ApplyFn(func(_ context.Context, _ runtime.Object, _ ...resource.ApplyOption) error { return errBoom @@ -274,9 +363,10 @@ func TestReconciler(t *testing.T) { return nil }, }, - Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Object{}), + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}, &fake.Object{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), o: []ReconcilerOption{WithApplicator(resource.ApplyFn(func(_ context.Context, _ runtime.Object, _ ...resource.ApplyOption) error { return nil @@ -305,9 +395,10 @@ func TestReconciler(t *testing.T) { }, MockStatusUpdate: test.NewMockStatusUpdateFn(errBoom), }, - Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Object{}), + Scheme: fake.SchemeWith(&fake.Trait{}, &fake.Workload{}, &fake.Object{}), }, t: resource.TraitKind(fake.GVK(&fake.Trait{})), + w: resource.WorkloadKind(fake.GVK(&fake.Workload{})), p: resource.ObjectKind(fake.GVK(&fake.Object{})), o: []ReconcilerOption{WithApplicator(resource.ApplyFn(func(_ context.Context, _ runtime.Object, _ ...resource.ApplyOption) error { return nil @@ -322,7 +413,7 @@ func TestReconciler(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { - r := NewReconciler(tc.args.m, tc.args.t, tc.args.p, tc.args.o...) + r := NewReconciler(tc.args.m, tc.args.t, tc.args.w, tc.args.p, tc.args.o...) got, err := r.Reconcile(reconcile.Request{}) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {