Migrate pkg/proxy to contextual logging: Part 1

[fatsheep9146]

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

1. 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)

2. 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)

3. 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]

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]

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]

/label sig/instrumentation

[k8s-ci-robot]

@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]

/wg structured-logging

[uablrek]

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]

/cc @danwinship @aroradaman

You should probably monitor this

[fatsheep9146]

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.

[aroradaman]

Overall LGTM, few suggestions.

[uablrek]

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]

LGTM label has been added.

Git tree hash: 3544916c905b21be2e77dd83136e5179ac2dc22e

[fatsheep9146]

@danwinship could you help review this again?

[fatsheep9146]

/retest

[fatsheep9146]

/retest

[aroradaman]

/retest

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

[k8s-ci-robot]

@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]

/approve

[k8s-ci-robot]

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

/lgtm

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

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 5a79ecafbd4393b62950efd0e4fae6d9df569a1a