🐛 Set unhealthyLimitKey for logging always #5110

enxebre · 2021-08-18T13:42:03Z

What this PR does / why we need it:
Without this the controller is panicking as the logger expect every key to be a string.

I0818 10:41:47.830532       1 machinehealthcheck_controller.go:108] controller-runtime/manager/controller/machinehealthcheck "msg"="Reconciling" "name"="agl" "namespace"="clusters-agl" "reconciler group"="cluster.x-k8s.io" "reconciler kind"="MachineHealthCheck"
panic: key is not a string: null

goroutine 640 [running]:
k8s.io/klog/v2/klogr.flatten(0xc00012bf80, 0x6, 0x6, 0xc000d16360, 0x8a)
	/go/pkg/mod/k8s.io/klog/[email protected]/klogr/klogr.go:158 +0x62e
k8s.io/klog/v2/klogr.klogger.Info(0x3, 0x0, 0xc00029a480, 0x38, 0xc001030500, 0xa, 0xa, 0x1b830d7, 0x9, 0x1b969b8, ...)
	/go/pkg/mod/k8s.io/klog/[email protected]/klogr/klogr.go:201 +0x625
sigs.k8s.io/cluster-api/controllers.(*MachineHealthCheckReconciler).reconcile(0xc000481810, 0x1dbd618, 0xc00084f290, 0x1dc70d8, 0xc00084b1d0, 0xc0004384e0, 0xc0009883c0, 0x9, 0xc0004384e0, 0x0, ...)
	/workspace/controllers/machinehealthcheck_controller.go:285 +0x12d4
sigs.k8s.io/cluster-api/controllers.(*MachineHealthCheckReconciler).Reconcile(0xc000481810, 0x1dbd618, 0xc00084f290, 0xc000620660, 0xc, 0xc00062064c, 0x3, 0xc00084ef00, 0x0, 0x0, ...)
	/workspace/controllers/machinehealthcheck_controller.go:164 +0x665
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc0001e4640, 0x1dbd570, 0xc0000a4140, 0x19d25c0, 0xc0003785a0)
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:298 +0x30d
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc0001e4640, 0x1dbd570, 0xc0000a4140, 0xc0004d7700)
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:253 +0x205
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2(0xc0001165f0, 0xc0001e4640, 0x1dbd570, 0xc0000a4140)
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:214 +0x6b
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:210 +0x425

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 #

enxebre · 2021-08-18T13:45:32Z

cc @JoelSpeed

JoelSpeed · 2021-08-18T14:20:32Z

Is there any way we can add a test for this that would panic if we ever regress?

vincepri · 2021-08-18T14:25:58Z

controllers/machinehealthcheck_controller.go

 	var unhealthyLimitKey, unhealthyLimitValue interface{}
+	if m.Spec.UnhealthyRange == nil {
+		unhealthyLimitKey = "max unhealthy"
+		unhealthyLimitValue = m.Spec.MaxUnhealthy
+	} else {
+		unhealthyLimitKey = "unhealthy range"
+		unhealthyLimitValue = *m.Spec.UnhealthyRange
+	}


Do we have to optimize here? Could we just call the logger twice in two ifs below? We could potentially use a switch case if readability is a concern.

I'm not sure I follow, I updated to call the logger twice. PTAL.

JoelSpeed

This definitely makes it easier to follow what's actually happening

I wonder if it's worth moving some of these strings to some consts?

Without this the controller is panicking as the logger expect every key to be a string

JoelSpeed

/lgtm

enxebre · 2021-08-19T10:33:59Z

/retest

enxebre · 2021-08-23T10:12:08Z

@vincepri PTAL

stmcginnis · 2021-08-23T12:05:26Z

controllers/machinehealthcheck_controller.go

 			message = fmt.Sprintf("Remediation is not allowed, the number of not started or unhealthy machines does not fall within the range (total: %v, unhealthy: %v, unhealthyRange: %v)",
 				totalTargets,
 				len(unhealthy),
 				*m.Spec.UnhealthyRange)
 		}

