Use proper tmp directory for update-openapi-spec.sh #115106

mhmxs · 2023-01-16T11:03:04Z

This change introduces a separate temp folder for update-openapi-spec.sh to avoid permission issues with other scripts.

What type of PR is this?

/kind bug
/sig node

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #115105

k8s-ci-robot · 2023-01-16T11:03:14Z

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-01-16T11:04:02Z

/release-note-none

k8s-triage-robot · 2023-01-16T13:11:40Z

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

aojea · 2023-01-16T13:57:29Z

/assign @thockin

Tim you are already fixing this area, so you'll be the best person to review this

bart0sh · 2023-01-16T21:20:17Z

hack/update-openapi-spec.sh

@@ -47,20 +47,20 @@ trap cleanup EXIT SIGINT

 kube::golang::setup_env

-TMP_DIR=$(mktemp -d /tmp/update-openapi-spec.XXXX)
+TMP_DIR=${TMP_DIR:-$(mktemp -d 2>/dev/null || mktemp -d -t update-openapi-spec.XXXX)}


@mhmxs Thank you for your PR!

Can you explain why do we need to call mktemp twice? I'd say mktemp -d -t update-openapi-spec.XXXXXXXXXX should be enough.

@bart0sh I was looking for existing solutions (there are a few), and this looks the most proper one: https://github.com/kubernetes/kubernetes/blob/master/hack/lib/etcd.sh#L83 🙏

interesting. It was brought by this commit with the explanation 'mktemp on OSX behaves differently than Linux'

It could be true, but it's still able to create temporary directory on my MacBookPro:

> uname Darwin > mktemp -d -t bla.XXXX /var/folders/ct/1d805dgs08g37h03gx0rclm00000gn/T/bla.XXXX.INI0XPZy > echo $? 0 > file /var/folders/ct/1d805dgs08g37h03gx0rclm00000gn/T/bla.XXXX.INI0XPZy /var/folders/ct/1d805dgs08g37h03gx0rclm00000gn/T/bla.XXXX.INI0XPZy: directory

and a lot of the scripts in hack/ seems to call mktemp only once:

$ git grep 'mktemp -d' ./hack/ hack/lib/etcd.sh: ETCD_DIR=${ETCD_DIR:-$(mktemp -d 2>/dev/null || mktemp -d -t test-etcd.XXXXXX)} hack/lib/etcd.sh: ETCD_SCRAPE_DIR=$(mktemp -d -t test.XXXXXX)/etcd-scrapes hack/lib/util.sh: KUBE_TEMP=$(mktemp -d 2>/dev/null || mktemp -d -t kubernetes.XXXXXX) hack/run-prometheus-on-etcd-scrapes.sh:UNPACKDIR="/tmp/$(cd /tmp && mktemp -d unpack.XXXXXX)" hack/update-openapi-spec.sh:TMP_DIR=$(mktemp -d /tmp/update-openapi-spec.XXXX) hack/update-vendor.sh:TMP_DIR="${TMP_DIR:-$(mktemp -d /tmp/update-vendor.XXXX)}" hack/update-vendor.sh: local_tmp_dir=$(mktemp -d "${TMP_DIR}/pin_replace.XXXX") hack/update-vendor.sh: local_tmp_dir=$(mktemp -d "${TMP_DIR}/group_replace.XXXX") hack/update-vendor.sh: local_tmp_dir=$(mktemp -d "${TMP_DIR}/add_generated_comments.XXXX") hack/verify-e2e-test-ownership.sh:tmpdir=$(mktemp -d -t verify-e2e-test-ownership.XXXX) hack/verify-generated-swagger-docs.sh:_tmpdir="$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")" hack/verify-internal-modules.sh:_tmpdir="$(kube::realpath "$(mktemp -d -t verify-internal-modules.XXXXXX)")" hack/verify-mocks.sh:_tmpdir="$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")" hack/verify-vendor-licenses.sh:_tmpdir="$(mktemp -d "${KUBE_ROOT}/_tmp/kube-licenses.XXXXXX")" hack/verify-vendor.sh:_tmpdir="$(mktemp -d "${KUBE_ROOT}/_tmp/kube-vendor.XXXXXX")" hack/verify-yamlfmt.sh:_tmpdir="$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")"

I'd suggest to investigate this further to fully understand why it's called twice only in 2 scripts.

@smarterclayton can you help us to understand the idea of this commit?

@bart0sh There are lots of hard-coded temp locations too:

