tests: ensure that pre-submits get additional reviews #33958

pohly · 2024-12-16T10:45:47Z

Blocking pre-submit jobs must be for stable, important features. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196.

To ensure that this is considered when defining pre-submit jobs, they need to be listed in config/tests/jobs/policy/presubmit-jobs.yaml. make update-config-fixtures re-generates that file to the expected state for inclusion in a PR.

config/tests/jobs/presubmit-jobs.yaml

aojea · 2024-12-16T10:59:03Z

/assign @aojea @BenTheElder

seems an interesting approach that may solve the existing problem, @BenTheElder is more familiar with all the edges here, let's hear his thoughts

pohly · 2024-12-16T11:39:29Z

https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/test-infra/33958/pull-test-infra-unit-test/1868608463559462912

The test worked, my branch was out-dated and thus I didn't include an up-to-date generated file:

$ git diff
diff --git a/config/tests/jobs/presubmit-jobs.yaml b/config/tests/jobs/presubmit-jobs.yaml
index b7415779ba..197152ed12 100644
--- a/config/tests/jobs/presubmit-jobs.yaml
+++ b/config/tests/jobs/presubmit-jobs.yaml
@@ -25,7 +25,7 @@ run_if_changed:
       run_if_changed: ^(go.mod|go.sum|vendor)
     - name: pull-kubernetes-apidiff-client-go
       optional: true
-      run_if_changed: (^staging\/src\/k8s.io\/client-go)|(^staging\/src\/k8s.io\/code-generator\/examples)
+      run_if_changed: ((^staging\/src\/k8s.io\/client-go)|(^staging\/src\/k8s.io\/code-generator\/examples))/.*\.go
     - name: pull-kubernetes-conformance-image-test
       optional: true
       run_if_changed: conformance

k8s-ci-robot · 2024-12-16T11:44:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please ask for approval from aojea. 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

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

pohly · 2024-12-16T11:49:15Z

config/tests/jobs/jobs_test.go

 	coreapi "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	yaml "sigs.k8s.io/yaml/goyaml.v3"


This is "our" fork of gopkg.in/yaml.v3. We should use the fork in the entire repo and drop the dependency on the original one, but that's a topic for another PR.

filed #33971

pohly · 2024-12-16T12:06:14Z

config/tests/jobs/presubmit-jobs.yaml

+      run_if_changed: /staging/src/k8s.io/component-base/logs/
+    - name: pull-kubernetes-kind-text-logging
+      optional: true
+      run_if_changed: /staging/src/k8s.io/component-base/logs/


These two may qualify for merge-blocking now. I'll check...

Having them show up in this list is a useful reminder 😅

Apropos "merge-blocking": as discussed on Slack, we don't want jobs to run only occasionally and be merge-blocking. We currently have one such job. I demoted it to optional: true, changed the test so that it errors when it encounters such a job (that's how I found it), and removed the optional from the file because all run_if_changed jobs now will be optional.

aojea · 2024-12-16T12:47:13Z

LGTM

tagging @michelle192837 too

aojea · 2024-12-16T12:47:50Z

config/tests/jobs/jobs_test.go

+	// If a job has the same settings regardless of the branch, then
+	// we can reduce to a single entry without the branch info.
+	shorterJobs := make([]presubmitJob, 0, len(*jobs))


any possible side effect of this simplification?

The job still shows up as intended in config/tests/jobs/presubmit-jobs.yaml with all of it's settings, we just don't show for which branches it gets defined. For that a reviewer would have to look at the other YAMLs. Forking a job for a release does not change config/tests/jobs/presubmit-jobs.yaml which IMHO is desirable.

However, it opens a small loophole: a job could get defined and accepted for master, then later someone adds variants for release branches without that triggering a detailed review because config/tests/jobs/presubmit-jobs.yaml doesn't get touched. I think this is acceptable.

with all of it's settings, we just don't show for which branches it gets defined. For that a reviewer would have to look at the other YAMLs.

is it possible to add a comment that shows the versions

# also available in v1.31, v1.32

or is that not worth it?

Reducing Branches and SkipBranches (a regular expression!) to simple version numbers would be a bit challenging, albeit solvable.

But the bigger question is: do we want this file to change when adding the same job to a release branch? It closes a loophole, but also adds churn during each release.

@BenTheElder: thoughts on this?

aojea · 2024-12-16T13:44:18Z

config/tests/jobs/presubmit-jobs.yaml

+    - name: pull-kubernetes-conformance-image-test
+      optional: true
+      run_if_changed: conformance
+    - name: pull-kubernetes-conformance-kind-ipv6-parallel


this can be removed

done in #33965

aojea · 2024-12-16T13:45:48Z

config/tests/jobs/presubmit-jobs.yaml

