Cleanup CI integration scripts #130760

BenTheElder · 2025-03-12T21:19:17Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Most of what these scripts do at this point is redundant or bad practice

I've done:

stop setting ARTIFACTS from WORKSPACE, CI handles setting ARTIFACTS and WORKSPACE isn't used anymore (bazel?)
stop cd-ing GOPATH, CI sets the working dir already and local users won't necessarily have GOPATH
stop clobbering PATH with hardcoded assumptions, source install-etcd.sh instead (which updates PATH)
don't redundantly set KUBE_COVER to the default
pass logging env inline so the command can be pasted locally
set -x so the command is visible
add TODO about needing a wrapper script just to call install-etcd

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

builds on top of #130758 because I'd prioritize merging that smaller change first.

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2025-03-12T21:19:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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:

~~hack/OWNERS~~ [BenTheElder]

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

BenTheElder · 2025-03-12T21:19:45Z

hack/jenkins/test-dockerized.sh

-GOTOOLCHAIN="${hack_tools_gotoolchain}" go -C "./hack/tools" install gotest.tools/gotestsum
-
-# Disable coverage report
-export KUBE_COVER="n"


this is the default

BenTheElder · 2025-03-12T21:20:30Z

hack/jenkins/test-cmd-dockerized.sh


-export PATH=${GOPATH}/bin:${PWD}/third_party/etcd:/usr/local/go/bin:${PATH}


we don't need to do this, just source ./hack/install-etcd.sh, kube::etcd::install will update PATH, de-duping the PATH computation

and the gopath bin things?

The CI image already has PATH setup reasonably aside from installing etcd, which is the correct place to handle this. We handle etcd path below.

You can also seem that the CI jobs pass after this change:
https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/130760/pull-kubernetes-integration/1900251659217408000
https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/130760/pull-kubernetes-cmd/1900251658978332672

When Kubernetes installs a tool into GOPATH/bin, it does it inside of a script, and it does it in a temporary GOPATH under _output or similar, while setting PATH inside that script.

the install etcd script is an outlier, it doesn't install from source though, and it's old

BenTheElder · 2025-03-12T21:20:53Z

hack/jenkins/test-cmd-dockerized.sh

-export PATH=${GOPATH}/bin:${PWD}/third_party/etcd:/usr/local/go/bin:${PATH}
-
-# Set artifacts directory
-export ARTIFACTS=${ARTIFACTS:-"${WORKSPACE}/artifacts"}


this is useless, CI always sets ARTIFACTS, and locally you won't have either set. WORKSPACE was a bazel-ism

BenTheElder · 2025-03-12T21:21:13Z

hack/jenkins/test-cmd-dockerized.sh

-# Set artifacts directory
-export ARTIFACTS=${ARTIFACTS:-"${WORKSPACE}/artifacts"}
-
-cd "${GOPATH}/src/k8s.io/kubernetes"


this is useless, CI already sets the working directory to the repo clone

worse, it assumes GOPATH for the checkout

BenTheElder · 2025-03-12T21:22:45Z

hack/jenkins/test-dockerized.sh

-if [ -n "${KUBE_HACK_TOOLS_GOTOOLCHAIN:-}" ]; then
-  hack_tools_gotoolchain="${KUBE_HACK_TOOLS_GOTOOLCHAIN}";
-fi
-GOTOOLCHAIN="${hack_tools_gotoolchain}" go -C "./hack/tools" install gotest.tools/gotestsum


this should be useless, or if not, we should fix that, make test automatically handles gotestsum, I think we still have to add that to these (PR is WIP ....)

We ultimately run gotestsum via make test so we don't need this.

BenTheElder · 2025-03-12T21:23:35Z

hack/jenkins/test-integration-dockerized.sh

-./hack/install-etcd.sh
-
-make test-integration
+make test-integration KUBE_KEEP_VERBOSE_TEST_OUTPUT=y LOG_LEVEL=4


passing env this way makes it a pasteable command to produce the same output locally

BenTheElder · 2025-03-12T21:32:51Z

hack/install-etcd.sh

@@ -27,4 +27,4 @@ source "${KUBE_ROOT}/hack/lib/init.sh"

 FOUND="$(echo "${PATH}" | sed 's/:/\n/g' | grep -q "^${KUBE_ROOT}/third_party/etcd$" || true)"
 kube::etcd::install
-test -n "${FOUND}" || echo "  PATH=\"\$PATH:${KUBE_ROOT}/third_party/etcd\""
+test -n "${FOUND}" || echo "export PATH=\"\${PATH}:${KUBE_ROOT}/third_party/etcd\""


