🐛 Improve log k/v pairs and a improve/drop a few log lines #9813

sbueringer · 2023-12-05T13:26:11Z

What this PR does / why we need it:

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 #

sbueringer · 2023-12-05T13:45:51Z

/test pull-cluster-api-e2e-full-main

sbueringer · 2023-12-06T15:02:58Z

/cherry-pick release-1.6

k8s-infra-cherrypick-robot · 2023-12-06T15:03:01Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.6

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.

sbueringer · 2023-12-06T15:03:01Z

/cherry-pick release-1.5

k8s-infra-cherrypick-robot · 2023-12-06T15:03:04Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

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.

sbueringer · 2023-12-07T14:11:52Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

+	if configOwner.GetKind() == "Machine" {
+		// AddOwners adds the owners of Machine as k/v pairs to the logger.
+		// Specifically, it will add KubeadmControlPlane, MachineSet and MachineDeployment.
+		ctx, log, err = clog.AddOwners(ctx, r.Client, configOwner)


We have to call clog.AddOwner with the Machine and not with the KubeadmConfig. Only the Machine has the KCP, MS, MD owner hierarchy

sbueringer · 2023-12-07T14:13:33Z

bootstrap/kubeadm/main.go

@@ -294,7 +294,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {

 	// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
 	// requiring a connection to a remote cluster
-	log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
+	log := ctrl.Log.WithValues("component", "remote/clustercachetracker")


WithName just adds a prefix to all messages. We stopped using that in all other places.

This adds something similar to the controller or webhook k/v pair. I called it component because the CCT is not a controller

What about adding this k/v pair to log inside NewClusterCacheTracker vs doing this before every call to this method?

Good idea. Done

controlplane/kubeadm/internal/controllers/controller.go

sbueringer · 2023-12-07T14:16:21Z

internal/controllers/machineset/machineset_controller.go

@@ -890,7 +890,7 @@ func (r *Reconciler) updateStatus(ctx context.Context, cluster *clusterv1.Cluste
 				availableReplicasCount++
 			}
 		} else if machine.GetDeletionTimestamp().IsZero() {
-			log.Info("Waiting for the Kubernetes node on the machine to report ready state")
+			log.V(4).Info("Waiting for the Kubernetes node on the machine to report ready state")


This is way to verbose on log level 0 and seems totally out-of-place (it's confusing if the MS controller is logging on log level 0 that it's waiting for a node to become ready. It is only doing that for status calculation but otherwise doesn't influence MS reconciliation). Similar logs around here use log level 4

Ever so slight nit to break this out into its own PR.

I moved the event recorder stuff to a separate PR and changed the scope of this PR to "fix k/v paris and improve/drop a few log lines". I think that should be okay

sbueringer · 2023-12-07T14:16:57Z

/cherry-pick release-1.6

k8s-infra-cherrypick-robot · 2023-12-07T14:16:59Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.6

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.

sbueringer · 2023-12-07T14:17:00Z

/cherry-pick release-1.5

k8s-infra-cherrypick-robot · 2023-12-07T14:17:02Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

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.

sbueringer · 2023-12-07T14:22:30Z

/test pull-cluster-api-e2e-full-main

sbueringer · 2023-12-07T14:22:36Z

/assign @fabriziopandini @chrischdi

chrischdi

One question, as of that

/lgtm

controlplane/kubeadm/internal/controllers/controller.go

sbueringer · 2023-12-12T10:19:02Z

/hold cancel

Moved the event recorder change in its own PR and changed the scope of this PR

/assign @fabriziopandini @chrischdi

jackfrancis · 2023-12-12T15:51:24Z

/lgtm

k8s-ci-robot · 2023-12-12T15:51:31Z

LGTM label has been added.

Git tree hash: 14394659cbe9fad6e34e3f8734c240121b4756d6

fabriziopandini

one nit, otherwise lgtm

fabriziopandini · 2023-12-13T12:30:01Z

bootstrap/kubeadm/main.go

@@ -294,7 +294,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {

 	// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
 	// requiring a connection to a remote cluster
-	log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
+	log := ctrl.Log.WithValues("component", "remote/clustercachetracker")


What about adding this k/v pair to log inside NewClusterCacheTracker vs doing this before every call to this method?

fabriziopandini

/lgtm
/approve

k8s-ci-robot · 2023-12-14T13:53:01Z

LGTM label has been added.

Git tree hash: a142ee1bb2bafca69175b5f0e29519aa3afcf520

k8s-ci-robot · 2023-12-14T13:53:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

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

k8s-infra-cherrypick-robot · 2023-12-14T14:17:43Z

@sbueringer: new pull request created: #9880

In response to this:

/cherry-pick release-1.6

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-12-14T14:18:17Z

@sbueringer: #9813 failed to apply on top of branch "release-1.5":

Applying: Improve log k/v pairs and a improve/drop a few log lines
Using index info to reconstruct a base tree...
M	bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
M	bootstrap/kubeadm/main.go
M	controlplane/kubeadm/main.go
M	docs/book/src/tasks/experimental-features/runtime-sdk/implement-extensions.md
M	internal/controllers/cluster/suite_test.go
M	internal/controllers/machine/suite_test.go
M	internal/controllers/machinedeployment/machinedeployment_controller.go
M	internal/controllers/machinedeployment/suite_test.go
M	internal/controllers/machinehealthcheck/suite_test.go
M	internal/controllers/machineset/machineset_controller.go
M	internal/controllers/machineset/suite_test.go
M	internal/controllers/topology/cluster/suite_test.go
M	main.go
M	test/infrastructure/docker/main.go
Falling back to patching base and 3-way merge...
Auto-merging test/infrastructure/docker/main.go
Auto-merging main.go
Auto-merging internal/controllers/topology/cluster/suite_test.go
CONFLICT (content): Merge conflict in internal/controllers/topology/cluster/suite_test.go
Auto-merging internal/controllers/machineset/suite_test.go
Auto-merging internal/controllers/machineset/machineset_controller.go
Auto-merging internal/controllers/machinehealthcheck/suite_test.go
Auto-merging internal/controllers/machinedeployment/suite_test.go
Auto-merging internal/controllers/machinedeployment/machinedeployment_controller.go
Auto-merging internal/controllers/machine/suite_test.go
Auto-merging internal/controllers/cluster/suite_test.go
Auto-merging docs/book/src/tasks/experimental-features/runtime-sdk/implement-extensions.md
Auto-merging controlplane/kubeadm/main.go
Auto-merging bootstrap/kubeadm/main.go
Auto-merging bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Improve log k/v pairs and a improve/drop a few log lines
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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-ci-robot requested review from elmiko and jackfrancis December 5, 2023 13:26

sbueringer changed the title ~~[WIP] 🌱 Improve log k/v pairs & consistent event recorder names~~ [WIP] 🐛 Fix log k/v pairs & consistent event recorder names Dec 6, 2023

sbueringer force-pushed the pr-finalize-logging branch 4 times, most recently from 0e329f6 to fa4d1fe Compare December 7, 2023 14:09

sbueringer commented Dec 7, 2023

View reviewed changes

sbueringer changed the title ~~[WIP] 🐛 Fix log k/v pairs & consistent event recorder names~~ 🐛 Fix log k/v pairs & consistent event recorder names Dec 7, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2023

sbueringer added the area/logging Issues or PRs related to logging label Dec 7, 2023

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Dec 7, 2023

k8s-ci-robot assigned chrischdi and fabriziopandini Dec 7, 2023

chrischdi reviewed Dec 11, 2023

View reviewed changes

controlplane/kubeadm/internal/controllers/controller.go Outdated Show resolved Hide resolved

sbueringer force-pushed the pr-finalize-logging branch from fa4d1fe to 9751194 Compare December 12, 2023 10:12

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

k8s-ci-robot requested review from chrischdi and fabriziopandini December 12, 2023 10:12

sbueringer force-pushed the pr-finalize-logging branch from 9751194 to 525add5 Compare December 12, 2023 10:17

sbueringer changed the title ~~🐛 Fix log k/v pairs & consistent event recorder names~~ 🐛 Improve log k/v pairs and a improve/drop a few log lines Dec 12, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2023

k8s-ci-robot assigned jackfrancis Dec 12, 2023

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

fabriziopandini reviewed Dec 13, 2023

View reviewed changes

Improve log k/v pairs and a improve/drop a few log lines

87e940d

sbueringer force-pushed the pr-finalize-logging branch from 525add5 to 87e940d Compare December 13, 2023 12:45

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

k8s-ci-robot requested review from fabriziopandini and jackfrancis December 13, 2023 12:45

sbueringer mentioned this pull request Dec 13, 2023

Bump to Cluster API v1.7 kubernetes-sigs/cluster-api-provider-vsphere#2549

Closed

7 tasks

fabriziopandini reviewed Dec 14, 2023

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2023

k8s-ci-robot merged commit 23f09dd into kubernetes-sigs:main Dec 14, 2023
21 checks passed

k8s-ci-robot added this to the v1.7 milestone Dec 14, 2023

k8s-infra-cherrypick-robot mentioned this pull request Dec 14, 2023

[release-1.6] 🐛 Improve log k/v pairs and a improve/drop a few log lines #9880

Merged

sbueringer deleted the pr-finalize-logging branch December 14, 2023 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Improve log k/v pairs and a improve/drop a few log lines #9813

🐛 Improve log k/v pairs and a improve/drop a few log lines #9813

sbueringer commented Dec 5, 2023

sbueringer commented Dec 5, 2023

sbueringer commented Dec 6, 2023

k8s-infra-cherrypick-robot commented Dec 6, 2023

sbueringer commented Dec 6, 2023

k8s-infra-cherrypick-robot commented Dec 6, 2023

sbueringer Dec 7, 2023

sbueringer Dec 7, 2023

fabriziopandini Dec 13, 2023

sbueringer Dec 13, 2023 •

edited

Loading

sbueringer Dec 7, 2023

jackfrancis Dec 11, 2023

sbueringer Dec 12, 2023

sbueringer commented Dec 7, 2023

k8s-infra-cherrypick-robot commented Dec 7, 2023

sbueringer commented Dec 7, 2023

k8s-infra-cherrypick-robot commented Dec 7, 2023

sbueringer commented Dec 7, 2023

sbueringer commented Dec 7, 2023

chrischdi left a comment

sbueringer commented Dec 12, 2023 •

edited

Loading

jackfrancis commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

fabriziopandini left a comment

fabriziopandini Dec 13, 2023

fabriziopandini left a comment

k8s-ci-robot commented Dec 14, 2023

k8s-ci-robot commented Dec 14, 2023

k8s-infra-cherrypick-robot commented Dec 14, 2023

k8s-infra-cherrypick-robot commented Dec 14, 2023

🐛 Improve log k/v pairs and a improve/drop a few log lines #9813

🐛 Improve log k/v pairs and a improve/drop a few log lines #9813

Conversation

sbueringer commented Dec 5, 2023

sbueringer commented Dec 5, 2023

sbueringer commented Dec 6, 2023

k8s-infra-cherrypick-robot commented Dec 6, 2023

sbueringer commented Dec 6, 2023

k8s-infra-cherrypick-robot commented Dec 6, 2023

sbueringer Dec 7, 2023

Choose a reason for hiding this comment

sbueringer Dec 7, 2023

Choose a reason for hiding this comment

fabriziopandini Dec 13, 2023

Choose a reason for hiding this comment

sbueringer Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

sbueringer Dec 7, 2023

Choose a reason for hiding this comment

jackfrancis Dec 11, 2023

Choose a reason for hiding this comment

sbueringer Dec 12, 2023

Choose a reason for hiding this comment

sbueringer commented Dec 7, 2023

k8s-infra-cherrypick-robot commented Dec 7, 2023

sbueringer commented Dec 7, 2023

k8s-infra-cherrypick-robot commented Dec 7, 2023

sbueringer commented Dec 7, 2023

sbueringer commented Dec 7, 2023

chrischdi left a comment

Choose a reason for hiding this comment

sbueringer commented Dec 12, 2023 • edited Loading

jackfrancis commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Dec 13, 2023

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 14, 2023

k8s-ci-robot commented Dec 14, 2023

k8s-infra-cherrypick-robot commented Dec 14, 2023

k8s-infra-cherrypick-robot commented Dec 14, 2023

sbueringer Dec 13, 2023 •

edited

Loading

sbueringer commented Dec 12, 2023 •

edited

Loading