Adding ChildSetReconciler to manage a set of child resources by LittleBaiBai · Pull Request #382 · vmware-labs/reconciler-runtime

LittleBaiBai · 2023-06-14T18:22:32Z

Relates to #75

reconcilers/reconcilers.go

LittleBaiBai · 2023-06-14T19:08:41Z

reconcilers/reconcilers_test.go

+				},
+			},
+		},
+		"sanitize each child before logging": { //TODO: what's different?


I copied this test from the ChildReconciler test suite but I don't fully understand how we are testing the functionality here. There seems to be no assertion on logs, and changing the return value of SanitizeForEachChild, or SanitizeForChild in ChildReconciler suite, doesn't seem to affect the test runs.

Closes vmware-labs#75

scothis

initial comments, will need to dig deeper tomorrow

scothis · 2023-06-14T21:06:48Z

reconcilers/reconcilers.go

 	"encoding/json"
 	"errors"
 	"fmt"
+	utilErrs "k8s.io/apimachinery/pkg/util/errors"


this package is already imported as apierrs

apierrs "k8s.io/apimachinery/pkg/api/errors" is a different package from this.

scothis · 2023-06-14T21:07:46Z

reconcilers/reconcilers.go

 		return Result{}, err
 	}
-	r.ReflectChildStatusOnParent(ctx, resource, child, err)
+	r.ReflectChildStatusOnParent(ctx, resource, child, nil)


was this intentional?

Yes it was intended. The if block before this already handled the case when err != nil so the err here will be nil and it reads better to me. Happy to revert it if that's preferred.

scothis · 2023-06-14T22:27:30Z

reconcilers/reconcilers.go

+		return Result{}, aggregateErr
+	}
+	if aggregateErr != nil {
+		for _, eachErr := range aggregateErr.(utilErrs.Aggregate).Errors() {


this cast appears to be redundant since aggregateErr is already an Aggregate as defined by the reconcile signature.

Good point. That was there before I change the signature of the method. Updated in latest commits.

scothis · 2023-06-14T22:31:24Z

reconcilers/reconcilers.go

+				r.ReflectChildSetStatusOnParent(ctx, resource, childSet, aggregateErr)
+				return Result{}, nil


this return can obscure other errors that exist in the aggregate

Good catch. Could you share more on why you chose to return Result{}, nil for ChildReconciler here? Is it for not triggering the rateLimiter in the requeue? I'm tempted to move the ReflectChildSetStatusOnParent call outside of the for loop and always return return Result{}, aggregateErr when something goes wrong.

scothis · 2023-06-14T22:34:54Z

reconcilers/reconcilers.go

+	return defaultValue
+}
+
+func (r *ChildSetReconciler[T, CT, CLT]) reconcile(ctx context.Context, resource T) ([]CT, utilErrs.Aggregate) {


Aggregate errors strip out nil values, so the errors are separated from the specific child, is this is ok? Switch to []error if linking the error with a child resource is important.

Oh interesting! I'll experiment with that.

As I try to rewrite it, there are errors that could happen before the actual child resource reconciliation, e.g. listing actual children. Also, when error occurs, the result returned by r.stamp.Manage is usually nilT so I still don't see how we can link the error to the child resource even if we switch it to []Error.

scothis · 2023-06-14T22:52:24Z

reconcilers/reconcilers.go

+				// on the reconciled resource as being not ready.
+				apierr := eachErr.(apierrs.APIStatus)
+				conflicted := r.ChildType.DeepCopyObject().(CT)
+				_ = c.APIReader.Get(ctx, types.NamespacedName{Namespace: resource.GetNamespace(), Name: apierr.Status().Details.Name}, conflicted)


this line comes from an earlier era of ChildReconciler where children were forced to exist in the same namespace at the parent resource. This restriction was relaxed, but this line wasn't updated... I'll need to think about how to resolve this since the namespace is not on the err object.

scothis · 2023-06-14T23:00:10Z

reconcilers/reconcilers.go

+	}
+
+	actualChildrenMap, err1 := r.childListToMap(actualChildren)
+	if err1 != nil {


avoid non-semantic names for errors. Either reassign err, or if you need to avoid shadowing the existing value, use a semantic name.

Updated in latest commits

scothis · 2023-06-14T23:04:55Z

reconcilers/reconcilers.go

+
+func (r *ChildSetReconciler[T, CT, CLT]) hasChild(childSet []CT) bool {
+	hasChild := false
+	if !internal.IsNil(childSet) {


checking the length is a more common way to nil check a slice, unless nil and length 0 have different semantic values.

Updated in latest commits

…, switched to nilCT

scothis · 2023-07-28T18:29:12Z

Folded concepts into #396

vmwclabot added the cla-not-required label Jun 14, 2023

LittleBaiBai force-pushed the child-set-subreconciler branch from c4df50a to 0787ec4 Compare June 14, 2023 18:25

LittleBaiBai commented Jun 14, 2023

View reviewed changes

reconcilers/reconcilers.go Outdated Show resolved Hide resolved

LittleBaiBai force-pushed the child-set-subreconciler branch 3 times, most recently from 1b62191 to bc1d6f2 Compare June 14, 2023 19:01

LittleBaiBai commented Jun 14, 2023

View reviewed changes

Add ChildSetReconciler to manage a set of child resources

3599f72

Closes vmware-labs#75

LittleBaiBai force-pushed the child-set-subreconciler branch from bc1d6f2 to 3599f72 Compare June 14, 2023 20:23

scothis reviewed Jun 14, 2023

View reviewed changes

LittleBaiBai added 2 commits June 15, 2023 10:15

Use semantic names for errors, check size of slice rather than nil check

25410af

Using emptyCT as desired causes deletions to be recognized as updates…

6e8e6ce

…, switched to nilCT

LittleBaiBai requested a review from scothis June 20, 2023 20:09

scothis mentioned this pull request Jul 25, 2023

ChildSetReconciler as a dynamic composition of ChildReconcilers #396

Merged

scothis closed this Jul 28, 2023

scothis mentioned this pull request Mar 26, 2024

Proxy vmware-labs/reconciler-runtime reconcilerio/runtime#382

Closed

		r.ReflectChildSetStatusOnParent(ctx, resource, childSet, aggregateErr)
		return Result{}, nil

Conversation

LittleBaiBai commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scothis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LittleBaiBai Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LittleBaiBai Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scothis commented Jul 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LittleBaiBai commented Jun 14, 2023 •

edited

Loading

LittleBaiBai Jun 15, 2023 •

edited

Loading

LittleBaiBai Jun 15, 2023 •

edited

Loading