cluster-bootstrap: Support structured and contextual logging #127658

soma00333 · 2024-09-26T10:55:18Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

I implemented the support for structured and contextual logging in cluster-bootstrap, following the guideline:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

In addition, I've modified the log messages based on the following guideline:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#remove-string-formatting-from-log-message

I've confirmed that they pass the logcheck tests.
You can run logcheck with make test or make logcheck

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

I've confirm that they pass the logcheck tests.

$GOPATH/bin/logcheck -check-contextual ./staging/src/k8s.io/cluster-bootstrap/...

$GOPATH/bin/logcheck -check-structured ./staging/src/k8s.io/cluster-bootstrap/...

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

k8s-ci-robot · 2024-09-26T10:55:28Z

Hi @soma00333. Thanks for your PR.

I'm waiting for a kubernetes 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-sigs/prow repository.

k8s-ci-robot · 2024-09-26T10:55:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soma00333
Once this PR has been reviewed and has the lgtm label, please assign deads2k, neolit123 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

hack/OWNERS
pkg/controller/OWNERS
plugin/pkg/auth/authenticator/OWNERS
staging/src/k8s.io/cluster-bootstrap/OWNERS

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

bells17 · 2024-09-27T12:18:49Z

/triage accepted

bells17 · 2024-09-27T14:18:49Z

/ok-to-test

soma00333 · 2024-09-28T00:12:07Z

The error is caused by a race condition in the Test_Run_Positive_VolumeMountControllerAttachEnabledRace test. It highlights two key issues:

Volume Reconstruction Failure: The test cannot find the expected directory (fake-dir), resulting in a failure to reconstruct volumes from disk. This is shown by the error message: Cannot get volumes from disk, skip sync states for volume reconstruction.
Panic Due to Closed Channel: The test encounters a panic caused by attempting to close an already closed channel, as seen in the repeated message: recovered from panic close of closed channel.

Try running the test again.

soma00333 · 2024-09-28T00:14:51Z

/retest-required

nilekhc · 2024-10-07T16:29:51Z

/assign @stlaz

stlaz · 2024-11-11T13:55:01Z

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go

@@ -45,8 +45,8 @@ func GetData(secret *v1.Secret, key string) string {
 }

 // HasExpired will identify whether the secret expires
-func HasExpired(secret *v1.Secret, currentTime time.Time) bool {
-	_, expired := GetExpiration(secret, currentTime)
+func HasExpired(logger klog.Logger, secret *v1.Secret, currentTime time.Time) bool {


I believe we should use a module-level logger here instead of wiring it this way.

Hmmmm, I see that that's not a pattern that kubernetes follows. That's a shame. You can ignore the above comment.

I appreciate your comment.
We can pass ctx instead of logger.
e.g.

func HasExpired(ctx context.Context, secret *v1.Secret, currentTime time.Time) bool {}

Do you prefer this?

I think it's ok to just use what you've got. I originally came from python where you'd set up a logger per-module so that you don't have to pass it in arguments. It's possible that that pattern would be harder to implement in golang given it's a compiled language.

stlaz · 2024-11-11T14:00:04Z

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go

-		klog.V(3).Infof("Unparseable expiration time (%s) in %s/%s Secret: %v. Treating as expired.",
-			expiration, secret.Namespace, secret.Name, err)
+		logger.V(3).Info("Unparseable expiration time in Secret. Treating as expired.",
+			"expiration", expiration, "namespace", secret.Namespace, "secretName", secret.Name, "error", err)


Suggested change

"expiration", expiration, "namespace", secret.Namespace, "secretName", secret.Name, "error", err)

"expirationTime", expiration, "secretNamespace", secret.Namespace, "secretName", secret.Name, "err", err)

I fixed here

stlaz · 2024-11-11T14:00:39Z

staging/src/k8s.io/cluster-bootstrap/util/secrets/secrets.go

-		klog.V(3).Infof("Expired bootstrap token in %s/%s Secret: %v",
-			secret.Namespace, secret.Name, expiration)
+		logger.V(3).Info("Expired bootstrap token in Secret.",
+			"namespace", secret.Namespace, "secretName", secret.Name, "expiration", expiration)


Suggested change

"namespace", secret.Namespace, "secretName", secret.Name, "expiration", expiration)

"secretNamespace", secret.Namespace, "secretName", secret.Name, "expiration", expiration)

I fixed here

stlaz · 2024-11-11T14:33:07Z

/lgtm

k8s-ci-robot · 2024-11-11T14:33:13Z

LGTM label has been added.

Git tree hash: dc5daf247d5f46c0bd6f03abfdb43751ba0263e1

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 26, 2024

k8s-ci-robot requested review from atiratree and enj September 26, 2024 10:55

soma00333 marked this pull request as ready for review September 26, 2024 11:00

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 Sep 26, 2024

k8s-ci-robot requested a review from CecileRobertMichon September 26, 2024 11:00

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 27, 2024

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 Sep 27, 2024

k8s-ci-robot assigned stlaz Oct 7, 2024

stlaz suggested changes Nov 11, 2024

View reviewed changes

stlaz reviewed Nov 11, 2024

View reviewed changes

Support structured and contextual logging for cluster-bootstrap

1a03f12

soma00333 force-pushed the feature/cluster-bootstrap-structured-and-contextual-logging branch from 5ef215f to 1a03f12 Compare November 11, 2024 14:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster-bootstrap: Support structured and contextual logging #127658

cluster-bootstrap: Support structured and contextual logging #127658

soma00333 commented Sep 26, 2024 •

edited

Loading

k8s-ci-robot commented Sep 26, 2024

k8s-ci-robot commented Sep 26, 2024

bells17 commented Sep 27, 2024

bells17 commented Sep 27, 2024

soma00333 commented Sep 28, 2024 •

edited

Loading

soma00333 commented Sep 28, 2024

nilekhc commented Oct 7, 2024

stlaz Nov 11, 2024

stlaz Nov 11, 2024

soma00333 Nov 11, 2024

stlaz Nov 11, 2024

stlaz Nov 11, 2024

soma00333 Nov 11, 2024

stlaz Nov 11, 2024

soma00333 Nov 11, 2024

stlaz commented Nov 11, 2024

k8s-ci-robot commented Nov 11, 2024

	"expiration", expiration, "namespace", secret.Namespace, "secretName", secret.Name, "error", err)
	"expirationTime", expiration, "secretNamespace", secret.Namespace, "secretName", secret.Name, "err", err)

	"namespace", secret.Namespace, "secretName", secret.Name, "expiration", expiration)
	"secretNamespace", secret.Namespace, "secretName", secret.Name, "expiration", expiration)

cluster-bootstrap: Support structured and contextual logging #127658

Are you sure you want to change the base?

cluster-bootstrap: Support structured and contextual logging #127658

Conversation

soma00333 commented Sep 26, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 26, 2024

k8s-ci-robot commented Sep 26, 2024

bells17 commented Sep 27, 2024

bells17 commented Sep 27, 2024

soma00333 commented Sep 28, 2024 • edited Loading

soma00333 commented Sep 28, 2024

nilekhc commented Oct 7, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Nov 11, 2024

k8s-ci-robot commented Nov 11, 2024

soma00333 commented Sep 26, 2024 •

edited

Loading

soma00333 commented Sep 28, 2024 •

edited

Loading