Bump clusterloader deps #3050

wojtek-t · 2024-12-13T09:55:13Z

No description provided.

wojtek-t · 2024-12-13T12:21:29Z

clusterloader2/cmd/clusterloader.go

@@ -124,6 +124,7 @@ func validateClusterFlags() *errors.ErrorList {
 }

 func initFlags() {
+	klog.InitFlags(nil)


This is a bit weird - without this line the failure is "--v" flag unknown, with this line it is duplicate failure...

With this line:

./clusterloader flag redefined: v panic: ./clusterloader flag redefined: v goroutine 1 [running]: flag.(*FlagSet).Var(0xc00019e0e0, {0x2bd2ae0, 0x413b730}, {0x2bbca60, 0x1}, {0x25dc117, 0x22}) /usr/local/go/src/flag/flag.go:1028 +0x37d k8s.io/klog/v2.InitFlags.func1(0xc00022c960?) /home/prow/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:447 +0x31 flag.(*FlagSet).VisitAll(0x7fa1040fdb18?, 0xc0009c5c50) /usr/local/go/src/flag/flag.go:458 +0x42 k8s.io/klog/v2.InitFlags(0xc0008cba08?) /home/prow/go/pkg/mod/k8s.io/klog/[email protected]/klog.go:446 +0x3c main.initFlags() /home/prow/go/src/k8s.io/perf-tests/clusterloader2/cmd/clusterloader.go:127 +0x19 main.main() /home/prow/go/src/k8s.io/perf-tests/clusterloader2/cmd/clusterloader.go:261 +0x45

Without this line:

F1213 12:42:20.574783 21458 clusterloader.go:262] Flag parse failed: unknown flag: --v

In which package do you have this problem?

Never mind, it's ./clusterloader.

The command line of clusterloader gets set up in clusterloader2/pkg/flags/flags.go:

pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ContinueOnError) // klog.InitFlags(nil) pflag.CommandLine.AddGoFlagSet(flag.CommandLine)

At the time of that AddGoFlagSet, -v is not in flag.CommandLine yet because the code which now adds it gets invoked later.

The solution is to remove this entire init and do the flag set in a more controlled fashion in main().

