Upgrade to klogv2

[serathius]

As part of Structured Logging effort we will be upgrading klog to version 2. This issue will propose plan for upgrading all dependencies and be used to maintain progress.

Part of kubernetes/enhancements#1602

Plan:

• Cut klog release 2.0.0 (@dims)
• Send PRs to k/k dependencies upgrading klog (taken from update klog version to v2 #89717 (comment))
  • k8s.io/gengo (@dims, Switch to klog v2 gengo#173)
  • k8s.io/utils (@dims, Switch to klog v2 utils#152)
  • k8s.io/kube-openapi (@tanjunchen, k8s.io/kube-openapi:upgrade k8s.io/klog to v2 kube-openapi#192)
  • github.com/google/cadvisor (@dims, Switch to klog v2 google/cadvisor#2478)
  • github.com/GoogleCloudPlatform/k8s-cloud-provider (@tanjunchen. [github.com/GoogleCloudPlatform/k8s-cloud-provider]:upgrade k8s.io/klog/v2 GoogleCloudPlatform/k8s-cloud-provider#37, Fix klog.V(x) references for klog v2 GoogleCloudPlatform/k8s-cloud-provider#38)
  • sigs.k8s.io/apiserver-network-proxy (@tanjunchen, Switch to klog v2 [kubernetes-sigs/apiserver-network-proxy] kubernetes-sigs/apiserver-network-proxy#100)
  • k8s.io/repo-infra (@dims, Switch to klog v2 repo-infra#185)
• [Optional] Run scalability tests on draft PR to check for performance regressions (@tosi3k)
• Get approval from root owners & schedule upgrade (@dims)
• Send announcement kubernetes-dev@ (@dims)
• Execute the upgrade (@dims)

/cc @dims @liggitt @smarterclayton @wojtek-t
/milestone v1.19

[dims]

@serathius please add me to utils repo as well.

[dims]

/sig instrumentation
/area code-organization

[serathius]

/cc @44past4

[serathius]

/cc @yuzhiquan

[serathius]

/cc @mm4tt

[zhijianli88]

/cc @zhijianli88

[hase1128]

/cc

[tanjunchen]

/cc
pick up sigs.k8s.io/apiserver-network-proxy
k8s.io/kube-openapi
github.com/GoogleCloudPlatform/k8s-cloud-provider

[fenggw-fnst]

/cc

[dims]

@serathius please mark klog release as DONE ( https://github.com/kubernetes/klog/releases/tag/v2.0.0 )

[serathius]

@mm4tt We are ready to run scalability tests on #89955

[dims]

@serathius please mark k8s.io/utils and k8s.io/klog as done.

[dims]

@serathius filed kubernetes/gengo#173 for gengo

[dims]

@serathius file kubernetes/repo-infra#185 for repo-infra

[dims]

@tanjunchen can you please file the PRs that you picked up and throw us a link?

[tanjunchen]

@dims i have open these prs. As seen above.

[serathius]

Linked all PRs to issue description.

I was planning to find some time and pick one repo on weekend, but you beat me up to it. Thanks for all the help!

[dims]

@serathius kubernetes/gengo#173 has merged.

[tanjunchen]

kubernetes/kube-openapi#192 has been merged .
GoogleCloudPlatform/k8s-cloud-provider#37 has been merged . @serathius

[serathius]

Talked with @mm4tt, he proposed to run 5k release blocking performance tests and do A/B performance analysis.
I will try to find someone from SIG-scalability to help but it might take time due Holidays.

[mm4tt]

SIG Scalability will run the tests by the end of the next week. Will have a more precise estimate today.

[mm4tt]

@tosi3k from SIG Scalability will run the tests

[tosi3k]

Follow-up: I will run the tests tomorrow.

[dims]

@tosi3k @mm4tt @serathius Let's please use #90183 for the tests.

[dims]

@tosi3k @mm4tt @serathius did the scalability CI jobs get started? i have updated #90183 and i haven't heard from y'all

[tosi3k]

@dims sorry for the late update. The 5k CI job based on commit 2f0e334 of #90183 is currently running and will take a few more hours. I will give you more update on Monday.

[tosi3k]

@dims the test succeeded but I'm concerned about the actual commit I run the test on top of. It was the freshest one when I left work on Friday but I saw one with description switch over k/k to use klog v2 being pushed afterwards, hence I believe the test I started on Friday didn't really make sense :(. Shall I rerun the tests on top of the most recent change (which is cd1edd0 I believe but please correct me if I'm wrong)?

[serathius]

@tosi3k Thanks for running it.
Could you provide link to dashboard with results?

I think intention was to do A/B tests. So run tests twice, one before the update and one after update. @mm4tt can you confirm?

[mm4tt]

@serathius, you're right, but we don't need to run the test twice manually. We can use a daily 5k CI/CD run as a baseline.
That being said, before we spin up the dashboards, I'd like to get answer to @tosi3k's question, i.e. whether the manual run he did actually had klogv2 enabled.

[dims]

@serathius my bad, i had not pushed everything needed before i added that note :(

[mm4tt]

No worries, @tosi3k can confirm, but I think he reserved a slot for re-running the tests tomorrow.

@dims, could you please double check and give us an explicit green light confirming that the PR is ready to be tested?

[dims]

@mm4tt yes, what is in #90183 is good. specifically cd1edd0c0e06af7c4dd02838d55ccc84586c89fe is the last commit

For next time best way to cross check is ensuring that "k8s.io/klog" is removed from root go.mod/sum and "k8s.io/klog/v2" is added there instead.

-	k8s.io/klog v1.0.0
+	k8s.io/klog/v2 v2.0.0

[tosi3k]

Okay, I will run the test using cd1edd0 commit tomorrow and compare the results with our daily 5k CI/CD test run the day after tomorrow then.

[tosi3k]

I ran a test job with the experiment and the test passed. Looking at the dashboards I noticed a few changes:

• Clusterloader
  LIST calls for pods in the namespace scope take more time on average than before change (1.63s vs. 3.59s),
• kube-apiserver
  slightly higher variance of memory allocation for kube-apiserver (but the memory usage stays the same on average),

Overall, the results look good (at least from my novice perspective).

Dashboard with metrics before the change: link (source: grafana-prom).
Dashboard with metrics after the change: link (source: klogv2-experiment).

[serathius]

@mm4tt does this look ok for you?

[mm4tt]

Thanks, @tosi3k!

The latency of the pod lists seems suspicious, but my intuition says it's a red herring, most likely coming from the different setup of the test runs. The CI/CD test uses configuration than the manual test. For manual test, we used our scalability prow instance, it's likely that the cl2 was run in different region than the cluster, which could affect the pod list (IIRC, cl2 is the main issuer of these calls).

So yes, to summarize it looks ok. We don't see any obvious scalability blockers for klogv2.

[serathius]

Thanks @dims for all great work!