Update to controller-runtime 0.19.1 / Kube 1.31

xrstf commented 1 month ago

This PR brings Prow up-to-speed with the latest Kubernetes and controller-runtime dependencies, plus a few more changes to make these new dependencies work.

controller-tools 0.16.4

Without this update, codegen would fail:

k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".PodSpec
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".Affinity
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".Toleration
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".PodSpec
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".ResourceRequirements
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".Affinity
k8s.io/api/core/v1:-: unknown type "k8s.io/api/core/v1".Toleration
sigs.k8s.io/prow/pkg/apis/prowjobs/v1:-: unable to locate schema for type "k8s.io/api/core/v1".PodSpec
sigs.k8s.io/prow/pkg/apis/prowjobs/v1:-: unable to locate schema for type "k8s.io/api/core/v1".ResourceRequirements
sigs.k8s.io/prow/pkg/apis/prowjobs/v1:-: unable to locate schema for type "k8s.io/api/core/v1".ResourceRequirements
sigs.k8s.io/prow/pkg/apis/prowjobs/v1:-: unable to locate schema for type "k8s.io/api/core/v1".ResourceRequirements
sigs.k8s.io/prow/pkg/apis/prowjobs/v1:-: unable to locate schema for type "k8s.io/api/core/v1".ResourceRequirements
sigs.k8s.io/prow/pkg/apis/prowjobs/v1:-: unable to locate schema for type "k8s.io/api/core/v1".Affinity
sigs.k8s.io/prow/pkg/apis/prowjobs/v1:-: unable to locate schema for type "k8s.io/api/core/v1".Toleration
Error: not all generators ran successfully
run `controller-gen crd:crdVersions=v1 paths=./pkg/apis/prowjobs/v1 output:stdout -w` to see all available markers, or `controller-gen crd:crdVersions=v1 paths=./pkg/apis/prowjobs/v1 output:stdout -h` for usage

golangci-lint 1.58.0

After updating code-generator, staticcheck suddenly threw false positives like:

Go version: go version go1.22.3 linux/amd64
Golangci-lint version: golangci-lint has version v1.57.2 built with go1.22.3 from (unknown, modified: ?, mod sum: "h1:NNhxfZyL5He1WWDrIvl1a4n5bvWZBcgAqBwlJAAgLTw=") on (unknown)
pkg/pluginhelp/hook/hook_test.go:146:44: SA5011: possible nil pointer dereference (staticcheck)
        if got, expected := sets.New[string](help.AllRepos...), sets.New[string](expectedAllRepos...); !got.Equal(expected) {
                                                  ^
pkg/pluginhelp/hook/hook_test.go:143:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
        if help == nil {

However looking at the code, the help == nil check is leading to a t.Fatal, which should be recognized by staticcheck. I have no idea why this suddenly happened, but updating to the next highest golangci-lint version fixes the issue.

Flakiness due to rate limitting

I noticed some tests flaking a lot and started digging. It turns out the issue wasn't actually from loops timing out or contexts getting cancelled, but from the client-side rate limitting that is enabled in the kube clients. I think during integration tests it doesn't make much sense to have rate limitting, as this would mean a lot of code potentially has to handle errors arising from it.

I have therefore disabled the rate limiter by setting cfg.RateLimiter = flowcontrol.NewFakeAlwaysRateLimiter() in the integration test utility code.

Deck re-run tests

These tests have been reworked quite a bit, as they were quite flaky. The issue ultimately boiled down to the old code sorting ProwJobs by ResourceVersion, but during testing I found that it happens quite a lot that ProwJobs are created/updated nearly simultaneously. This has been resolved by sorting the ProwJobs by CreationTimestamp instead, which is unaffected by update calls.

However that is nearly the smallest change in the refactoring.

Instead of deleting Pods, the code now rotates the underlying Deploymens and waits for all Pods to be rotated as well. This is IMO the superior, cleaner solution and less prone to race conditions.
This rotation is now performed every time the job config is updated, to consistently guarantee Pods have gotten the updates. It is unclear to me why the tests for the first job-config update did rotate the Pods, but then later on had a 3*30s waiting loop to just wait for the config updates to be propagated. Now it's IMO clearer: When the config is updated, control plane is rotated and we wait until that's done before proceeding.
The old tests leaked ProwJobs because Horologium would at the last moment often create one last Job not caught by the cleanup. This has been fixed now, too, by again simply rotating the Horologium deployment.
Re-running a job is now a standalone function, mostly for readability. I also replaced the homegrown retry loop with wait.PollUntilContextTimeout. It's IMO unnecessary to have a back-off mechanism in integration tests like this. It just needlessly slows down the test.

The "rotate Deployment instead of deleting Pods manually"-method has been applied to all other integration tests.

k8s-ci-robot commented 1 month ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

k8s-ci-robot commented 1 month ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: xrstf Once this PR has been reviewed and has the lgtm label, please assign krzyzacy for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubernetes-sigs/prow/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

netlify[bot] commented 1 month ago

Deploy Preview for k8s-prow ready!

Name	Link
Latest commit	68c7dc43d3141ec8b598e5b81cc4a37364f6d92f
Latest deploy log	https://app.netlify.com/sites/k8s-prow/deploys/6730d84b21ecf30007595a84
Deploy Preview	https://deploy-preview-293--k8s-prow.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

xrstf commented 1 month ago

/test all

xrstf commented 1 month ago

/test all

xrstf commented 1 month ago

/test all

xrstf commented 1 week ago

/test all

xrstf commented 1 week ago

/test all

xrstf commented 1 week ago

/test all

petr-muller commented 1 week ago

/cc

k8s-ci-robot commented 1 week ago

PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

kubernetes-sigs / prow