🌱 Fix patch errors not being logged #9224

AndiDog · 2023-08-17T11:49:22Z

What this PR does / why we need it:

~~Log which KubeadmConfig gets reconciled so that every log line is easier to reason about~~
Fix patch errors not being logged (rerr vs. err)

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

n/a

/area logging
/area provider/bootstrap-kubeadm

k8s-ci-robot · 2023-08-17T11:49:30Z

Hi @AndiDog. 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.

killianmuldoon

Thanks for this!

/ok-to-test

related to #6994

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

killianmuldoon · 2023-08-17T12:55:10Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -173,9 +173,11 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	}
 	log = log.WithValues(configOwner.GetKind(), klog.KRef(configOwner.GetNamespace(), configOwner.GetName()), "resourceVersion", configOwner.GetResourceVersion())

-	log = log.WithValues("Cluster", klog.KRef(configOwner.GetNamespace(), configOwner.ClusterName()))
+	log = log.WithValues("Cluster", klog.KRef(configOwner.GetNamespace(), configOwner.ClusterName()), "KubeadmConfig", klog.KRef(config.GetNamespace(), config.GetName()))


This pair should already be included at this point when controller runtime sets up the logger.

You mean KubeadmConfig which I added? I definitely don't see it in a failing test's log, so that's why I wanted to have it.

Example before:

I0817 15:10:42.545337 55146 kubeadmconfig_controller.go:179] "msg"="Reconciling KubeadmConfig" "Cluster"={"name":"cluster","namespace":"default"} "MachinePool"={"name":"worker-machinepool","namespace":"default"} "resourceVersion"="999"

I found controller-runtime to actually fill it for production controller logs (namely in func (blder *Builder) doController(r reconcile.Reconciler) error):

kubeadmconfig_controller.go:314] "Refreshing token until the infrastructure has a chance to consume it" controller="kubeadmconfig" controllerGroup="bootstrap.cluster.x-k8s.io" controllerKind="KubeadmConfig" KubeadmConfig="mynamespace/my-nodepool0"

So my problem is that tests never use the controller-runtime builder but construct KubeadmConfigReconciler{...} directly. Not worth fixing this as I did, so I'll revert. Overall though, having the object logged in the test logs would help the developer experience.

Didn't look at the code but potentially we could try to use the builder in tests as well (i.e. via our SetupWithManager func)

Hm probably more complicated then that

That would fix it - currently most of the KCP tests set up the controller using the raw reconciler e.g.

