🌱 Reuse `hasMatchingLabels` in MachineSet controller #4652

enxebre · 2021-05-24T10:27:53Z

What this PR does / why we need it:
This let the machineSet reconciler reuse the logic from hasMatchingLabels.
It also fix getMachineSetsForMachine to return error appropriately to let the caller ignore it or not.

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 #

JoelSpeed

One quick question, otherwise seems reasonable to me

controllers/machineset_controller.go

controllers/machineset_controller_test.go

controllers/machineset_controller.go

srm09

some minor nits/suggestions.

srm09 · 2021-05-26T17:21:24Z

controllers/machineset_controller.go

-		log.Error(err, "Failed to list machine sets")
-		return nil
+	if err := r.Client.List(ctx, msList, client.InNamespace(m.Namespace)); err != nil {
+		return nil, fmt.Errorf("failed to list MachineSets: %w", err)


nit: Maybe also re-introduce the logger here.

Suggested change

return nil, fmt.Errorf("failed to list MachineSets: %w", err)

log.Error(err, "Failed to list machine sets")

return nil, errors.Wrapf(err, "failed to list MachineSets")

The second one is more of a general thing, in most of the places in the codebase we use Wrapf instead of Errorf. Maybe we should use a single practice across (with the emphasis on maybe)

errors.Wrapf is around probably only because there was no native support for it before golang 1.13. Unless there's a reason to not I suggest we rely on the standard errors library package and follow fmt.Errorf %w", err) pattern for any upcoming changes rather than perpetuate the old pattern.
https://blog.golang.org/go1.13-errors#TOC_3.3.

I agree on the fmt.Errorf, but I like the suggestion to keep the logging.

This function is now returning an appropriate concatenated error string. It's a consumer of the function choice to handle the error, which in this case is already logging, so if we log here it'd be duplicated https://github.com/kubernetes-sigs/cluster-api/pull/4652/files#diff-2678c5ef8cd150e20f9ea965dcaaea54e17e2c8010eb37447b5745760aca890fR538-R540

We usually use the pkg/errors package instead of fmt

@vincepri Is there a reason to keep perpetuating that pattern and an external dependency rather than using the standard core package? I assume we are using pkg/errors only because before Golang 1.13 there was no builtin way to wrap errors? If that's the case I'd be in favour of using the standard library for upcoming changes fmt.Errorf %w", err).
No need to bikeshed on this though, I'm good either way though I would like to get context and understand the motivation.

Updated anyways so we can move along.

pkg/errors is in general easier to use, and it's used pretty much everywhere within k8s https://grep.app/search?q=pkg/errors&filter[repo][0]=kubernetes/kubernetes — In general though, if it's not broken why switch?

controllers/machineset_controller.go

enxebre · 2021-06-08T08:31:43Z

/test pull-cluster-api-test-main

enxebre · 2021-06-08T19:07:02Z

/test pull-cluster-api-test-main

JoelSpeed

/lgtm

vincepri · 2021-06-09T15:10:48Z

controllers/machineset_controller.go

@@ -519,6 +519,7 @@ func (r *MachineSetReconciler) waitForMachineDeletion(ctx context.Context, machi
 // MachineToMachineSets is a handler.ToRequestsFunc to be used to enqeue requests for reconciliation
 // for MachineSets that might adopt an orphaned Machine.
 func (r *MachineSetReconciler) MachineToMachineSets(o client.Object) []ctrl.Request {
+	log := ctrl.LoggerFrom(context.TODO(), "object", client.ObjectKeyFromObject(o))


FYI, this will never log unless the global logger is set

Added a comment

vincepri · 2021-06-09T15:11:23Z

controllers/machineset_controller.go

@@ -534,7 +535,11 @@ func (r *MachineSetReconciler) MachineToMachineSets(o client.Object) []ctrl.Requ
 		}
 	}

-	mss := r.getMachineSetsForMachine(context.TODO(), m)
+	mss, err := r.getMachineSetsForMachine(context.TODO(), m)


Can we define this context only once at the top of the function?

vincepri · 2021-06-09T15:13:52Z

/retitle 🌱 Reuse hasMatchingLabels in MachineSet controller

This let the machineSet reconciler reuse the logic from hasMatchingLabels. It also fix getMachineSetsForMachine to return error appropriately to let the caller ignore them or not.

vincepri

/approve
/lgtm

k8s-ci-robot · 2021-06-09T16:35:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2021

k8s-ci-robot requested review from CecileRobertMichon and sbueringer May 24, 2021 10:27

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 24, 2021

enxebre changed the title ~~Reuse hasMatchingLabels~~ 🌱 Reuse hasMatchingLabels May 24, 2021

JoelSpeed reviewed May 24, 2021

View reviewed changes

controllers/machineset_controller.go Outdated Show resolved Hide resolved

vincepri reviewed May 24, 2021

View reviewed changes

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/machineset_controller_test.go Show resolved Hide resolved

sbueringer reviewed May 25, 2021

View reviewed changes

controllers/machineset_controller.go Outdated Show resolved Hide resolved

controllers/machineset_controller.go Outdated Show resolved Hide resolved

srm09 reviewed May 26, 2021

View reviewed changes

enxebre force-pushed the hasMatchingLabels branch 2 times, most recently from c1ebebe to 55c20f3 Compare May 31, 2021 09:26

enxebre force-pushed the hasMatchingLabels branch from 55c20f3 to 0624a57 Compare June 8, 2021 08:22

JoelSpeed reviewed Jun 9, 2021

View reviewed changes

k8s-ci-robot assigned JoelSpeed Jun 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021

vincepri reviewed Jun 9, 2021

View reviewed changes

k8s-ci-robot changed the title ~~🌱 Reuse hasMatchingLabels~~ 🌱 Reuse hasMatchingLabels in MachineSet controller Jun 9, 2021

Reuse hasMatchingLabels

9ad4ea8

This let the machineSet reconciler reuse the logic from hasMatchingLabels. It also fix getMachineSetsForMachine to return error appropriately to let the caller ignore them or not.

enxebre force-pushed the hasMatchingLabels branch from 0624a57 to 9ad4ea8 Compare June 9, 2021 15:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021

vincepri approved these changes Jun 9, 2021

View reviewed changes

k8s-ci-robot assigned vincepri Jun 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2021

k8s-ci-robot merged commit 45a0abb into kubernetes-sigs:master Jun 9, 2021

k8s-ci-robot added this to the v0.4 milestone Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Reuse `hasMatchingLabels` in MachineSet controller #4652

🌱 Reuse `hasMatchingLabels` in MachineSet controller #4652

enxebre commented May 24, 2021 •

edited

Loading

JoelSpeed left a comment

srm09 left a comment

srm09 May 26, 2021

enxebre May 31, 2021 •

edited

Loading

stmcginnis Jun 3, 2021

enxebre Jun 3, 2021

vincepri Jun 7, 2021

enxebre Jun 8, 2021 •

edited

Loading

enxebre Jun 8, 2021

vincepri Jun 8, 2021

enxebre commented Jun 8, 2021

enxebre commented Jun 8, 2021

JoelSpeed left a comment

vincepri Jun 9, 2021

enxebre Jun 9, 2021

vincepri Jun 9, 2021

enxebre Jun 9, 2021

vincepri commented Jun 9, 2021

vincepri left a comment

k8s-ci-robot commented Jun 9, 2021

	return nil, fmt.Errorf("failed to list MachineSets: %w", err)
	log.Error(err, "Failed to list machine sets")
	return nil, errors.Wrapf(err, "failed to list MachineSets")

🌱 Reuse hasMatchingLabels in MachineSet controller #4652

🌱 Reuse hasMatchingLabels in MachineSet controller #4652

Conversation

enxebre commented May 24, 2021 • edited Loading

JoelSpeed left a comment

Choose a reason for hiding this comment

srm09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Jun 8, 2021

enxebre commented Jun 8, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jun 9, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 9, 2021

🌱 Reuse `hasMatchingLabels` in MachineSet controller #4652

🌱 Reuse `hasMatchingLabels` in MachineSet controller #4652

enxebre commented May 24, 2021 •

edited

Loading

enxebre May 31, 2021 •

edited

Loading

enxebre Jun 8, 2021 •

edited

Loading