Migrate `pkg/proxy` to contextual logging: Part 1 #122979

fatsheep9146 · 2024-01-26T05:14:28Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Use contextual logging for pkg/proxy

After trying to convert pkg/proxy to contextual logging in one pr, I found it's hard to make it and will let the pr grow too large to review.
So I deciced to split the converting job into several parts, here is the part 1.

In this part, I focus on serveral things

Change the functions in cmd/kube-proxy to use context.Context not klog.Logger as input parameters as @danwinship suggested in

Thinking about the eventual migration of pkg/proxy to contextual, in pkg/proxy/nftables at least, we are using context.TODO(), which ought to be fixed at some point to get a proper context.Context from the top level (ie, here). I'm not sure if that has any implications for the right way to migrate these files now...

Originally posted by @danwinship in #122197 (review)

Remove the logger instance in struct ProxyServer, since the "add logger to instance" is often not good option as @pholy said

We had various cases like this in other components. It's always a decision between "add logger" / "add context" / "add logger to instance" - but that last option is often not good because the caller might have a logger with additional key/value pairs that then get ignored when calling a method which uses some other logger that was stored previously.

Originally posted by @pohly in #122197 (comment)

We add a top logger object as subfield for each implementation of proxy.Provider, for example the logger field in struct iptables.Proxier, the reason is that each proxy.Provider will be called as a informer event handler in ProxyServer, there is not a good place to add logger from outside. And there are also some key/values can only be got inside the library of proxy.Provider, for example ipFamily.

Which issue(s) this PR fixes:

Related kubernetes/enhancements#3077

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Migrated the pkg/proxy to use [contextual logging](https://k8s.io/docs/concepts/cluster-administration/system-logs/#contextual-logging).

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging

k8s-ci-robot · 2024-01-26T05:14:37Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2024-01-26T05:14:38Z

Hi @fatsheep9146. 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/test-infra repository.

fatsheep9146 · 2024-01-27T12:02:44Z

/label sig/instrumentation

k8s-ci-robot · 2024-01-27T12:02:46Z

@fatsheep9146: The label(s) /label sig/instrumentation cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, official-cve-feed. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label sig/instrumentation

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.

fatsheep9146 · 2024-01-27T12:04:37Z

/wg structured-logging

uablrek · 2024-01-29T16:58:10Z

The "per-proxier" logger should have WithValue("family", ipFamily). E.g. first in NewProxier() do:

	logger = logger.WithValues("family", ipFamily)

In dual-stack there are two instances of the "proxier", one for IPv4 and one for IPv6. They log individually and often with the same message (and values). It is useful to see which of the proxiers that encounters an error for example.

I had a short discussion with @pohly on slack about performance, and it is fine to create a logger WithValues once.

I made the change above and tested. Here is a sample (json logging used):

{
  "ts": 1706546611912.709,
  "caller": "iptables/proxier.go:630",
  "msg": "Updated proxier node labels",
  "v": 4,
  "family": "IPv4",
  "labels": {
    "beta.kubernetes.io/arch": "amd64",
    "beta.kubernetes.io/os": "linux",
    "kubernetes.io/arch": "amd64",
    "kubernetes.io/hostname": "vm-002",
    "kubernetes.io/os": "linux"
  }
}
{
  "ts": 1706546611912.75,
  "caller": "iptables/proxier.go:630",
  "msg": "Updated proxier node labels",
  "v": 4,
  "family": "IPv6",
  "labels": {
    "beta.kubernetes.io/arch": "amd64",
    "beta.kubernetes.io/os": "linux",
    "kubernetes.io/arch": "amd64",
    "kubernetes.io/hostname": "vm-002",
    "kubernetes.io/os": "linux"
  }
}

uablrek · 2024-01-29T17:00:16Z

/cc @danwinship @aroradaman

You should probably monitor this

fatsheep9146 · 2024-01-30T01:23:30Z

The "per-proxier" logger should have WithValue("family", ipFamily). E.g. first in NewProxier() do:
	logger = logger.WithValues("family", ipFamily)
In dual-stack there are two instances of the "proxier", one for IPv4 and one for IPv6. They log individually and often with the same message (and values). It is useful to see which of the proxiers that encounters an error for example.

I had a short discussion with @pohly on slack about performance, and it is fine to create a logger WithValues once.

I made the change above and tested. Here is a sample (json logging used):
{
  "ts": 1706546611912.709,
  "caller": "iptables/proxier.go:630",
  "msg": "Updated proxier node labels",
  "v": 4,
  "family": "IPv4",
  "labels": {
    "beta.kubernetes.io/arch": "amd64",
    "beta.kubernetes.io/os": "linux",
    "kubernetes.io/arch": "amd64",
    "kubernetes.io/hostname": "vm-002",
    "kubernetes.io/os": "linux"
  }
}
{
  "ts": 1706546611912.75,
  "caller": "iptables/proxier.go:630",
  "msg": "Updated proxier node labels",
  "v": 4,
  "family": "IPv6",
  "labels": {
    "beta.kubernetes.io/arch": "amd64",
    "beta.kubernetes.io/os": "linux",
    "kubernetes.io/arch": "amd64",
    "kubernetes.io/hostname": "vm-002",
    "kubernetes.io/os": "linux"
  }
}

Thanks! I will take this into account.

pkg/proxy/config/config.go

pkg/proxy/iptables/proxier.go

pkg/proxy/util/linebuffer.go

aroradaman

Overall LGTM, few suggestions.

uablrek · 2024-04-11T08:41:14Z

The "root logger" proposed in #122979 (comment) is not included. I think this will be needed in the future, but it's not urgent. Now the global logger is used, which should be ok for now.

/lgtm

(my lgtm will be valid for ipvs only. @danwinship may approve other changes)

k8s-ci-robot · 2024-04-11T08:41:21Z

LGTM label has been added.

Git tree hash: 3544916c905b21be2e77dd83136e5179ac2dc22e

fatsheep9146 · 2024-04-12T01:17:15Z

@danwinship could you help review this again?

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 · 2024-04-22T08:24:23Z

/retest

fatsheep9146 · 2024-04-22T10:50:50Z

/retest

aroradaman · 2024-04-22T11:46:12Z

/retest

https://kubernetes.slack.com/archives/C09QYUH5W/p1713628805680269

k8s-ci-robot · 2024-04-22T11:50:16Z

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

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gci-gce-nftables	`be4535b`	link	false	`/test pull-kubernetes-e2e-gci-gce-nftables`

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.

danwinship · 2024-04-22T21:50:19Z

/approve

k8s-ci-robot · 2024-04-22T21:50:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, fatsheep9146, uablrek

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/kube-proxy/OWNERS~~ [danwinship]
~~pkg/proxy/OWNERS~~ [danwinship]

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

aroradaman · 2024-04-23T07:44:10Z

/lgtm

(label was lost after rebase) (https://github.com/kubernetes/kubernetes/compare/bfbb11ff3f71de275f4d9b22d07babcaf02ab0fe..be4535bd34ac09e57a0b1c478cbcebd051a42598 )

k8s-ci-robot · 2024-04-23T07:44:16Z

LGTM label has been added.

Git tree hash: 5a79ecafbd4393b62950efd0e4fae6d9df569a1a

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

k8s-ci-robot requested review from aojea and smarterclayton January 26, 2024 05:24

k8s-ci-robot added wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 27, 2024

k8s-ci-robot requested review from aroradaman and danwinship January 29, 2024 17:00

danwinship reviewed Jan 31, 2024

View reviewed changes

pkg/proxy/config/config.go Outdated Show resolved Hide resolved

pkg/proxy/iptables/proxier.go Outdated Show resolved Hide resolved

fatsheep9146 force-pushed the pkg-proxy-support-contextual-logging branch from 4178a58 to 239ea97 Compare February 8, 2024 08:38

aroradaman reviewed Apr 9, 2024

View reviewed changes

pkg/proxy/util/linebuffer.go Show resolved Hide resolved

aroradaman reviewed Apr 9, 2024

View reviewed changes

fatsheep9146 force-pushed the pkg-proxy-support-contextual-logging branch from ced8e83 to bfbb11f Compare April 10, 2024 13:41

fatsheep9146 requested review from aroradaman and uablrek April 10, 2024 13:42

k8s-ci-robot assigned uablrek Apr 11, 2024

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

uablrek approved these changes Apr 11, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2024

convert k8s.io/kubernetes/pkg/proxy to contextual logging, part 1

be4535b

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>

fatsheep9146 force-pushed the pkg-proxy-support-contextual-logging branch from bfbb11f to be4535b Compare April 22, 2024 05:29

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 a review from uablrek April 22, 2024 05:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2024

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

k8s-ci-robot assigned aroradaman Apr 23, 2024

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

k8s-ci-robot merged commit 8dd9d1a into kubernetes:master Apr 23, 2024
17 of 18 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `pkg/proxy` to contextual logging: Part 1 #122979

Migrate `pkg/proxy` to contextual logging: Part 1 #122979

fatsheep9146 commented Jan 26, 2024 •

edited

Loading

k8s-ci-robot commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

fatsheep9146 commented Jan 27, 2024

k8s-ci-robot commented Jan 27, 2024

fatsheep9146 commented Jan 27, 2024

uablrek commented Jan 29, 2024

uablrek commented Jan 29, 2024

fatsheep9146 commented Jan 30, 2024

aroradaman left a comment

uablrek commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

fatsheep9146 commented Apr 12, 2024

fatsheep9146 commented Apr 22, 2024

fatsheep9146 commented Apr 22, 2024

aroradaman commented Apr 22, 2024

k8s-ci-robot commented Apr 22, 2024

danwinship commented Apr 22, 2024

k8s-ci-robot commented Apr 22, 2024

aroradaman commented Apr 23, 2024

k8s-ci-robot commented Apr 23, 2024

Migrate pkg/proxy to contextual logging: Part 1 #122979

Migrate pkg/proxy to contextual logging: Part 1 #122979

Conversation

fatsheep9146 commented Jan 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 Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

fatsheep9146 commented Jan 27, 2024

k8s-ci-robot commented Jan 27, 2024

fatsheep9146 commented Jan 27, 2024

uablrek commented Jan 29, 2024

uablrek commented Jan 29, 2024

fatsheep9146 commented Jan 30, 2024

aroradaman left a comment

Choose a reason for hiding this comment

uablrek commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

fatsheep9146 commented Apr 12, 2024

fatsheep9146 commented Apr 22, 2024

fatsheep9146 commented Apr 22, 2024

aroradaman commented Apr 22, 2024

k8s-ci-robot commented Apr 22, 2024

danwinship commented Apr 22, 2024

k8s-ci-robot commented Apr 22, 2024

aroradaman commented Apr 23, 2024

k8s-ci-robot commented Apr 23, 2024

Migrate `pkg/proxy` to contextual logging: Part 1 #122979

Migrate `pkg/proxy` to contextual logging: Part 1 #122979

fatsheep9146 commented Jan 26, 2024 •

edited

Loading