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 <scott@andrews.me>

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)
 		}

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)
 	}

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)

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