[go] Bump images, dependencies and versions to go 1.24.0

[cpanato]

What type of PR is this?

/kind feature
/hold

What this PR does / why we need it:

• Bump images, dependencies and versions to go 1.24.0

Which issue(s) this PR fixes:

Part of kubernetes/release#3772

/assign @MadhavJivrajani @liggitt @puerco @xmudrii @Verolop
cc @kubernetes/release-managers

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Kubernetes is now built with go 1.24.0

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

[cpanato]

/assign @MadhavJivrajani @liggitt @puerco @xmudrii @Verolop

[liggitt]

I was really hoping sig-storage / sig-windows would complete the sweep and update of windows symlink handling to complete our transition to go 1.23 before we moved on to go 1.24 :-/

Resolution of #129084 should be higher priority than go 1.24, in my opinion (which is tricky since the folks working on that are non-overlapping with the ones updating to go 1.24)

[pohly]

you cna push to your branch and then i can cherry pick

https://github.com/kubernetes/kubernetes/compare/master...pohly:kubernetes:update-main-go124?expand=1

[liggitt]

If they're not new checks which are newly failing with go 1.24, we should figure out why and decide whether to fix or disable the checks case-by-case.

Does it matter why we now get these issues reported? Isn't it more important whether they are real bugs that must be fixed?

I completely agree we should fix real issues. Understanding why they're newly reported would help us know if they are new issues related to the go 1.24 bump in some way that we should fix with this bump, rather than in a follow-up.

(and I agree the stylistic non-bug linting we should just ignore / suppress, likely forever)

[liggitt]

/lgtm
/approve

once CI is happy

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 3be923904d98da3bdc4a4cc458967247d0140601

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, dims, liggitt

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:

• OWNERS [dims,liggitt]
• build/OWNERS [dims,liggitt]
• hack/OWNERS [dims,liggitt]
• pkg/scheduler/OWNERS [dims,liggitt]
• plugin/pkg/auth/authorizer/node/OWNERS [dims,liggitt]
• staging/publishing/OWNERS [dims,liggitt]
• staging/src/k8s.io/api/OWNERS [liggitt]
• staging/src/k8s.io/apiextensions-apiserver/OWNERS [dims,liggitt]
• staging/src/k8s.io/apimachinery/OWNERS [dims,liggitt]
• staging/src/k8s.io/apiserver/OWNERS [dims,liggitt]
• staging/src/k8s.io/cli-runtime/OWNERS [dims,liggitt]
• staging/src/k8s.io/client-go/OWNERS [dims,liggitt]
• staging/src/k8s.io/cloud-provider/OWNERS [dims,liggitt]
• staging/src/k8s.io/cluster-bootstrap/OWNERS [dims,liggitt]
• staging/src/k8s.io/code-generator/OWNERS [dims,liggitt]
• staging/src/k8s.io/component-base/OWNERS [dims,liggitt]
• staging/src/k8s.io/component-helpers/OWNERS [dims,liggitt]
• staging/src/k8s.io/controller-manager/OWNERS [dims,liggitt]
• staging/src/k8s.io/cri-api/OWNERS [dims,liggitt]
• staging/src/k8s.io/cri-client/OWNERS [liggitt]
• staging/src/k8s.io/csi-translation-lib/OWNERS [dims,liggitt]
• staging/src/k8s.io/dynamic-resource-allocation/OWNERS [dims,liggitt]
• staging/src/k8s.io/endpointslice/OWNERS [dims,liggitt]
• staging/src/k8s.io/externaljwt/OWNERS [dims,liggitt]
• staging/src/k8s.io/kms/OWNERS [dims,liggitt]
• staging/src/k8s.io/kube-aggregator/OWNERS [dims,liggitt]
• staging/src/k8s.io/kube-controller-manager/OWNERS [dims,liggitt]
• staging/src/k8s.io/kube-proxy/OWNERS [dims,liggitt]
• staging/src/k8s.io/kube-scheduler/OWNERS [dims,liggitt]
• staging/src/k8s.io/kubectl/OWNERS [dims,liggitt]
• staging/src/k8s.io/kubelet/OWNERS [dims,liggitt]
• staging/src/k8s.io/metrics/OWNERS [dims,liggitt]
• staging/src/k8s.io/mount-utils/OWNERS [dims,liggitt]
• staging/src/k8s.io/pod-security-admission/OWNERS [dims,liggitt]
• staging/src/k8s.io/sample-apiserver/OWNERS [dims,liggitt]
• staging/src/k8s.io/sample-cli-plugin/OWNERS [dims,liggitt]
• staging/src/k8s.io/sample-controller/OWNERS [dims,liggitt]
• test/e2e/storage/OWNERS [dims,liggitt]
• test/images/OWNERS [dims,liggitt]
• test/utils/image/OWNERS [dims,liggitt]
• vendor/OWNERS [dims,liggitt]

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

[pohly]

Understanding why they're newly reported would help us know if they are new issues related to the go 1.24 bump in some way that we should fix with this bump, rather than in a follow-up.

Ack.

We can ignore gocritic. All of the reported issue are stylistic and not regressions caused by Go 1.24.