+    - name: pull-kubernetes-e2e-capz-azure-disk
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-disk-vmss
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-disk-windows
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-file
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-file-vmss
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-azure-file-windows
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-conformance
+      optional: true
+      run_if_changed: azure.*\.go
+    - name: pull-kubernetes-e2e-capz-windows-1-29


all of this seems that must be removed

ubernetes$ find . | grep -E "azure.*go" ./test/e2e/framework/providers/azure/azure.go ./test/e2e/framework/providers/azure/doc.go ./staging/src/k8s.io/csi-translation-lib/plugins/azure_file.go ./staging/src/k8s.io/csi-translation-lib/plugins/azure_disk_test.go ./staging/src/k8s.io/csi-translation-lib/plugins/azure_disk.go ./staging/src/k8s.io/csi-translation-lib/plugins/azure_file_test.go ./staging/src/k8s.io/client-go/applyconfigurations/core/v1/azurefilepersistentvolumesource.go ./staging/src/k8s.io/client-go/applyconfigurations/core/v1/azurediskvolumesource.go ./staging/src/k8s.io/client-go/applyconfigurations/core/v1/azurefilevolumesource.go ./staging/src/k8s.io/client-go/plugin/pkg/client/auth/azure/azure_stub.go

aojea · 2024-12-16T13:55:19Z

config/tests/jobs/presubmit-jobs.yaml

+    - name: pull-kubernetes-e2e-gci-gce-ipvs
+      optional: true
+      run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/.*/ipvs/)


this is not maintained and I don't have time for it , so we remove it

done in #33965

aojea · 2024-12-16T13:55:34Z

config/tests/jobs/presubmit-jobs.yaml

+    - name: pull-kubernetes-e2e-kind-ipvs
+      optional: true
+      run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/proxy/ipvs/)


ditto https://github.com/kubernetes/test-infra/pull/33958/files#r1886860432

done in #33965

pohly · 2024-12-16T14:06:16Z

I suggest we remove or update jobs in separate PRs. I can then rebase this one and we merge it when we are happy with the current state.

BenTheElder · 2024-12-16T17:26:21Z

config/tests/jobs/README.md

@@ -8,4 +8,7 @@ To run via bazel: `bazel test //config/tests/jobs/...`

 To run via go: `go test .`

+If tests fail, re-run with the `UPDATE_FIXTURE_DATA=true` env variable
+and include the modified files in the PR which updates the jobs.


we should probably have a convenient make target since people will be doing this often, normally we have verify scripts spit out the command people need to run when there's generated data to update

I added make update-config-fixtures plus some other make targets like update-unit for symmetry with make unit which happens to run the Go test. The Go test itself now points to make update-config-fixtures because that is faster when the goal is to update the file.

BenTheElder · 2024-12-16T17:27:01Z

config/tests/jobs/presubmit-jobs.yaml

@@ -0,0 +1,158 @@
+# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT!


we should move this into a dedicated sub-folder so we can set a specific list of OWNERS for kubernetes presubmits

Moved to config/tests/jobs/policy together with the test, a README.md and the OWNERS file. I copied that from ./config/testgrids/kubernetes/presubmits/OWNERS with some changes:

removed emeritus

added no_parent_owners: true

added sig/infra as label

That leaves @BenTheElder, @aojea and @test-infra-oncall as reviewer/approver. I probably should remove the latter?

See https://github.com/kubernetes/test-infra/compare/42d46db8df3ecb356f984228408850235f27f446..aa8ef289dd9e091456be488d1d4ee7f4927ee846 for these changes before rebasing.

pohly · 2024-12-17T17:03:33Z

Interesting...

$ make go-unit
hack/make-rules/go-test/unit.sh
+ umask 0022
+ mkdir -p /work/gopath/src/k8s.io/test-infra/_output
+ /work/gopath/src/k8s.io/test-infra/_bin/gotestsum --junitfile=/work/gopath/src/k8s.io/test-infra/_output/junit-unit.xml -- ./...
✓  config/prow/cluster/build/boskos-resources (cached)
✓  config/tests/jobs/policy (cached)
✓  config/tests/lint (cached)
✓  config/tests/mergelists (cached)
✓  config/tests/jobs (cached)
∅  def/configmap
...
DONE 6812 tests in 1.006s

$ echo $?
0

No failure!

But when I run go test manually, there is one:

$ go test ./...
...
--- FAIL: TestPresubmitsKubernetesDashboards (0.00s)
    config_test.go:456: pull-kubernetes-e2e-gce-network-proxy-http-connect: should not be in presubmits-kubernetes-blocking because not actually merge-blocking for kubernetes/kubernetes
FAIL
FAIL	k8s.io/test-infra/config/tests/testgrids	1.280s
...
ok  	k8s.io/test-infra/testgrid/pkg/configurator/prow	(cached)
FAIL

