Use the same way to create tmp directories at hacks #116044

mhmxs · 2023-02-24T14:42:59Z

What type of PR is this?

This change eliminates different ways to create temp directories in scripts at hack

/kind cleanup
/sig node
/release-note-none

What this PR does / why we need it:

It makes sense to use the same method for temp folder creation.

Which issue(s) this PR fixes:

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.:

k8s-ci-robot · 2023-02-24T14:43:08Z

Hi @mhmxs. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

mhmxs · 2023-02-24T14:43:57Z

/release-note-none

bart0sh · 2023-02-24T22:05:22Z

/triage accepted
/priority important-longterm

bart0sh · 2023-02-24T22:05:32Z

/ok-to-test

mhmxs · 2023-02-26T11:50:18Z

/retest

mhmxs · 2023-02-26T13:34:28Z

/retest

mhmxs · 2023-02-26T14:37:35Z

/retest

mhmxs · 2023-02-26T15:27:31Z

/retest

bart0sh · 2023-04-11T18:35:47Z

/retest

bart0sh · 2023-04-11T18:37:56Z

@mhmxs If you still need this PR then please rebase, if not, please close the PR.

bart0sh · 2023-04-18T07:24:21Z

/retest

bart0sh · 2023-04-18T07:26:12Z

@mhmxs please, squash your commits. This should help to get rid of do-not-merge/contains-merge-commits label and proceed further with this pr.

bart0sh · 2023-04-18T10:14:55Z

/retest

bart0sh · 2023-04-18T11:31:11Z

hack/verify-vendor.sh

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

 # create a nice clean place to put our new vendor tree
 mkdir -p "${KUBE_ROOT}/_tmp"
-_tmpdir="$(mktemp -d "${KUBE_ROOT}/_tmp/kube-vendor.XXXXXX")"
+_tmpdir="$(kube::realpath "$(mktemp -d "${KUBE_ROOT}/_tmp/$(basename "$0").XXXXXX")")"


This is repeated everywhere. Would it make sense to wrap it into a function, e.e. kube::mktemp or something similar?

Make sense, I just followed @thockin 's instructions.

bart0sh · 2023-04-18T11:33:51Z

cluster/gce/upgrade-aliases.sh

@@ -27,7 +27,7 @@ if [[ "${KUBERNETES_PROVIDER:-gce}" != "gce" ]]; then
 fi

 KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
-source "${KUBE_ROOT}/hack/lib/util.sh"
+source "${KUBE_ROOT}/hack/lib/init.sh"


How this kind of changes are related to creating tmp directories?

@bart0sh
Because kube::realpath lives at init.sh and init includes util, so made sense to include init everywhere.

kubernetes/hack/lib/init.sh

Line 211 in ed8b4b7

kube::realpath() {

Makes sense to me. Thank you for the explanations!

See above comment - let's not make changes (especially under cluster) unless we REALLY know why.

bart0sh · 2023-04-18T11:35:01Z

/assign @pacoxu @SergeyKanzhelev

k8s-ci-robot · 2023-04-18T12:16:19Z

@mhmxs: 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-local-e2e	`ed8b4b7`	link	false	`/test pull-kubernetes-local-e2e`

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

pacoxu · 2023-04-19T17:36:08Z

/cc @thockin

mhmxs · 2023-04-19T19:40:30Z

@bart0sh Seems local-e2e job fails all the time. I don't understand how the errors are relevant to my change, so i have to investigate more. I'm a newbie here, so I need some time to understand what is going on.

bart0sh · 2023-04-20T08:22:30Z

@mhmxs

Seems local-e2e job fails all the time.

Did you try to rebase on top of the latest master? It might help if the failure is caused by something else.

thockin

Breaking this up would help reviewability a lot. 1 commit, 1 idea.

thockin · 2023-04-21T16:40:11Z

cluster/common.sh

@@ -24,7 +24,7 @@ KUBE_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")"/.. && pwd)

 DEFAULT_KUBECONFIG="${HOME:-.}/.kube/config"

-source "${KUBE_ROOT}/hack/lib/util.sh"
+source "${KUBE_ROOT}/hack/lib/init.sh"


init.sh has a lot of stuff, including changin global vars - I don't think I would make this sort change unless you know why you need to

thockin · 2023-04-21T16:41:03Z

cluster/gce/upgrade-aliases.sh

@@ -27,7 +27,7 @@ if [[ "${KUBERNETES_PROVIDER:-gce}" != "gce" ]]; then
 fi

 KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
-source "${KUBE_ROOT}/hack/lib/util.sh"
+source "${KUBE_ROOT}/hack/lib/init.sh"


See above comment - let's not make changes (especially under cluster) unless we REALLY know why.

thockin · 2023-04-21T16:43:26Z

cluster/gce/util.sh

-    # If the master image is not set, we use the latest GCI image.
-    # Otherwise, we respect whatever is set by the user.
-    export MASTER_IMAGE=${KUBE_GCE_MASTER_IMAGE:-${GCI_VERSION}}
+  : "${GCI_VERSION?= required}"


Even I don't know this syntax. This seems unrelated to the topic of this PR and should at least be in a different commit if not a whole new PR.