-		logger.V(3).Info(


It seems like we could keep a single log call here, as well as below without needing to add an if/else block. The values can just be set in variables as they were, then only have one call out to the logger. The only thing missing from the existing code was to make sure the key was set to something if remediationAllowed.

Thanks for feedback @stmcginnis. Is this suggesting just the opposite to #5110 (comment)? @vincepri

Seems @JoelSpeed finds it more readable with the if/else #5110 (review)

I don't really have any strong opinion as far as we stop the controller from panicking.

@stmcginnis @JoelSpeed @vincepri please let me know if you want to merge it as it is or make any specific change. Feedback seems to go in both directions atm.

I don't have a really strong opinion on this, but having seen both, I think I prefer the way it is now. While it's more code, it's also more explicit about what's happening and I personally find it easier to follow this way.

Also, thinking about how the logging works, with this approach the log line will include the actual line numbers of the log calls and might make it easier to trace the logging and which if-statements were entered, as opposed with today where you have to work out the values that were substituted and then work out where they were set

vincepri · 2021-08-25T15:01:23Z

/approve

k8s-ci-robot · 2021-08-25T15:01:36Z

[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

enxebre · 2021-08-26T11:27:53Z

/retest

sbueringer · 2021-08-26T11:32:25Z

/retest

To pick kubernetes-sigs/cluster-api#5110 in capi and use capa 0.7.0 release image. Will drop this when plumbing these images through our pipeline.

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

k8s-ci-robot requested review from fabriziopandini and vincepri August 18, 2021 13:42

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 18, 2021

enxebre force-pushed the mhc-log branch from 3b21293 to 7060f75 Compare August 18, 2021 13:56

vincepri reviewed Aug 18, 2021

View reviewed changes

enxebre force-pushed the mhc-log branch from 7060f75 to 8b783c4 Compare August 19, 2021 09:58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 19, 2021

JoelSpeed reviewed Aug 19, 2021

View reviewed changes

Set unhealthyLimitKey for logging always

bdea489

Without this the controller is panicking as the logger expect every key to be a string

enxebre force-pushed the mhc-log branch from 8b783c4 to bdea489 Compare August 19, 2021 10:10

JoelSpeed reviewed Aug 19, 2021

View reviewed changes

k8s-ci-robot assigned JoelSpeed Aug 19, 2021

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

stmcginnis reviewed Aug 23, 2021

View reviewed changes

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

k8s-ci-robot merged commit af60d1d into kubernetes-sigs:master Aug 26, 2021

k8s-ci-robot added this to the v0.4 milestone Aug 26, 2021

csrwng mentioned this pull request Sep 14, 2021

machine health checks causing capi controller to panic openshift/hypershift#471

Closed

enxebre added a commit to enxebre/hypershift that referenced this pull request Sep 14, 2021

Update capi/capa images

eea84ed

To pick kubernetes-sigs/cluster-api#5110 in capi and use capa 0.7.0 release image. Will drop this when plumbing these images through our pipeline.

enxebre added a commit to enxebre/hypershift that referenced this pull request Sep 14, 2021

Update capi/capa images

4166665

To pick kubernetes-sigs/cluster-api#5110 in capi and use capa 0.7.0 release image. Will drop this when plumbing these images through our pipeline.

enxebre added a commit to enxebre/hypershift that referenced this pull request Sep 14, 2021

Update capi/capa images

2642a2b

To pick kubernetes-sigs/cluster-api#5110 in capi and use capa 0.7.0 release image. Will drop this when plumbing these images through our pipeline.

enxebre mentioned this pull request Sep 14, 2021

Update capi/capa images openshift/hypershift#472

Merged

enxebre added a commit to enxebre/hypershift that referenced this pull request Sep 17, 2021

Update capi/capa images

1292908

To pick kubernetes-sigs/cluster-api#5110 in capi and use capa 0.7.0 release image. Will drop this when plumbing these images through our pipeline.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Set unhealthyLimitKey for logging always #5110

🐛 Set unhealthyLimitKey for logging always #5110

enxebre commented Aug 18, 2021 •

edited

Loading

enxebre commented Aug 18, 2021

JoelSpeed commented Aug 18, 2021

vincepri Aug 18, 2021

enxebre Aug 19, 2021

JoelSpeed left a comment

JoelSpeed left a comment

enxebre commented Aug 19, 2021

enxebre commented Aug 23, 2021

stmcginnis Aug 23, 2021

enxebre Aug 24, 2021 •

edited

Loading

JoelSpeed Aug 25, 2021

vincepri commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

enxebre commented Aug 26, 2021

sbueringer commented Aug 26, 2021

🐛 Set unhealthyLimitKey for logging always #5110

🐛 Set unhealthyLimitKey for logging always #5110

Conversation

enxebre commented Aug 18, 2021 • edited Loading

enxebre commented Aug 18, 2021

JoelSpeed commented Aug 18, 2021

vincepri Aug 18, 2021

Choose a reason for hiding this comment

enxebre Aug 19, 2021

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

enxebre commented Aug 19, 2021

enxebre commented Aug 23, 2021

stmcginnis Aug 23, 2021

Choose a reason for hiding this comment

enxebre Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

JoelSpeed Aug 25, 2021

Choose a reason for hiding this comment

vincepri commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

enxebre commented Aug 26, 2021

sbueringer commented Aug 26, 2021

enxebre commented Aug 18, 2021 •

edited

Loading

enxebre Aug 24, 2021 •

edited

Loading