From 7342347711d6bc8d263ceec13a4f1a484f376019 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Mon, 11 Mar 2024 11:56:34 -0400 Subject: [PATCH] List children exactly once for ChildSetReconciler (#494) Previously, the ChildSetReconciler would list children once for the set and again for each child being reconciled. At best this was inefficient, at worst it introduced subtile issues when the content of the listing changed over the course of the reconcile. Now the content of the list from the ChildSetReconciler is reused by each child that is reconciled. Signed-off-by: Scott Andrews --- reconcilers/child.go | 25 +++++++++++++++---------- reconcilers/childset.go | 20 ++++++++++++++------ reconcilers/childset_test.go | 3 +++ testing/client.go | 15 +++++++++++++++ 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/reconcilers/child.go b/reconcilers/child.go index 3cab787..6a4525b 100644 --- a/reconcilers/child.go +++ b/reconcilers/child.go @@ -306,16 +306,21 @@ func (r *ChildReconciler[T, CT, CLT]) reconcile(ctx context.Context, resource T) c := RetrieveConfigOrDie(ctx) actual := r.ChildType.DeepCopyObject().(CT) - children := r.ChildListType.DeepCopyObject().(CLT) - if err := c.List(ctx, children, r.listOptions(ctx, resource)...); err != nil { - return nilCT, err + children := RetrieveKnownChildren[CT](ctx) + if children == nil { + // use existing known children when available, fall back to lookup + list := r.ChildListType.DeepCopyObject().(CLT) + if err := c.List(ctx, list, r.listOptions(ctx, resource)...); err != nil { + return nilCT, err + } + children = extractItems[CT](list) } - items := r.filterChildren(resource, children) - if len(items) == 1 { - actual = items[0] - } else if len(items) > 1 { + children = r.filterChildren(resource, children) + if len(children) == 1 { + actual = children[0] + } else if len(children) > 1 { // this shouldn't happen, delete everything to a clean slate - for _, extra := range items { + for _, extra := range children { log.Info("deleting extra child", "child", namespaceName(extra)) if err := c.Delete(ctx, extra); err != nil { if !errors.Is(err, ErrQuiet) { @@ -362,9 +367,9 @@ func (r *ChildReconciler[T, CT, CLT]) desiredChild(ctx context.Context, resource return r.DesiredChild(ctx, resource) } -func (r *ChildReconciler[T, CT, CLT]) filterChildren(resource T, children CLT) []CT { +func (r *ChildReconciler[T, CT, CLT]) filterChildren(resource T, children []CT) []CT { items := []CT{} - for _, child := range extractItems[CT](children) { + for _, child := range children { if r.ourChild(resource, child) { items = append(items, child) } diff --git a/reconcilers/childset.go b/reconcilers/childset.go index b659bf3..d4197a9 100644 --- a/reconcilers/childset.go +++ b/reconcilers/childset.go @@ -283,7 +283,13 @@ func (r *ChildSetReconciler[T, CT, CLT]) Reconcile(ctx context.Context, resource WithName(r.Name) ctx = logr.NewContext(ctx, log) - cr, err := r.composeChildReconcilers(ctx, resource) + knownChildren, err := r.knownChildren(ctx, resource) + if err != nil { + return Result{}, err + } + ctx = stashKnownChildren(ctx, knownChildren) + + cr, err := r.composeChildReconcilers(ctx, resource, knownChildren) if err != nil { return Result{}, err } @@ -292,11 +298,9 @@ func (r *ChildSetReconciler[T, CT, CLT]) Reconcile(ctx context.Context, resource return result, err } -func (r *ChildSetReconciler[T, CT, CLT]) composeChildReconcilers(ctx context.Context, resource T) (SubReconciler[T], error) { +func (r *ChildSetReconciler[T, CT, CLT]) knownChildren(ctx context.Context, resource T) ([]CT, error) { c := RetrieveConfigOrDie(ctx) - childIDs := sets.NewString() - children := r.ChildListType.DeepCopyObject().(CLT) ourChildren := []CT{} if err := c.List(ctx, children, r.voidReconciler.listOptions(ctx, resource)...); err != nil { @@ -309,12 +313,16 @@ func (r *ChildSetReconciler[T, CT, CLT]) composeChildReconcilers(ctx context.Con ourChildren = append(ourChildren, child.DeepCopyObject().(CT)) } - ctx = stashKnownChildren(ctx, ourChildren) + return ourChildren, nil +} + +func (r *ChildSetReconciler[T, CT, CLT]) composeChildReconcilers(ctx context.Context, resource T, knownChildren []CT) (SubReconciler[T], error) { desiredChildren, desiredChildrenErr := r.DesiredChildren(ctx, resource) if desiredChildrenErr != nil && !errors.Is(desiredChildrenErr, OnlyReconcileChildStatus) { return nil, desiredChildrenErr } + childIDs := sets.NewString() desiredChildByID := map[string]CT{} for _, child := range desiredChildren { id := r.IdentifyChild(child) @@ -328,7 +336,7 @@ func (r *ChildSetReconciler[T, CT, CLT]) composeChildReconcilers(ctx context.Con desiredChildByID[id] = child } - for _, child := range ourChildren { + for _, child := range knownChildren { id := r.IdentifyChild(child) childIDs.Insert(id) } diff --git a/reconcilers/childset_test.go b/reconcilers/childset_test.go index 2f33e67..39ceb0c 100644 --- a/reconcilers/childset_test.go +++ b/reconcilers/childset_test.go @@ -167,6 +167,9 @@ func TestChildSetReconciler(t *testing.T) { configMapBlueGiven.DieReleasePtr(), configMapGreenGiven.DieReleasePtr(), }, + WithReactors: []rtesting.ReactionFunc{ + rtesting.CalledAtMostTimes("list", "ConfigMapList", 1), + }, Metadata: map[string]interface{}{ "SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] { r := defaultChildSetReconciler(c) diff --git a/testing/client.go b/testing/client.go index 56dabd5..3a524b3 100644 --- a/testing/client.go +++ b/testing/client.go @@ -460,6 +460,21 @@ func InduceFailure(verb, kind string, o ...InduceFailureOpts) ReactionFunc { } } +// CalledAtMostTimes error if the client is called more than the max number of times for a verb and kind +func CalledAtMostTimes(verb, kind string, maxCalls int) ReactionFunc { + callCount := 0 + return func(action Action) (handled bool, ret runtime.Object, err error) { + if !action.Matches("list", "ConfigMapList") { + return false, nil, nil + } + callCount++ + if callCount <= maxCalls { + return false, nil, nil + } + return true, nil, fmt.Errorf("%s %s called %d times, expected %d call(s)", verb, kind, callCount, maxCalls) + } +} + type namedAction interface { Action GetName() string