🌱 remove redundant watch event handlers via Owns()

[adityabhatia]

What this PR does / why we need it:

Owns() watches for child resources and enqueues requests for owners (owner reference) with controller reference. This PR aims at removing Owns() where such enqueuing is already in place via Watches(), thereby removing both redundant watch and enqueuing of resources.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[k8s-ci-robot]

Hi @adityabhatia. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@adityabhatia: The label(s) area/help cannot be applied, because the repository doesn't have them.

In response to this:

/area help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[adityabhatia]

/area machine
/area machineset

[sbueringer]

@adityabhatia let me know when/if I should take a look (I'm ignoring WIP PRs per default if nobody asks for a review)

[adityabhatia]

@adityabhatia let me know when/if I should take a look (I'm ignoring WIP PRs per default if nobody asks for a review)

Sure please have a look. I wanted to create an issue for it, explaining the goal (to do a feasibility check).

Eg.

		Owns(&clusterv1.MachineSet{}).
		Watches(
			&clusterv1.MachineSet{},
			handler.EnqueueRequestsFromMapFunc(r.MachineSetToDeployments),
		)

here Owns() (internally translates to a watching and enqueuing owner resources of controller type) and Watches() do the same thing. If the idea sounds good, I can go ahead and create an issue with a more detailed description. 🙂

[sbueringer]

This is only equivalent to Owns if we basically implement the same thing. But MachineSetToDeployments does more than that. I assume because we can't always rely on ownerRefs

[adityabhatia]

Makes sense, the labels are there as backup. Going by that logic, now thinking from the opposite perspective wouldn't Owns() be redundant?

[sbueringer]

Assuming our implementation is bug free (which should be easy to verify). Yes!

[sbueringer]

I took a closer look into the mapping functions. Looks like they don't enqueue at all based on ownerRefs

	// Check if the controller reference is already set and
	// return an empty result when one is found.
	for _, ref := range ms.ObjectMeta.GetOwnerReferences() {
		if ref.Controller != nil && *ref.Controller {
			return result
		}
	}

I think the goal of these function is to only enqueue if owner is not set. And Owns then takes care of it once the owner is set.

// MachineSetToDeployments is a handler.ToRequestsFunc to be used to enqueue requests for reconciliation
// for MachineDeployments that might adopt an orphaned MachineSet.
func (r *Reconciler) MachineSetToDeployments(ctx context.Context, o client.Object) []ctrl.Request {

Maybe we should add the comment here in the builder as well? (and of course keep Owns)

[sbueringer]

@adityabhatia WDYT?

[adityabhatia]

Sry somehow lost track of this PR, I added comments now.

[sbueringer]

Thank you!

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: bb67874ce29a28459d0f7339124d389f3d6c690d

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

/ok-to-test