r := &KubeadmControlPlaneReconciler{

Not sure it's worth it for tests where you can set things up to get info on which cluster etc. is involved in the test code. But would be fine with it if someone wanted to make the changes

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

killianmuldoon

Thanks!

/lgtm

k8s-ci-robot · 2023-08-17T13:26:37Z

LGTM label has been added.

Git tree hash: da08bacbf57728e6d6e3cef9be047fa2fc4540b9

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

sbueringer · 2023-08-17T13:37:53Z

One suggestion #9224 (comment)

sbueringer · 2023-08-17T16:48:49Z

Thx!

/lgtm

/assign @killianmuldoon

k8s-ci-robot · 2023-08-17T16:48:54Z

LGTM label has been added.

Git tree hash: 13c06738c6240fe5b673812a1f0a8c8f5b61107f

killianmuldoon

/lgtm
/approve

Thanks!

k8s-ci-robot · 2023-08-17T17:26:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: killianmuldoon

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

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

sbueringer · 2023-08-17T18:08:33Z

/cherry-pick release-1.5

sbueringer · 2023-08-17T18:08:36Z

/cherry-pick release-1.4

k8s-infra-cherrypick-robot · 2023-08-17T18:09:09Z

@sbueringer: new pull request created: #9234

In response to this:

/cherry-pick release-1.5

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-infra-cherrypick-robot · 2023-08-17T18:09:13Z

@sbueringer: new pull request created: #9235

In response to this:

/cherry-pick release-1.4

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 added area/logging Issues or PRs related to logging area/provider/bootstrap-kubeadm Issues or PRs related to CAPBK cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 17, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 17, 2023

k8s-ci-robot requested review from richardcase and ykakarap August 17, 2023 11:49

killianmuldoon reviewed Aug 17, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2023

killianmuldoon removed the area/provider/bootstrap-kubeadm Issues or PRs related to CAPBK label Aug 17, 2023

AndiDog force-pushed the logging-improvements branch from 979c29e to 43d0c82 Compare August 17, 2023 13:24

killianmuldoon reviewed Aug 17, 2023

View reviewed changes

k8s-ci-robot assigned killianmuldoon Aug 17, 2023

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

sbueringer reviewed Aug 17, 2023

View reviewed changes

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go Outdated Show resolved Hide resolved

AndiDog force-pushed the logging-improvements branch from 43d0c82 to cd72881 Compare August 17, 2023 13:29

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2023

k8s-ci-robot requested a review from killianmuldoon August 17, 2023 13:29

AndiDog changed the title ~~🌱 Logging improvements for KubeadmConfigReconciler~~ 🌱 Fix patch errors not being logged Aug 17, 2023

Fix patch errors not being logged

252ff99

AndiDog force-pushed the logging-improvements branch from cd72881 to 252ff99 Compare August 17, 2023 14:04

k8s-ci-robot assigned sbueringer Aug 17, 2023

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

killianmuldoon reviewed Aug 17, 2023

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 17, 2023

k8s-ci-robot merged commit 62d734c into kubernetes-sigs:main Aug 17, 2023
19 checks passed

k8s-ci-robot added this to the v1.6 milestone Aug 17, 2023

This was referenced Aug 17, 2023

[release-1.5] 🌱 Fix patch errors not being logged #9234

Merged

[release-1.4] 🌱 Fix patch errors not being logged #9235

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Fix patch errors not being logged #9224

🌱 Fix patch errors not being logged #9224

AndiDog commented Aug 17, 2023 •

edited

k8s-ci-robot commented Aug 17, 2023

killianmuldoon left a comment

killianmuldoon Aug 17, 2023

AndiDog Aug 17, 2023

sbueringer Aug 17, 2023 •

edited

sbueringer Aug 17, 2023

killianmuldoon Aug 17, 2023

killianmuldoon left a comment

k8s-ci-robot commented Aug 17, 2023

sbueringer commented Aug 17, 2023

sbueringer commented Aug 17, 2023

k8s-ci-robot commented Aug 17, 2023

killianmuldoon left a comment

k8s-ci-robot commented Aug 17, 2023

sbueringer commented Aug 17, 2023

sbueringer commented Aug 17, 2023

k8s-infra-cherrypick-robot commented Aug 17, 2023

k8s-infra-cherrypick-robot commented Aug 17, 2023

🌱 Fix patch errors not being logged #9224

🌱 Fix patch errors not being logged #9224

Conversation

AndiDog commented Aug 17, 2023 • edited

k8s-ci-robot commented Aug 17, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

killianmuldoon Aug 17, 2023

Choose a reason for hiding this comment

AndiDog Aug 17, 2023

Choose a reason for hiding this comment

sbueringer Aug 17, 2023 • edited

Choose a reason for hiding this comment

sbueringer Aug 17, 2023

Choose a reason for hiding this comment

killianmuldoon Aug 17, 2023

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 17, 2023

sbueringer commented Aug 17, 2023

sbueringer commented Aug 17, 2023

k8s-ci-robot commented Aug 17, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 17, 2023

sbueringer commented Aug 17, 2023

sbueringer commented Aug 17, 2023

k8s-infra-cherrypick-robot commented Aug 17, 2023

k8s-infra-cherrypick-robot commented Aug 17, 2023

AndiDog commented Aug 17, 2023 •

edited

sbueringer Aug 17, 2023 •

edited