Everything under cluster/ is overly brittle and needs extra attention

thockin · 2023-04-21T16:52:11Z

hack/lib/etcd.sh

-ETCD_VERSION=${ETCD_VERSION:-3.5.7}
-ETCD_HOST=${ETCD_HOST:-127.0.0.1}
-ETCD_PORT=${ETCD_PORT:-2379}
+ETCD_VERSION="${ETCD_VERSION:-3.5.7}"


Can you break quoting changes out to a separate commit

thockin · 2023-04-21T16:53:58Z

hack/local-up-cluster.sh

-TMP_DIR=${TMP_DIR:-$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")}
+LOG_SPEC="${LOG_SPEC:-""}"
+LOG_DIR="${LOG_DIR:-"/tmp"}"
+TMP_DIR="${TMP_DIR:-$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")}"


Quoting changes to a different commit, please?

thockin · 2023-04-21T16:54:35Z

hack/make-rules/clean.sh

@@ -19,7 +19,7 @@ set -o nounset
 set -o pipefail

 KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/../..
-source "${KUBE_ROOT}/hack/lib/util.sh"
+source "${KUBE_ROOT}/hack/lib/init.sh"


As above - I'd like to know why?

thockin · 2023-04-21T17:12:51Z

hack/make-rules/test-e2e-node.sh

@@ -36,7 +36,7 @@ skip=${SKIP-"\[Flaky\]|\[Slow\]|\[Serial\]"}
 # Currently, parallelism only affects when REMOTE=true. For local test,
 # ginkgo default parallelism (cores - 1) is used.
 parallelism=${PARALLELISM:-8}
-artifacts="${ARTIFACTS:-"/tmp/_artifacts/$(date +%y%m%dT%H%M%S)"}"
+artifacts="${ARTIFACTS:-$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")/_artifacts/$(date +%y%m%dT%H%M%S)}"


If we are going to repeat the pattern everywhere, why not capture it as

function kube::util::mktemp-dir() { local script="$0" if [[ -n "$1" ]]; then script="$1" fi base="$(basename "${script}")" mktemp -d -t "${base}.XXXXXX" }

thockin · 2023-04-21T17:17:39Z

It's also uncclear to me why readlink and realpath are in init and not util (and named as such)

k8s-ci-robot · 2023-05-05T06:35:46Z

PR needs rebase.

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/test-infra repository.

bart0sh · 2023-05-11T11:46:39Z

@mhmxs please address remaining comments to go further with this PR, thanks.

mhmxs · 2023-05-11T12:41:46Z

@mhmxs please address remaining comments to go further with this PR, thanks.

@bart0sh I haven't forget, but the suggestion is to cut this PR into 2-3 separated ones, which makes sense, because it helps identify which change broke e2e testing. So I think we should close this one, and let me split it.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 24, 2023

k8s-ci-robot requested review from SataQiu and sttts February 24, 2023 14:43

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 24, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 24, 2023

bart0sh added this to Needs Reviewer in SIG Node PR Triage Feb 24, 2023

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. area/provider/gcp Issues or PRs related to gcp provider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Feb 27, 2023

k8s-ci-robot added do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 18, 2023

Use the same way to create tmp directories at hacks

ed8b4b7

mhmxs force-pushed the mktemp-cleanup branch from f3d9021 to ed8b4b7 Compare April 18, 2023 07:41

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Apr 18, 2023

bart0sh reviewed Apr 18, 2023

View reviewed changes

bart0sh moved this from Waiting on Author to Needs Reviewer in SIG Node PR Triage Apr 18, 2023

k8s-ci-robot assigned pacoxu and SergeyKanzhelev Apr 18, 2023

k8s-ci-robot requested a review from thockin April 19, 2023 17:36

thockin reviewed Apr 21, 2023

View reviewed changes

bart0sh moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Apr 25, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2023

mhmxs closed this May 11, 2023

SIG Node PR Triage automation moved this from Waiting on Author to Done May 11, 2023

Use the same way to create tmp directories at hacks #116044

Use the same way to create tmp directories at hacks #116044

Conversation

mhmxs commented Feb 24, 2023

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.:

k8s-ci-robot commented Feb 24, 2023

mhmxs commented Feb 24, 2023

bart0sh commented Feb 24, 2023

bart0sh commented Feb 24, 2023

mhmxs commented Feb 26, 2023

mhmxs commented Feb 26, 2023

mhmxs commented Feb 26, 2023

mhmxs commented Feb 26, 2023

bart0sh commented Apr 11, 2023

bart0sh commented Apr 11, 2023

bart0sh commented Apr 18, 2023

bart0sh commented Apr 18, 2023

bart0sh commented Apr 18, 2023

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

bart0sh commented Apr 18, 2023

k8s-ci-robot commented Apr 18, 2023

pacoxu commented Apr 19, 2023

mhmxs commented Apr 19, 2023

bart0sh commented Apr 20, 2023

thockin left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin Apr 21, 2023 • edited

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

thockin commented Apr 21, 2023

k8s-ci-robot commented May 5, 2023

bart0sh commented May 11, 2023

mhmxs commented May 11, 2023

thockin left a comment •

edited

thockin Apr 21, 2023 •

edited