$ git grep '/tmp/' ./hack/ hack/cherry_pick_pull.sh: echo "+++ Downloading patch to /tmp/${pull}.patch (in case you need to do this again)" hack/cherry_pick_pull.sh: curl -o "/tmp/${pull}.patch" -sSL "https://github.com/${MAIN_REPO_ORG}/${MAIN_REPO_NAME}/pull/${pull}.patch" hack/cherry_pick_pull.sh: echo " $ git am -3 /tmp/${pull}.patch" hack/cherry_pick_pull.sh: git am -3 "/tmp/${pull}.patch" || { hack/cherry_pick_pull.sh: subject=$(grep -m 1 "^Subject" "/tmp/${pull}.patch" | sed -e 's/Subject: \[PATCH//g' | sed 's/.*] //') hack/cherry_pick_pull.sh: rm -f "/tmp/${pull}.patch" hack/lib/golang.sh: ) &> "/tmp//${platform//\//_}.build" & hack/lib/golang.sh: cat "/tmp//${platform//\//_}.build" hack/lib/init.sh:# T=/tmp/$$.$RANDOM hack/lib/init.sh:# T=/tmp/$$.$RANDOM hack/lib/util.sh: $(kube::util::find-binary kubectl) --kubeconfig="${dest_dir}/${client_id}.kubeconfig" config view --minify --flatten > "/tmp/${client_id}.kubeconfig" hack/lib/util.sh: mv -f "/tmp/${client_id}.kubeconfig" "${dest_dir}/${client_id}.kubeconfig" hack/local-up-cluster.sh: SERVICE_ACCOUNT_KEY=${SERVICE_ACCOUNT_KEY:-/tmp/kube-serviceaccount.key} hack/local-up-cluster.sh: cat <<EOF > /tmp/kube_egress_selector_configuration.yaml hack/local-up-cluster.sh: EGRESS_SELECTOR_CONFIG_FILE="/tmp/kube_egress_selector_configuration.yaml" hack/local-up-cluster.sh: cat <<EOF > /tmp/kube-audit-policy-file hack/local-up-cluster.sh: AUDIT_POLICY_FILE="/tmp/kube-audit-policy-file" hack/local-up-cluster.sh: cat <<EOF > /tmp/kubelet.yaml hack/local-up-cluster.sh: cat <<EOF >> /tmp/kubelet.yaml hack/local-up-cluster.sh: } >>/tmp/kubelet.yaml hack/local-up-cluster.sh: --config=/tmp/kubelet.yaml >"${KUBELET_LOG}" 2>&1 & hack/local-up-cluster.sh: cat <<EOF > /tmp/kube-proxy.yaml hack/local-up-cluster.sh: fi >>/tmp/kube-proxy.yaml hack/local-up-cluster.sh: --config=/tmp/kube-proxy.yaml \ hack/local-up-cluster.sh: cat <<EOF > /tmp/kube-scheduler.yaml hack/local-up-cluster.sh: --config=/tmp/kube-scheduler.yaml \ hack/local-up-cluster.sh: && curl -sSL --retry 5 --output /tmp/cni."${CNI_TARGETARCH}".tgz "${CNI_PLUGINS_URL}" \ hack/local-up-cluster.sh: && echo "${!cni_plugin_sha} /tmp/cni.${CNI_TARGETARCH}.tgz" | tee /tmp/cni.sha256 \ hack/local-up-cluster.sh: && sha256sum --ignore-missing -c /tmp/cni.sha256 \ hack/local-up-cluster.sh: && rm -f /tmp/cni.sha256 \ hack/local-up-cluster.sh: && sudo tar -C /opt/cni/bin -xzvf /tmp/cni."${CNI_TARGETARCH}".tgz \ hack/local-up-cluster.sh: && rm -rf /tmp/cni."${CNI_TARGETARCH}".tgz \ hack/local-up-cluster.sh:export KUBELET_CIDFILE=/tmp/kubelet.cid hack/make-rules/test-cmd.sh:SERVICE_ACCOUNT_KEY=${SERVICE_ACCOUNT_KEY:-/tmp/kube-serviceaccount.key} hack/make-rules/test-cmd.sh: --cert-dir="${TMPDIR:-/tmp/}" \ hack/make-rules/test-e2e-node.sh:artifacts="${ARTIFACTS:-"/tmp/_artifacts/$(date +%y%m%dT%H%M%S)"}" hack/make-rules/test.sh: cover_report_dir="/tmp/k8s_coverage/$(kube::util::sortable_date)" hack/make-rules/verify.sh: local output="${KUBE_JUNIT_REPORT_DIR:-/tmp/junit-results}" hack/run-prometheus-on-etcd-scrapes.sh:CONFIG="/tmp/$(cd /tmp && mktemp config.XXXXXX)" hack/run-prometheus-on-etcd-scrapes.sh:UNPACKDIR="/tmp/$(cd /tmp && mktemp -d unpack.XXXXXX)" hack/serve-prom-scrapes.sh:response_file="/tmp/$(cd /tmp && mktemp response.XXXXXX)" hack/update-openapi-spec.sh:TMP_DIR=$(mktemp -d /tmp/update-openapi-spec.XXXX) hack/update-openapi-spec.sh:API_LOGFILE=${API_LOGFILE:-/tmp/openapi-api-server.log} hack/update-openapi-spec.sh:SERVICE_ACCOUNT_KEY=${SERVICE_ACCOUNT_KEY:-/tmp/kube-serviceaccount.key} hack/update-vendor.sh:TMP_DIR="${TMP_DIR:-$(mktemp -d /tmp/update-vendor.XXXX)}"

