pkg icon indicating copy to clipboard operation
pkg copied to clipboard

Generate reconcileFuncs

Open markusthoemmes opened this issue 5 years ago • 4 comments

The generated reconcilers/controllers had me so hyped up, that I now want to generate all the things 😂.

In all seriousness: What if we could generate the Reconcile${Type} functions as well? They usually follow a common schema:

func ReconcileT(ctx context.Context, desired *type.T) (*type.T, error) {
	lister := type.GetListerForT(ctx)
	client := type.GetClientForT(ctx)
​
	actual, err := lister.Ts(desired.namespace).Get(desired.name)
	if errors.IsNotFound(err) {
		created, err = client.Ts(desired.namespace).Create(desired)
		if err != nil {
			return nil, fmt.Errorf("error creating T: %w", err)
		}
		return created, nil
	} else if err != nil {
		return nil, fmt.Errorf("error fetching T: %w", err)
	} else if *metav1.GetControllerOf(actual) != *metav1.GetControllerOf(desired) {
		return nil, NewNotControlledError(...)
	} else {
		if !equality.Semantic.DeepEqual(desired.Spec, actual.Spec) {
			want := actual.DeepCopy()
			want.Spec = desired.Spec
			updated, err := client.Ts(want.namespace).Update(want)
			if err != nil {
				return nil, fmt.Errorf("error updating T: %w", err)
			}
			return updated, nil
		}
	}
}

They do have diverged quite a bit in Serving though. I looked a bit through all of our instances of such functions and noticed a few (maybe key) differences between some of them:

  1. Some of the emit events when creating/updating succeeds/fails.
  2. Some of them determine DeepEqual including the annotations/labels.
  3. Some of them alter the parent object's Status based on the action that happened.

I wonder if there is room for us to converge on a common set of actions we want to do for each of these reconcile functions and whether or not we can generate them. Writing them is usually mindless boilerplate.

Some ideas on the divergences above: We could add a return value "Event", which can be used to determine what happened in the function to send events (if the reconciler wants to do that) or set status based on that event too.

Note: I don't think the clients/informers are currently available on the context being passed through the reconcile loop, so the interface I've shown is not quite right.

I prototyped this very roughly in Serving here https://github.com/knative/serving/compare/master...markusthoemmes:codegen-reconcilefunc?expand=1.

markusthoemmes avatar Feb 12 '20 10:02 markusthoemmes

/cc @vaikas @n3wscott @mattmoor

markusthoemmes avatar Feb 12 '20 10:02 markusthoemmes

As discussed in slack, I think this is a great idea!! Couple of thoughts, wonder if we should diff labels/annotations as well since at least some of the resources utilize those and it would be good to get them updated properly. I think Event being returned would be good escape hatch to return information that the reconciler might want to act upon, as you touch, sending an event is a good one for sure, there might be others? :)

vaikas avatar Feb 12 '20 11:02 vaikas

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Aug 24 '20 16:08 github-actions[bot]

/lifecycle frozen

I also think that this would be very useful, but we should ensure that it is able to capture the vast majority of our needs.

cc @ImJasonH @vdemeester @afrittoli too as an FYI.

mattmoor avatar Aug 24 '20 16:08 mattmoor