diff --git a/Makefile b/Makefile index 7e9f2f668c91..826404e54a86 100644 --- a/Makefile +++ b/Makefile @@ -59,6 +59,13 @@ update-go-deps: .PHONY: verify-go-deps verify-go-deps: hack/make-rules/verify/go-deps.sh +.PHONY: update-unit +.PHONY: update-go-unit +update-unit: update-go-unit + UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh +.PHONY: update-config-fixture +update-config-fixture: + UPDATE_FIXTURE_DATA=true hack/make-rules/go-test/unit.sh ./config/tests/... # ======================== Image Building/Publishing =========================== # Build and publish miscellaneous images that get pushed to the specified REGISTRY. # The full set of images covered by these targets is configured in diff --git a/config/tests/jobs/README.md b/config/tests/jobs/README.md index 3cd192905cde..f9e36fd78305 100644 --- a/config/tests/jobs/README.md +++ b/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. + [prow.k8s.io]: https://prow.k8s.io diff --git a/config/tests/jobs/policy/OWNERS b/config/tests/jobs/policy/OWNERS new file mode 100644 index 000000000000..3c6a544f25e8 --- /dev/null +++ b/config/tests/jobs/policy/OWNERS @@ -0,0 +1,17 @@ +# See the OWNERS docs at https://go.k8s.io/owners + +options: + no_parent_owners: true + +reviewers: +- test-infra-oncall # oncall +- BenTheElder # lead +- aojea # lead +approvers: +- test-infra-oncall # oncall +- BenTheElder # lead +- aojea # lead + +labels: +- sig/testing +- sig/infra diff --git a/config/tests/jobs/policy/README.md b/config/tests/jobs/policy/README.md new file mode 100644 index 000000000000..b791ffb3e6da --- /dev/null +++ b/config/tests/jobs/policy/README.md @@ -0,0 +1,13 @@ +The project has certain guidelines around jobs which are meant to ensure that +there's a balance between test coverage and costs for running the CI. For +example, non-blocking jobs that get trigger automatically for PRs should be +used judiciously. + +Because SIG leads are not necessarily familiar with those policies, SIG Testing +and SIG Infra need to be involved before merging jobs that fall into those +sensitive areas. This is achieved with tests and additional files in this +directory and a separate OWNERS file. + +To check whether jobs are okay, run the Go tests in this directory. +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. diff --git a/config/tests/jobs/policy/policy_test.go b/config/tests/jobs/policy/policy_test.go new file mode 100644 index 000000000000..5b63505b9211 --- /dev/null +++ b/config/tests/jobs/policy/policy_test.go @@ -0,0 +1,257 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package policy + +// This file validates Kubernetes's jobs configs against policies. + +import ( + "bytes" + "flag" + "fmt" + "os" + "path/filepath" + "slices" + "sort" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + yaml "sigs.k8s.io/yaml/goyaml.v3" + + cfg "sigs.k8s.io/prow/pkg/config" +) + +var configPath = flag.String("config", "../../../../config/prow/config.yaml", "Path to prow config") +var jobConfigPath = flag.String("job-config", "../../../jobs", "Path to prow job config") +var deckPath = flag.String("deck-path", "https://prow.k8s.io", "Path to deck") +var bucket = flag.String("bucket", "kubernetes-ci-logs", "Gcs bucket for log upload") +var k8sProw = flag.Bool("k8s-prow", true, "If the config is for k8s prow cluster") + +// Loaded at TestMain. +var c *cfg.Config + +func TestMain(m *testing.M) { + flag.Parse() + if *configPath == "" { + fmt.Println("--config must set") + os.Exit(1) + } + + conf, err := cfg.Load(*configPath, *jobConfigPath, nil, "") + if err != nil { + fmt.Printf("Could not load config: %v", err) + os.Exit(1) + } + c = conf + + os.Exit(m.Run()) +} + +func TestKubernetesPresubmitJobs(t *testing.T) { + jobs := c.AllStaticPresubmits([]string{"kubernetes/kubernetes"}) + var expected presubmitJobs + + for _, job := range jobs { + if !job.AlwaysRun && job.RunIfChanged == "" { + // Manually triggered, no additional review needed. + continue + } + + // Mirror those attributes of the job which must trigger additional reviews + // or are needed to identify the job. + j := presubmitJob{ + Name: job.Name, + SkipBranches: job.SkipBranches, + Branches: job.Branches, + + RunIfChanged: job.RunIfChanged, + SkipIfOnlyChanged: job.SkipIfOnlyChanged, + } + + // This uses separate top-level fields instead of job attributes to + // make it more obvious when run_if_changed is used. + if job.AlwaysRun { + expected.AlwaysRun = append(expected.AlwaysRun, j) + } else { + expected.RunIfChanged = append(expected.RunIfChanged, j) + + if !job.Optional { + // Absolute path is more user-friendly than ../../config/... + t.Errorf("Policy violation: %s in %s should use `optional: true` or `alwaysRun: true`.", job.Name, maybeAbsPath(job.SourcePath)) + } + } + + } + expected.Normalize() + + // Encode the expected content. + var expectedData bytes.Buffer + if _, err := expectedData.Write([]byte(`# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT! + +`)); err != nil { + t.Fatalf("unexpected error writing into buffer: %v", err) + } + + encoder := yaml.NewEncoder(&expectedData) + encoder.SetIndent(4) + if err := encoder.Encode(expected); err != nil { + t.Fatalf("unexpected error encoding %s: %v", presubmitsFile, err) + } + + // Compare. This proceeds on read or decoding errors because + // the file might get re-generated below. + var actual presubmitJobs + actualData, err := os.ReadFile(presubmitsFile) + if err != nil && !os.IsNotExist(err) { + t.Errorf("unexpected error: %v", err) + } + if err := yaml.Unmarshal(actualData, &actual); err != nil { + t.Errorf("unexpected error decoding %s: %v", presubmitsFile, err) + } + + // First check the in-memory structs. The diff is nicer for them (more context). + diff := cmp.Diff(actual, expected) + if diff == "" { + // Next check the encoded data. This should only be different on test updates. + diff = cmp.Diff(string(actualData), expectedData.String(), cmpopts.AcyclicTransformer("SplitLines", func(s string) []string { + return strings.Split(s, "\n") + })) + } + + if diff != "" { + if value, _ := os.LookupEnv("UPDATE_FIXTURE_DATA"); value == "true" { + if err := os.WriteFile(presubmitsFile, expectedData.Bytes(), 0644); err != nil { + t.Fatalf("unexpected error: %v", err) + } + t.Logf(` +%s was out-dated. Updated as requested with the following changes (- actual, + expected): +%s +`, maybeAbsPath(presubmitsFile), diff) + } else { + t.Errorf(` +%s is out-dated. Detected differences (- actual, + expected): +%s + +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 https://github.com/kubernetes/community/pull/8196. + +To ensure that this is considered when defining pre-submit jobs, they +need to be listed in %s. If the pre-submit job is really needed, +re-run the test with UPDATE_FIXTURE_DATA=true and include the modified +file. The following command can be used: + + make update-config-fixture +`, presubmitsFile, diff, presubmitsFile) + } + } +} + +// presubmitsFile contains the following struct. +const presubmitsFile = "presubmit-jobs.yaml" + +type presubmitJobs struct { + AlwaysRun []presubmitJob `yaml:"always_run"` + RunIfChanged []presubmitJob `yaml:"run_if_changed"` +} +type presubmitJob struct { + Name string `yaml:"name"` + SkipBranches []string `yaml:"skip_branches,omitempty"` + Branches []string `yaml:"branches,omitempty"` + RunIfChanged string `yaml:"run_if_changed,omitempty"` + SkipIfOnlyChanged string `yaml:"skip_if_only_changed,omitempty"` +} + +func (p *presubmitJobs) Normalize() { + sortJobs(&p.AlwaysRun) + sortJobs(&p.RunIfChanged) +} + +func sortJobs(jobs *[]presubmitJob) { + for _, job := range *jobs { + sort.Strings(job.SkipBranches) + sort.Strings(job.Branches) + } + sort.Slice(*jobs, func(i, j int) bool { + switch strings.Compare((*jobs)[i].Name, (*jobs)[j].Name) { + case -1: + return true + case 1: + return false + } + switch slices.Compare((*jobs)[i].SkipBranches, (*jobs)[j].SkipBranches) { + case -1: + return true + case 1: + return false + } + switch slices.Compare((*jobs)[i].Branches, (*jobs)[j].Branches) { + case -1: + return true + case 1: + return false + } + return false + }) + + // 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)) + for i := 0; i < len(*jobs); { + job := (*jobs)[i] + job.Branches = nil + job.SkipBranches = nil + + if sameSettings(*jobs, job) { + shorterJobs = append(shorterJobs, job) + // Fast-forward to next job. + for i < len(*jobs) && (*jobs)[i].Name == job.Name { + i++ + } + } else { + // Keep all of the different entries. + for i < len(*jobs) && (*jobs)[i].Name == job.Name { + shorterJobs = append(shorterJobs, (*jobs)[i]) + } + } + } + *jobs = shorterJobs +} + +func sameSettings(jobs []presubmitJob, ref presubmitJob) bool { + for _, job := range jobs { + if job.Name != ref.Name { + continue + } + if job.RunIfChanged != ref.RunIfChanged || + job.SkipIfOnlyChanged != ref.SkipIfOnlyChanged { + return false + } + } + return true +} + +// maybeAbsPath tries to make a path absolute. This is useful because +// relative paths in test output tend to be confusing when the user +// invoked the test outside of the test's directory. +func maybeAbsPath(path string) string { + if path, err := filepath.Abs(path); err == nil { + return path + } + return path +} diff --git a/config/tests/jobs/policy/presubmit-jobs.yaml b/config/tests/jobs/policy/presubmit-jobs.yaml new file mode 100644 index 000000000000..d72c1ed54ece --- /dev/null +++ b/config/tests/jobs/policy/presubmit-jobs.yaml @@ -0,0 +1,106 @@ +# AUTOGENERATED by "UPDATE_FIXTURE_DATA=true go test ./config/tests/jobs". DO NOT EDIT! + +always_run: + - name: pull-kubernetes-conformance-kind-ga-only-parallel + - name: pull-kubernetes-dependencies + - name: pull-kubernetes-e2e-ec2 + - name: pull-kubernetes-e2e-ec2-conformance + - name: pull-kubernetes-e2e-gce + - name: pull-kubernetes-e2e-gce-canary + - name: pull-kubernetes-e2e-kind + - name: pull-kubernetes-e2e-kind-ipv6 + - name: pull-kubernetes-integration + - name: pull-kubernetes-integration-go-compatibility + - name: pull-kubernetes-linter-hints + - name: pull-kubernetes-node-e2e-containerd + - name: pull-kubernetes-typecheck + - name: pull-kubernetes-unit + - name: pull-kubernetes-unit-go-compatibility + - name: pull-kubernetes-verify +run_if_changed: + - name: check-dependency-stats + run_if_changed: ^(go.mod|go.sum|vendor) + - name: pull-kubernetes-apidiff-client-go + run_if_changed: ((^staging\/src\/k8s.io\/client-go)|(^staging\/src\/k8s.io\/code-generator\/examples))/.*\.go + - name: pull-kubernetes-conformance-image-test + run_if_changed: conformance + - name: pull-kubernetes-conformance-kind-ipv6-parallel + run_if_changed: ^test/ + - name: pull-kubernetes-cross + run_if_changed: (^.go-version)|(^build/build-image/)|(^hack/lib/golang.sh)|(^build/common.sh) + - name: pull-kubernetes-e2e-autoscaling-hpa-cm + run_if_changed: ^(pkg\/controller\/podautoscaler\/|test\/e2e\/autoscaling\/custom_metrics_stackdriver_autoscaling.go$) + - name: pull-kubernetes-e2e-autoscaling-hpa-cpu + run_if_changed: ^(pkg\/controller\/podautoscaler\/|test\/e2e\/autoscaling\/horizontal_pod_autoscaling|test\/e2e\/framework\/autoscaling\/) + - name: pull-kubernetes-e2e-capz-azure-disk + run_if_changed: azure.*\.go + - name: pull-kubernetes-e2e-capz-azure-disk-vmss + run_if_changed: azure.*\.go + - name: pull-kubernetes-e2e-capz-azure-disk-windows + run_if_changed: azure.*\.go + - name: pull-kubernetes-e2e-capz-azure-file + run_if_changed: azure.*\.go + - name: pull-kubernetes-e2e-capz-azure-file-vmss + run_if_changed: azure.*\.go + - name: pull-kubernetes-e2e-capz-azure-file-windows + run_if_changed: azure.*\.go + - name: pull-kubernetes-e2e-capz-conformance + run_if_changed: azure.*\.go + - name: pull-kubernetes-e2e-capz-windows-1-29 + run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.* + - name: pull-kubernetes-e2e-capz-windows-1-30 + run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.* + - name: pull-kubernetes-e2e-capz-windows-1-31 + run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.* + - name: pull-kubernetes-e2e-capz-windows-1-32 + run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.* + - name: pull-kubernetes-e2e-capz-windows-master + run_if_changed: azure.*\.go$|.*windows\.go$|test/e2e/windows/.* + - name: pull-kubernetes-e2e-gce-cos-alpha-features + run_if_changed: ^.*feature.*\.go + - name: pull-kubernetes-e2e-gce-csi-serial + run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi) + - name: pull-kubernetes-e2e-gce-kubelet-credential-provider + run_if_changed: ^(pkg\/credentialprovider\/|test\/e2e_node\/plugins\/gcp-credential-provider) + - name: pull-kubernetes-e2e-gce-network-policies + run_if_changed: ^(test/e2e/network/|pkg/apis/networking) + - name: pull-kubernetes-e2e-gce-network-proxy-grpc + run_if_changed: ^(cluster/gce/manifests/konnectivity-server.yaml$|cluster/gce/addons/konnectivity-agent) + - name: pull-kubernetes-e2e-gce-network-proxy-http-connect + run_if_changed: ^(cluster/gce/manifests/konnectivity-server.yaml$|cluster/gce/addons/konnectivity-agent) + - name: pull-kubernetes-e2e-gce-providerless-1-30 + run_if_changed: (provider|cloud-controller-manager|cloud|ipam|azure|legacy-cloud-providers|test\/e2e\/cloud\/gcp|test\/e2e\/framework\/provider|nvidia|accelerator|test\/e2e\/network|test\/e2e\/storage) + - name: pull-kubernetes-e2e-gce-storage-slow + run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi) + - name: pull-kubernetes-e2e-gce-storage-snapshot + run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi) + - name: pull-kubernetes-e2e-gci-gce-autoscaling + run_if_changed: ^(cluster/gce/manifests/cluster-autoscaler.manifest$|cluster/addons/rbac/cluster-autoscaler) + - name: pull-kubernetes-e2e-gci-gce-ingress + run_if_changed: ^(test/e2e/network/|pkg/apis/networking) + - name: pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2 + run_if_changed: test/e2e/node/pod_resize.go|pkg/kubelet/kubelet.go|pkg/kubelet/kubelet_pods.go|pkg/kubelet/kuberuntime/kuberuntime_manager.go + - name: pull-kubernetes-e2e-kind-alpha-beta-features + run_if_changed: ^pkg/features/ + - name: pull-kubernetes-e2e-kind-kms + run_if_changed: staging/src/k8s.io/apiserver/pkg/storage/value/|staging/src/k8s.io/kms/|staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/|test/e2e/testing-manifests/auth/encrypt/ + - name: pull-kubernetes-e2e-kind-nftables + run_if_changed: ^(test/e2e/network/|pkg/apis/networking|pkg/proxy/nftables/) + - name: pull-kubernetes-e2e-storage-kind-disruptive + run_if_changed: ^(pkg\/controller\/volume|pkg\/kubelet\/volumemanager|pkg\/volume|pkg\/util\/mount|test\/e2e\/storage|test\/e2e\/testing-manifests\/storage-csi) + - name: pull-kubernetes-kind-dra + run_if_changed: /(dra|dynamicresources|resourceclaim|deviceclass|resourceslice|resourceclaimtemplate|dynamic-resource-allocation|pkg/apis/resource|api/resource)/.*.go + - name: pull-kubernetes-kind-dra-all + run_if_changed: /(dra|dynamicresources|resourceclaim|deviceclass|resourceslice|resourceclaimtemplate|dynamic-resource-allocation|pkg/apis/resource|api/resource)/.*.go + - name: pull-kubernetes-kind-json-logging + run_if_changed: /staging/src/k8s.io/component-base/logs/ + - name: pull-kubernetes-kind-text-logging + run_if_changed: /staging/src/k8s.io/component-base/logs/ + - name: pull-kubernetes-local-e2e + run_if_changed: local-up-cluster + - name: pull-kubernetes-node-e2e-crio-cgrpv1-dra + run_if_changed: (/dra/|/dynamicresources/|/resourceclaim/|/deviceclass/|/resourceslice/|/resourceclaimtemplate/|/dynamic-resource-allocation/|/pkg/apis/resource/|/api/resource/|/test/e2e_node/dra_).*\.(go|yaml) + - name: pull-kubernetes-node-kubelet-credential-provider + run_if_changed: ^(pkg\/credentialprovider\/|test\/e2e_node\/plugins\/gcp-credential-provider) + - name: pull-publishing-bot-validate + run_if_changed: ^staging/publishing.*$ diff --git a/go.mod b/go.mod index b1fc721b5b88..6e4f5b839b71 100644 --- a/go.mod +++ b/go.mod @@ -87,7 +87,7 @@ require ( sigs.k8s.io/controller-runtime v0.12.3 sigs.k8s.io/controller-tools v0.9.2 sigs.k8s.io/prow v0.0.0-20240419142743-3cb2506c2ff3 - sigs.k8s.io/yaml v1.3.0 + sigs.k8s.io/yaml v1.4.0 ) require ( diff --git a/go.sum b/go.sum index 12931547e24e..2baf43fbb04a 100644 --- a/go.sum +++ b/go.sum @@ -1158,5 +1158,5 @@ sigs.k8s.io/structured-merge-diff/v4 v4.2.3 h1:PRbqxJClWWYMNV1dhaG4NsibJbArud9kF sigs.k8s.io/structured-merge-diff/v4 v4.2.3/go.mod h1:qjx8mGObPmV2aSZepjQjbmb2ihdVs8cGKBraizNC69E= sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o= sigs.k8s.io/yaml v1.2.0/go.mod h1:yfXDCHCao9+ENCvLSE62v9VSji2MKu5jeNfTrofGhJc= -sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo= -sigs.k8s.io/yaml v1.3.0/go.mod h1:GeOyir5tyXNByN85N/dRIT9es5UQNerPYEKK56eTBm8= +sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E= +sigs.k8s.io/yaml v1.4.0/go.mod h1:Ejl7/uTz7PSA4eKMyQCUTnhZYNmLIl+5c2lQPGR2BPY=