you can either add them to this PR or create new one.

Let's make smaller PRs where we can - no need to pile them all into one PR.

The pattern I use in many of the verify-* scripts is:

_tmpdir="$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")"

bart0sh · 2023-01-16T22:35:13Z

/assign

bart0sh · 2023-01-17T07:23:25Z

/triage accepted
/priority important-longterm

bart0sh · 2023-01-17T07:24:45Z

/ok-to-test

mhmxs · 2023-01-18T09:55:36Z

@thockin I have replaced the mktemp command to the one you suggested.

bart0sh · 2023-01-18T11:13:41Z

/lgtm

k8s-ci-robot · 2023-01-18T11:13:47Z

LGTM label has been added.

Git tree hash: cde19f3c6dd9c47727555c47eded216d626c911f

bart0sh · 2023-01-18T11:50:15Z

/label tide/merge-method-squash

thockin · 2023-01-18T22:42:40Z

hack/update-openapi-spec.sh

@@ -47,20 +47,20 @@ trap cleanup EXIT SIGINT

 kube::golang::setup_env

-TMP_DIR=$(mktemp -d /tmp/update-openapi-spec.XXXX)
+TMP_DIR=${TMP_DIR:-$(kube::realpath "$(mktemp -d -t "$(basename "$0").XXXXXX")")}


If you want to score some more cleanup points, all of these should be quoted:

ETCD_HOST="${ETCD_HOST:-127.0.0.1}" ... API_LOGFILE="${API_LOGFILE:-${TMP_DIR}/openapi-api-server.log}"

etc :)

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2023-01-18T22:43:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhmxs, thockin

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~~ [thockin]

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

Use proper tmp directory for update-openapi-spec.sh

ae30618

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 16, 2023

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 Jan 16, 2023

k8s-ci-robot requested review from SataQiu and sttts January 16, 2023 11:04

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 16, 2023

k8s-ci-robot assigned thockin Jan 16, 2023

bart0sh reviewed Jan 16, 2023

View reviewed changes

k8s-ci-robot assigned bart0sh Jan 16, 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 Jan 17, 2023

bart0sh added this to Waiting on Author in SIG Node PR Triage Jan 17, 2023

mhmxs mentioned this pull request Jan 17, 2023

Make tmp dir configurable and dynamic at local-up-cluster.sh #115017

Merged

Use proper mktmp command in update-openapi-specs.sh

3028738

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2023

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

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 18, 2023

thockin reviewed Jan 18, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2023

k8s-ci-robot merged commit 17422ff into kubernetes:master Jan 19, 2023

SIG Node PR Triage automation moved this from Needs Approver to Done Jan 19, 2023

k8s-ci-robot added this to the v1.27 milestone Jan 19, 2023

mhmxs deleted the fix-openapi-spec-gen branch January 19, 2023 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use proper tmp directory for update-openapi-spec.sh #115106

Use proper tmp directory for update-openapi-spec.sh #115106

mhmxs commented Jan 16, 2023

k8s-ci-robot commented Jan 16, 2023

mhmxs commented Jan 16, 2023

k8s-triage-robot commented Jan 16, 2023

aojea commented Jan 16, 2023

bart0sh Jan 16, 2023

mhmxs Jan 16, 2023

bart0sh Jan 16, 2023

bart0sh Jan 16, 2023

mhmxs Jan 17, 2023

bart0sh Jan 17, 2023

thockin Jan 17, 2023

bart0sh commented Jan 16, 2023

bart0sh commented Jan 17, 2023

bart0sh commented Jan 17, 2023

mhmxs commented Jan 18, 2023

bart0sh commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

bart0sh commented Jan 18, 2023

thockin Jan 18, 2023 •

edited

thockin left a comment

k8s-ci-robot commented Jan 18, 2023

Use proper tmp directory for update-openapi-spec.sh #115106

Use proper tmp directory for update-openapi-spec.sh #115106

Conversation

mhmxs commented Jan 16, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

k8s-ci-robot commented Jan 16, 2023

mhmxs commented Jan 16, 2023

k8s-triage-robot commented Jan 16, 2023

aojea commented Jan 16, 2023

bart0sh Jan 16, 2023

Choose a reason for hiding this comment

mhmxs Jan 16, 2023

Choose a reason for hiding this comment

bart0sh Jan 16, 2023

Choose a reason for hiding this comment

bart0sh Jan 16, 2023

Choose a reason for hiding this comment

mhmxs Jan 17, 2023

Choose a reason for hiding this comment

bart0sh Jan 17, 2023

Choose a reason for hiding this comment

thockin Jan 17, 2023

Choose a reason for hiding this comment

bart0sh commented Jan 16, 2023

bart0sh commented Jan 17, 2023

bart0sh commented Jan 17, 2023

mhmxs commented Jan 18, 2023

bart0sh commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

bart0sh commented Jan 18, 2023

thockin Jan 18, 2023 • edited

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 18, 2023

thockin Jan 18, 2023 •

edited