this is more useful when pasted by the user, and it also hints that we've exported it if you've sourced the script.

- stop setting ARTIFACTS from WORKSPACE, CI handles setting ARTIFACTS and WORKSPACE isn't used anymore (bazel?) - stop cd-ing GOPATH, CI sets the working dir already and local users won't necessarily have GOPATH - sop clobbering PATH with hardcoded assumptions, source install-etcd.sh instead (which updates PATH) - don't redundantly set KUBE_COVER to the default - pass logging env inline so the command can be pasted locally - set -x so the command is visible - add TODO about needing a wrapper script just to call install-etcd

this is handled centrally by make test which is called by make test-integration which this script calls only make test's implementation actually calls gotestsum, and it also handles installing if needed

BenTheElder · 2025-03-13T18:27:25Z

/cc @aojea @pohly
Nearly all of these "CI wrapper scripts" for integration / test-cmd is unnecessary and/or counter-productive, this PR eliminates nearly all of it.

They continue to pass, I had to push a spelling fix, and there are some unrelated flakes in other jobs incl #130018 (comment)

BenTheElder · 2025-03-13T19:06:33Z

/restest
flake: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/130760/pull-kubernetes-e2e-kind/1900251659280322560

aojea · 2025-03-13T19:34:09Z

/lgtm

k8s-ci-robot · 2025-03-13T19:34:16Z

LGTM label has been added.

Git tree hash: 2291c313b61371f81ccef96fc23b3c85228148c0

BenTheElder · 2025-03-13T19:49:33Z

/retest
https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/130760/pull-kubernetes-e2e-kind/1900251659280322560

That's twice in a row now with this garbage colletor test, filing a bug

BenTheElder · 2025-03-13T19:51:05Z

No it's not, the retest didn't take, that's the same run. Ok.

Triage shows it has only failed once more in recent history https://storage.googleapis.com/k8s-triage/index.html?pr=1&text=Garbage%20collector%20should%20support%20orphan%20deletion%20of%20custom%20resources

BenTheElder · 2025-03-13T21:24:23Z

/sig testing

BenTheElder · 2025-03-13T21:24:41Z

/triage accepted
/priority backlog

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2025

k8s-ci-robot requested review from dims and SataQiu March 12, 2025 21:19

BenTheElder commented Mar 12, 2025

View reviewed changes

BenTheElder mentioned this pull request Mar 12, 2025

stop overriding max concurrency in CI, let automax procs handle it #130758

Merged

BenTheElder commented Mar 12, 2025

View reviewed changes

BenTheElder added 3 commits March 13, 2025 11:23

install-ectd.sh: brace PATH and suggest user export

8898868

benchmark-dockerized.sh: don't redundantly install gotestsum

1ddfc7b

this is handled centrally by make test which is called by make test-integration which this script calls only make test's implementation actually calls gotestsum, and it also handles installing if needed

BenTheElder force-pushed the cleanup-integration branch from a8b7670 to 1ddfc7b Compare March 13, 2025 18:23

BenTheElder changed the title ~~WIP: cleanup CI integration scripts~~ Cleanup CI integration scripts Mar 13, 2025

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 Mar 13, 2025

k8s-ci-robot requested review from aojea and pohly March 13, 2025 18:27

k8s-ci-robot assigned aojea Mar 13, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 13, 2025

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 13, 2025

k8s-ci-robot merged commit 129661b into kubernetes:master Mar 13, 2025
14 checks passed

k8s-ci-robot added this to the v1.33 milestone Mar 13, 2025

BenTheElder deleted the cleanup-integration branch March 18, 2025 20:13


		export PATH=${GOPATH}/bin:${PWD}/third_party/etcd:/usr/local/go/bin:${PATH}

Cleanup CI integration scripts #130760

Cleanup CI integration scripts #130760

Uh oh!

Conversation

BenTheElder commented Mar 12, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

k8s-ci-robot commented Mar 12, 2025

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!

BenTheElder Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

BenTheElder Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder commented Mar 13, 2025

Uh oh!

BenTheElder commented Mar 13, 2025

Uh oh!

aojea commented Mar 13, 2025

Uh oh!

k8s-ci-robot commented Mar 13, 2025

Uh oh!

BenTheElder commented Mar 13, 2025

Uh oh!

BenTheElder commented Mar 13, 2025

Uh oh!

BenTheElder commented Mar 13, 2025

Uh oh!

BenTheElder commented Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

BenTheElder Mar 13, 2025 •

edited

Loading

BenTheElder Mar 12, 2025 •

edited

Loading