component-helpers: Support structured and contextual logging #120637

bells17 · 2023-09-13T17:46:34Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implement support for structured logging and contextual logging in component-helpers.
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

I've unified the implementation to use contextual logging, as some parts of component-helpers were already using contextual logging.

Which issue(s) this PR fixes:

Fixes #120638

Special notes for your reviewer:

I've confirmed that they pass the logcheck tests:

$ GOOS=linux _output/local/bin/golangci-lint run --color=always --config=hack/golangci.yaml ./...

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2023-09-13T17:46:37Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mengjiao-liu · 2023-09-14T03:54:02Z

/wg structured-logging
/area logging

/assign @pohly

leilajal · 2023-09-14T16:34:02Z

/remove-sig api-machinery

bart0sh · 2023-09-15T09:23:00Z

/triage accepted
/priority important-longterm

pohly · 2024-04-21T09:26:24Z

pkg/controller/nodeipam/ipam/cloud_cidr_allocator_test.go

@@ -28,7 +28,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
-	"k8s.io/klog/v2/ktesting"
+	"k8s.io/kubernetes/test/utils/ktesting"


There's no need for this change, can you remove it?

Otherwise it would be better to use tCtx instead of ctx below to indicate that k/k ktesting (in contrast to klog ktesting) returns more than just a context.Context.

k8s-ci-robot · 2024-04-21T10:25:15Z

@bells17: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

bells17 · 2024-04-21T11:52:09Z

@pohly I have made the necessary corrections. Could you please review them again?

pohly

/lgtm

k8s-ci-robot · 2024-04-21T15:45:26Z

LGTM label has been added.

Git tree hash: ec6481fde3a099229eb832ce2697285d60c76ac4

pohly · 2024-04-22T05:44:03Z

/label tide/merge-method-squash

@bells17: should you need to update the PR again, then feel free to squash manually with a single, clean commit message.

aojea · 2024-04-22T06:07:46Z

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

@@ -750,7 +750,7 @@ func nodeAddressesChangeDetected(addressSet1, addressSet2 []v1.NodeAddress) bool
 	return false
 }