Not good!

pohly · 2024-12-17T17:10:19Z

No failure!

My _bin/gotestsum might have been too old. I can't reproduce after rebuilding it.

As bentheelder said on Slack (https://kubernetes.slack.com/archives/C2C40FMNF/p1734418617113169?thread_ts=1734417601.687079&cid=C2C40FMNF), "adding blocking tests that only sometimes run is confusing and doesn't work well". That's because a regression might get merged without running the job. Then any PR where the job gets run is blocked, whether it is related to the failure or not. Therefore pull-kubernetes-e2e-gce-network-proxy-http-connect should not be blocking.

Blocking pre-submit jobs must be for stable, important features and must always run. Non-blocking pre-submit jobs should only be run automatically if they meet the criteria outlined in kubernetes/community#8196. To ensure that this is considered when defining pre-submit jobs, they need to be listed in `config/tests/jobs/policy/presubmit-jobs.yaml`. The OWNERS file in that new directory ensures that relevant reviewers need to approve. `make update-config-fixture` re-generates that file to the expected state for inclusion in a PR.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/config Issues or PRs related to code in /config sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 16, 2024

k8s-ci-robot requested review from aojea and dims December 16, 2024 10:45

aojea reviewed Dec 16, 2024

View reviewed changes

config/tests/jobs/presubmit-jobs.yaml Outdated Show resolved Hide resolved

k8s-ci-robot assigned aojea and BenTheElder Dec 16, 2024

pohly force-pushed the pre-submit-checks branch from 4f6979f to 42d46db Compare December 16, 2024 11:44

pohly commented Dec 16, 2024

View reviewed changes

aojea reviewed Dec 16, 2024

View reviewed changes

aojea mentioned this pull request Dec 16, 2024

Add presubmits for kubedns/nodelocaldns bumps #33962

Merged

aojea reviewed Dec 16, 2024

View reviewed changes

aojea mentioned this pull request Dec 16, 2024

don't run automatically jobs that are not supervised #33965

Merged

BenTheElder reviewed Dec 16, 2024

View reviewed changes

pohly force-pushed the pre-submit-checks branch from 42d46db to aa8ef28 Compare December 17, 2024 16:03

k8s-ci-robot added area/jobs sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Dec 17, 2024

pohly force-pushed the pre-submit-checks branch from aa8ef28 to 18862e5 Compare December 17, 2024 16:14

pohly force-pushed the pre-submit-checks branch from 18862e5 to 7bfe705 Compare December 17, 2024 16:15

pohly mentioned this pull request Dec 17, 2024

testing strategy: add policy for presubmits kubernetes/community#8196

Open

pohly force-pushed the pre-submit-checks branch from 7bfe705 to bf72f1a Compare December 17, 2024 17:13

k8s-ci-robot added the area/testgrid label Dec 17, 2024

pohly force-pushed the pre-submit-checks branch from bf72f1a to aa82f16 Compare December 17, 2024 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: ensure that pre-submits get additional reviews #33958

tests: ensure that pre-submits get additional reviews #33958

pohly commented Dec 16, 2024 •

edited

Loading

aojea commented Dec 16, 2024

pohly commented Dec 16, 2024

k8s-ci-robot commented Dec 16, 2024

pohly Dec 16, 2024

aojea Dec 16, 2024

BenTheElder Dec 16, 2024

BenTheElder Dec 16, 2024

pohly Dec 16, 2024

pohly Dec 17, 2024

aojea commented Dec 16, 2024

aojea Dec 16, 2024

pohly Dec 16, 2024

aojea Dec 16, 2024

pohly Dec 16, 2024

pohly Dec 17, 2024

aojea Dec 16, 2024

aojea Dec 16, 2024

aojea Dec 16, 2024

aojea Dec 16, 2024

aojea Dec 16, 2024

aojea Dec 16, 2024

aojea Dec 16, 2024

pohly commented Dec 16, 2024

BenTheElder Dec 16, 2024

pohly Dec 17, 2024

BenTheElder Dec 16, 2024

pohly Dec 17, 2024

pohly Dec 17, 2024

pohly commented Dec 17, 2024

pohly commented Dec 17, 2024

		@@ -0,0 +1,158 @@
		# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT!

tests: ensure that pre-submits get additional reviews #33958

Are you sure you want to change the base?

tests: ensure that pre-submits get additional reviews #33958

Conversation

pohly commented Dec 16, 2024 • edited Loading

aojea commented Dec 16, 2024

pohly commented Dec 16, 2024

k8s-ci-robot commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Dec 17, 2024

pohly commented Dec 17, 2024

pohly commented Dec 16, 2024 •

edited

Loading