We can ignore testifylint and ginkgolinter. I'm pretty sure these are enhanced checks and only occur in tests.

govet's printf check is new. We knew that the Go team decided to enable that by default and fixed some occurrences, but since then this one got added in 9b6ef25:

ERROR: test/e2e/node/mount_propagation.go:200:18: printf: non-constant format string in call to k8s.io/kubernetes/test/e2e/framework.Logf (govet)
ERROR: 		framework.Logf(enterKubeletMountNS)

That leaves govet's lostcancel:

ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:999:2: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)
ERROR: 	cancelCtx, cancel = context.WithCancelCause(ctx)
ERROR: 	^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1004:1: lostcancel: this return statement may be reached without using the cancel var defined on line 999 (govet)
ERROR: }
ERROR: ^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1050:2: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)
ERROR: 	cancelCtx, cancel = context.WithCancelCause(ctx)
ERROR: 	^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1062:1: lostcancel: this return statement may be reached without using the cancel var defined on line 1050 (govet)
ERROR: }
ERROR: ^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1115:2: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)
ERROR: 	cancelCtx, cancel = context.WithCancelCause(ctx)
ERROR: 	^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1127:1: lostcancel: this return statement may be reached without using the cancel var defined on line 1115 (govet)
ERROR: }
ERROR: ^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1187:2: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)
ERROR: 	cancelCtx, cancel = context.WithCancelCause(ctx)
ERROR: 	^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1201:1: lostcancel: this return statement may be reached without using the cancel var defined on line 1187 (govet)
ERROR: }
ERROR: ^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1265:3: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)
ERROR: 		cancelCtx, cancel = context.WithCancelCause(ctx)
ERROR: 		^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1275:1: lostcancel: this return statement may be reached without using the cancel var defined on line 1265 (govet)
ERROR: }
ERROR: ^

I think that is because of added support for context.WithCancelCause. If I replace that with context.WithCancel on current master without this PR, I get the same warning:

ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1265:3: lostcancel: the cancel function is not used on all paths (possible context leak) (govet)
ERROR: 		cancelCtx, cancel = context.WithCancel(ctx)
ERROR: 		^
ERROR: staging/src/k8s.io/client-go/tools/cache/reflector_test.go:1275:1: lostcancel: this return statement may be reached without using the cancel var defined on line 1265 (govet)
ERROR: }
ERROR: ^

However, I'm not seeing anything in the Go release notes about this added support. Perhaps it was considered too minor to be mentioned.

[rajibmitra]

@pohly maybe we can try adding defer cancel() after each context.

[pohly]

maybe we can try adding defer cancel() after each context.

Yes, that'll be in a follow-up.

[liggitt]

https://storage.googleapis.com/k8s-triage/index.html?pr=1&text=failed%20to%20create%20fsnotify%20watcher%3A%20too%20many%20open%20files

pre-existing e2e flake

/retest

[liggitt]

hmmm... same failure

Kubernetes e2e suite: [It] [sig-cli] Kubectl client Simple pod should contain last line of the log

{ failed [FAILED] Expected
<string>: failed to create fsnotify watcher: too many open files
to contain substring
<string>: EOF

[rajibmitra]

hmmm... same failure

Kubernetes e2e suite: [It] [sig-cli] Kubectl client Simple pod should contain last line of the log
{ failed [FAILED] Expected
<string>: failed to create fsnotify watcher: too many open files
to contain substring
<string>: EOF

@liggitt is there any way we could increase the ulimit ?

[liggitt]

@liggitt is there any way we could increase the ulimit ?

I'm more interested in knowing why go 1.24 exacerbates the failure than working around it

[rajibmitra]

@liggitt is there any way we could increase the ulimit ?

I'm more interested in knowing why go 1.24 exacerbates the failure than working around it

Imho, once we have this in place #129688 (comment) , go 1.24 should stop complaining, seems like latest one is stricter.

[liggitt]

Imho, once we have this in place #129688 (comment) , go 1.24 should stop complaining, seems like latest one is stricter.

hrmm, but were any of the flagged locations relevant to the e2e test that's failing? I thought those were mostly in unit test locations we didn't care about

[liggitt]

/hold
/retest

(seeing how many identical failures we get, not 100% sure we're ok with merging with this level of failure yet)

[rajibmitra]

Imho, once we have this in place #129688 (comment) , go 1.24 should stop complaining, seems like latest one is stricter.

hrmm, but were any of the flagged locations relevant to the e2e test that's failing? I thought those were mostly in unit test locations we didn't care about

not sure of the locations, but i can quickly raise a PR that fixes the context test cases against master branch.

[liggitt]

yeah, the failing test is regularly failing in CI periodics already - https://storage.googleapis.com/k8s-triage/index.html?text=failed%20to%20create%20fsnotify%20watcher%3A%20too%20many%20open%20files

we need an open issue for the test / component owner to resolve that pretty quickly

/hold cancel

[k8s-ci-robot]

@cpanato: 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-gce	650db7f	link	unknown	/test pull-kubernetes-e2e-gce

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

[liggitt]

/retest

[cpanato]

nice!