-func updateNodeAddressesFromNodeIP(node *v1.Node, nodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) {
+func updateNodeAddressesFromNodeIP(ctx context.Context, node *v1.Node, nodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) {


where is this context used?

Not used (anymore?).

@bells17: can you remove the parameter? Please squash (but don't rebase!) during your next push.

where is this context used?

Not used (anymore?)

Thank you. This change was unnecessary, so I removed it.

@pohly I have squashed the commits into one. I followed the steps below, but please let me know if there are any issues with the procedure.

$ git rebase -i HEAD~6 $ git push origin component-helpers-structured-and-contextual-logging --force

Yes, looks good: https://github.com/kubernetes/kubernetes/compare/25765868bc0ae6c02f9f56f825d168e78a9d7406..5a3fd494b0c9c2f2185c5b905e99996d5a3a8774

aojea · 2024-04-22T09:21:03Z

staging/src/k8s.io/component-helpers/node/util/ips.go

-// cloudProvider is unset or `"external"`.
-func ParseNodeIPArgument(nodeIP, cloudProvider string) ([]net.IP, error) {
+// ParseNodeIPArgument parses kubelet's --node-ip argument.
+// If nodeIP contains invalid values, they will be returned as strings.


this API now is really weird, I assume that is to avoid logging inside a library that ... IIRC we have to do because of the sloppy IPs thing, and is only used in this file and in cmd/kubelet so I think that is not as bad ... @danwinship WDYT?

I'm to blame for the API, see #120637 (comment) for reference 😅

IMHO having a parser function do logging to inform the user should better be avoided. It depends on the context in which the parsing happens if and how the user should be informed, which isn't known to the function.

I added parseNodeIP originally but I feel like sig-node owns it now so it's more up to them.

The logging-in-the-middle-of-a-helper-function was just to preserve the original behavior where it used to be logging directly from cmd/kubelet/app/server.go, and probably I wouldn't have done it that way if I'd had to think about contexts/loggers at the time.

One possibility would be to not change the API of parseNodeIPs, and just have RunKubelet check if len(nodeIPs) == len(strings.Split(kubeServer.NodeIP, ",")) and warn that "some values" were invalid and were ignored if not.

or return (nodeIPs []net.IP, warning, err error) or (nodeIPs []net.IP, fatal bool, err error) (I feel like "fatal" should come after "err" there except that "err" should always be last...)

I was just dealing with the same thing in another PR. Maybe we need a standard convention for functions that want to emit/return both warnings and errors?

Returning warning (presumably a string) and a non-fatal pre-formatted error in the second case both have the problem that they use unstructured formatting (i.e. some flavor of fmt.Sprintf) of a message. That might not work well when the caller uses structured logging. I prefer returning just the information about a problem and leaving the formatting of that to the caller.

len(nodeIPs) == len(strings.Split(kubeServer.NodeIP, ",")) feels like it defeats the purpose of abstracting the parsing in a helper function.

I added parseNodeIP originally but I feel like sig-node owns it now so it's more up to them.

They'll probably just defer to you 😜

Do you want to pull someone else in or do you feel confident going ahead with it as-is and approving it?

Maybe we need a standard convention for functions that want to emit/return both warnings and errors?

Defining conventions is hard, ensuring that fellow developers know about them and follow them is even harder. I suspect unless Go itself defines something, we won't have a chance to establish this on our own for Kubernetes.

@mrunalp: you reviewed this earlier for SIG Node. Any opinion ("I feel like sig-node owns it now so it's more up to them")? ^^^

Do you want to pull someone else in or do you feel confident going ahead with it as-is and approving it?

I don't have approver bits on those directories anyway...

I prefer returning just the information about a problem and leaving the formatting of that to the caller.

I guess that since the semantics of the function are essentially "parse the valid IPs but ignore the invalid ones for backward-compatibility reasons", then returning a list of the invalid IPs does make sense.

I don't have approver bits on those directories anyway...

@mrunalp already approved earlier. What I need from you (or someone from SIG Networking) is approval for pkg/controller/nodeipam/ipam/OWNERS - I know, it's complicated 😢

aojea · 2024-04-22T09:22:43Z

LGTM the networking bits, just one question on the ParseNodeIPArgument function that I prefer @danwinship to take a look as he has more context

danwinship · 2024-04-23T14:29:56Z

/approve
for nodeipam changes

wojtek-t · 2024-04-24T08:59:00Z

/lgtm
/approve

k8s-ci-robot · 2024-04-24T08:59:06Z

LGTM label has been added.

Git tree hash: c18b444697d6bdc0834e3912978b42329d5ebbd5

k8s-ci-robot · 2024-04-24T08:59:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bells17, danwinship, mrunalp, pohly, wojtek-t

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:

~~cmd/kubelet/OWNERS~~ [mrunalp,wojtek-t]
~~cmd/kubemark/OWNERS~~ [wojtek-t]
~~hack/OWNERS~~ [pohly,wojtek-t]
~~pkg/controller/nodeipam/ipam/OWNERS~~ [danwinship,wojtek-t]
~~pkg/kubemark/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/component-helpers/apimachinery/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/component-helpers/node/OWNERS~~ [mrunalp,wojtek-t]

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

…tes#120637)

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 13, 2023

k8s-ci-robot requested review from derekwaynecarr and sttts September 13, 2023 17:47

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 13, 2023

bells17 marked this pull request as ready for review September 13, 2023 17:59

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

k8s-ci-robot requested review from deads2k and yujuhong September 13, 2023 18:00

bells17 force-pushed the component-helpers-structured-and-contextual-logging branch from def55dc to 196cf2a Compare September 13, 2023 18:34

k8s-ci-robot assigned pohly Sep 14, 2023

k8s-ci-robot added wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. area/logging labels Sep 14, 2023

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 14, 2023

bart0sh added this to Triage in SIG Node PR Triage Sep 15, 2023

pohly reviewed Apr 21, 2024

View reviewed changes

bells17 requested a review from pohly April 21, 2024 11:50

pohly approved these changes Apr 21, 2024

View reviewed changes

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

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 22, 2024

aojea reviewed Apr 22, 2024

View reviewed changes

component-helpers: Support structured and contextual logging

5a3fd49

bells17 force-pushed the component-helpers-structured-and-contextual-logging branch from 2576586 to 5a3fd49 Compare April 22, 2024 07:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2024

k8s-ci-robot requested review from aojea and pohly April 22, 2024 07:59

aojea reviewed Apr 22, 2024

View reviewed changes

pohly mentioned this pull request Apr 22, 2024

WIP: convert client-go/tools/cache to contextual logging #121989

Closed

3 tasks

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2024

k8s-ci-robot merged commit 1c917aa into kubernetes:master Apr 24, 2024
15 checks passed

SIG Node CI/Test Board automation moved this from Issues - In progress to Done Apr 24, 2024

SIG Node PR Triage automation moved this from not-sig-node to Done Apr 24, 2024

k8s-ci-robot added this to the v1.31 milestone Apr 24, 2024

bells17 deleted the component-helpers-structured-and-contextual-logging branch April 24, 2024 11:01

jingczhang pushed a commit to nokia/kubernetes that referenced this pull request May 7, 2024

component-helpers: Support structured and contextual logging (kuberne…

f9b189c

…tes#120637)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

component-helpers: Support structured and contextual logging #120637

component-helpers: Support structured and contextual logging #120637

bells17 commented Sep 13, 2023 •

edited

k8s-ci-robot commented Sep 13, 2023

mengjiao-liu commented Sep 14, 2023

leilajal commented Sep 14, 2023

bart0sh commented Sep 15, 2023

pohly Apr 21, 2024

k8s-ci-robot commented Apr 21, 2024 •

edited

bells17 commented Apr 21, 2024

pohly left a comment

k8s-ci-robot commented Apr 21, 2024

pohly commented Apr 22, 2024

aojea Apr 22, 2024

pohly Apr 22, 2024

bells17 Apr 22, 2024

bells17 Apr 22, 2024

pohly Apr 22, 2024

aojea Apr 22, 2024

pohly Apr 22, 2024

danwinship Apr 22, 2024

danwinship Apr 22, 2024

pohly Apr 22, 2024 •

edited

pohly Apr 22, 2024

pohly Apr 22, 2024

danwinship Apr 22, 2024

pohly Apr 23, 2024

aojea commented Apr 22, 2024

danwinship commented Apr 23, 2024

wojtek-t commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

component-helpers: Support structured and contextual logging #120637

component-helpers: Support structured and contextual logging #120637

Conversation

bells17 commented Sep 13, 2023 • edited

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?

k8s-ci-robot commented Sep 13, 2023

mengjiao-liu commented Sep 14, 2023

leilajal commented Sep 14, 2023

bart0sh commented Sep 15, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 21, 2024 • edited

bells17 commented Apr 21, 2024

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 21, 2024

pohly commented Apr 22, 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

Choose a reason for hiding this comment

pohly Apr 22, 2024 • edited

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

aojea commented Apr 22, 2024

danwinship commented Apr 23, 2024

wojtek-t commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

k8s-ci-robot commented Apr 24, 2024

bells17 commented Sep 13, 2023 •

edited

k8s-ci-robot commented Apr 21, 2024 •

edited

pohly Apr 22, 2024 •

edited