Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 remove redundant watch event handlers via Owns() #10048

Merged
merged 1 commit into from
Apr 10, 2024
Merged

🌱 remove redundant watch event handlers via Owns() #10048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/controllers/machinedeployment/machinedeployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
err = ctrl.NewControllerManagedBy(mgr).
For(&clusterv1.MachineDeployment{}).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adityabhatia WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Owns(&clusterv1.MachineSet{}).
// Watches enqueues MachineDeployment for corresponding MachineSet resources, if no managed controller reference (owner) exists.
Watches(
&clusterv1.MachineSet{},
handler.EnqueueRequestsFromMapFunc(r.MachineSetToDeployments),
Expand Down
1 change: 1 addition & 0 deletions internal/controllers/machineset/machineset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
err = ctrl.NewControllerManagedBy(mgr).
For(&clusterv1.MachineSet{}).
Owns(&clusterv1.Machine{}).
// Watches enqueues MachineSet for corresponding Machine resources, if no managed controller reference (owner) exists.
Watches(
&clusterv1.Machine{},
handler.EnqueueRequestsFromMapFunc(r.MachineToMachineSets),
Expand Down