diff --git a/clusterloader2/cmd/clusterloader.go b/clusterloader2/cmd/clusterloader.go index f6eab3ec1..810b5ff82 100644 --- a/clusterloader2/cmd/clusterloader.go +++ b/clusterloader2/cmd/clusterloader.go @@ -17,6 +17,7 @@ limitations under the License. package main import ( + "flag" "fmt" "io/ioutil" "net/http" @@ -25,6 +26,7 @@ import ( "path" "time" + "github.com/spf13/pflag" "gopkg.in/yaml.v2" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes" @@ -124,6 +126,8 @@ func validateClusterFlags() *errors.ErrorList { } func initFlags() { + pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ContinueOnError) + pflag.CommandLine.AddGoFlagSet(flag.CommandLine) flags.StringVar(&clusterLoaderConfig.ReportDir, "report-dir", "", "Path to the directory where the reports should be saved. Default is empty, which cause reports being written to standard output.") // TODO(https://github.com/kubernetes/perf-tests/issues/641): Remove testconfig and testoverrides flags when test suite is fully supported. flags.StringArrayVar(&testConfigPaths, "testconfig", []string{}, "Paths to the test config files") diff --git a/clusterloader2/pkg/flags/flags.go b/clusterloader2/pkg/flags/flags.go index 9dc866a41..0288d2b14 100644 --- a/clusterloader2/pkg/flags/flags.go +++ b/clusterloader2/pkg/flags/flags.go @@ -17,7 +17,6 @@ limitations under the License. package flags import ( - "flag" "fmt" "os" "strconv" @@ -27,11 +26,6 @@ import ( "github.com/spf13/pflag" ) -func init() { - pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ContinueOnError) - pflag.CommandLine.AddGoFlagSet(flag.CommandLine) -} - var flags []flagFunc // StringVar creates string flag with given parameters.

perf-tests/clusterloader2$ go run ./cmd --help ... --testoverrides stringArray Paths to the config overrides file. The latter overrides take precedence over changes in former files. --testsuite string Path to the test suite config file -v, --v valueChain number for the log level verbosity --vmodule valueChain comma-separated list of pattern=N settings for file-filtered logging (only works for the default text log format) F1213 15:15:29.635877 74280 clusterloader.go:266] Flag parse failed: pflag: help requested exit status 1

FWIW, the conflict occurs because you are indirectly depending on k8s.io/component-base/logs/testinit:

$ go run ./cmd --help /tmp/go-build2255311573/b001/exe/cmd flag redefined: v panic: /tmp/go-build2255311573/b001/exe/cmd flag redefined: v goroutine 1 [running]: flag.(*FlagSet).Var(0xc0001520e0, {0x2bd1960, 0x413a6b0}, {0x2bbb8c8, 0x1}, {0x25db079, 0x22}) /nvme/gopath/go-1.23.0/src/flag/flag.go:1028 +0x37d k8s.io/component-base/logs.AddGoFlags.func1(0xc0003ffe00?) /nvme/gopath/pkg/mod/k8s.io/[email protected]/logs/logs.go:135 +0x130 flag.(*FlagSet).VisitAll(0x0?, 0xc0004addc0) /nvme/gopath/go-1.23.0/src/flag/flag.go:458 +0x42 k8s.io/component-base/logs.AddGoFlags(0xc0001520e0, {0x0, 0x0, 0x2d?}) /nvme/gopath/pkg/mod/k8s.io/[email protected]/logs/logs.go:121 +0x91 k8s.io/component-base/logs/testinit.init.0() /nvme/gopath/pkg/mod/k8s.io/[email protected]/logs/testinit/testinit.go:31 +0x25 exit status 2 $ go mod why k8s.io/component-base/logs/testinit # k8s.io/component-base/logs/testinit k8s.io/perf-tests/clusterloader2/pkg/measurement/common/metrics k8s.io/kubernetes/test/e2e/framework k8s.io/component-base/logs/testinit

That's when doing the klog.Init in pkg/flags.init, that then runs before testinit. So basically two inits fight over who gets to register the flag.

That k8s.io/component-base/logs/testinit is supposed for tests, so it seemed okay to import it in k8s.io/kubernetes/test/e2e/framework. Why does the clusterloader2 command depend on the test/e2e/framework?

For historical reasons I guess - it was the simplest thing to do. We rely on some utility functions from test/e2e/framework. We can probably copy those and remove this dependency...

Primarily this one: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/ssh/ssh.go

pohly · 2024-12-13T13:58:07Z

clusterloader2/pkg/flags/flags.go

 )

 func init() {
 	pflag.CommandLine = pflag.NewFlagSet(os.Args[0], pflag.ContinueOnError)
-	klog.InitFlags(nil)


Importing a package should never unconditionally change the global flags. That needs to be under control of main().

I understand that this is the less convenient in unit tests. I am proposing https://github.com/kubernetes/kubernetes/tree/master/test/utils/ktesting as a solution for logging in unit tests, but it's currently only available inside Kubernetes.

wojtek-t · 2024-12-13T16:55:06Z

clusterloader2/pkg/measurement/common/slos/api_responsiveness_prometheus_test.go

+
+// FIXME: Remove it.
+func TestKlog(t *testing.T) {
+	klog.LogToStderr(false)


Based on chat with @pohly:

Same problem: test/e2e/framework is opinionated and installs a custom logger during init (test/e2e/framework/ginkgologger.go). The effect is that klog.SetOutput has no effect.

#3051 removed the biggest part.
I will try to finish it and that should hopefully unblock this PR.

wojtek-t · 2024-12-17T12:12:17Z

/assign @tosi3k

tosi3k · 2024-12-17T12:24:39Z

/lgtm
/approve

k8s-ci-robot · 2024-12-17T12:24:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tosi3k, 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:

~~clusterloader2/OWNERS~~ [tosi3k,wojtek-t]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 13, 2024

k8s-ci-robot requested a review from jprzychodzen December 13, 2024 09:55

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

k8s-ci-robot requested a review from mborsz December 13, 2024 09:55

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2024

wojtek-t force-pushed the bump_clusterloader_deps branch 3 times, most recently from a01e58b to 3e002b8 Compare December 13, 2024 11:02

wojtek-t commented Dec 13, 2024

View reviewed changes

wojtek-t force-pushed the bump_clusterloader_deps branch 2 times, most recently from dee5a57 to 19320e6 Compare December 13, 2024 13:41

pohly reviewed Dec 13, 2024

View reviewed changes

wojtek-t mentioned this pull request Dec 13, 2024

Remove dependency on kubernetes test/e2e in clusterloader2 #3051

Merged

wojtek-t force-pushed the bump_clusterloader_deps branch from ce7c008 to 43e9212 Compare December 13, 2024 15:03

wojtek-t mentioned this pull request Dec 13, 2024

[WIP] build(deps): bump k8s.io/kubernetes from 1.29.7 to 1.30.4 in /clusterloader2 #2863

Closed

wojtek-t force-pushed the bump_clusterloader_deps branch from 43e9212 to 80e519b Compare December 13, 2024 15:11

wojtek-t commented Dec 13, 2024

View reviewed changes

wojtek-t force-pushed the bump_clusterloader_deps branch from 80e519b to 07d031d Compare December 13, 2024 17:11

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 13, 2024

wojtek-t added 2 commits December 16, 2024 15:49

Bump clusterloader deps

26e3cb9

Cleanup klog

2c1b670

wojtek-t force-pushed the bump_clusterloader_deps branch from 07d031d to 2d58153 Compare December 16, 2024 14:50

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 16, 2024

wojtek-t force-pushed the bump_clusterloader_deps branch from 2d58153 to 205b28c Compare December 17, 2024 11:10

Fix clusterloader2 failures after flag registration changes

abf58be

wojtek-t force-pushed the bump_clusterloader_deps branch from 205b28c to abf58be Compare December 17, 2024 11:34

wojtek-t changed the title ~~[WIP] Bump clusterloader deps~~ Bump clusterloader deps Dec 17, 2024

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 Dec 17, 2024

k8s-ci-robot assigned tosi3k Dec 17, 2024

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

k8s-ci-robot merged commit e65fb60 into kubernetes:master Dec 17, 2024
7 checks passed

tosi3k mentioned this pull request Dec 18, 2024

Revert "Bump clusterloader deps" #3075

Merged

This was referenced Jan 8, 2025

Cleanup klog in clusterloader #3086

Merged

Bump clusterloader deps #3088

Merged

Bump clusterloader deps #3050

Bump clusterloader deps #3050

Uh oh!

Conversation

wojtek-t commented Dec 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t commented Dec 17, 2024

Uh oh!

tosi3k commented Dec 17, 2024

Uh oh!

k8s-ci-robot commented Dec 17, 2024

Uh oh!

Uh oh!

Uh oh!