Godep scripts cleanup

[thockin]

Another try at sanitizing godep scripts. Instead of messing with containers, or assuming anything about GOPATHs, let's just work with whatever GOPATH we are given. If you want sane godep/restore behavior you can use ./hack/run-in-gopath.sh ./hack/godep-restore.sh. This will restore into your _output dir. You can update deps yourself, and then run ./hack/run-in-gopath.sh ./hack/godep-save.sh.

xref #44873

This also checks out godep into your working GOPATH. Without this, we have to go get it every time. We should just vendor it.

[k8s-github-robot]

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", "release-note-experimental" or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

[cblecker]

Are you running this locally? Master hasn't moved to go1.9 yet.

[thockin]

blarch, will revert that part :)

[cblecker]

This is the other reason to run godep in containers.. different git versions provided slightly different strings 😞 . Is this your local env, or inside a container?

[thockin]

local. The problem with doing this in containers is that some envs (jenkins) are already containerized, and we end up with a tangle of "is this already in a container? call a different script" sort of hacks. I want to move this forward as "it works locally, where locally can be in-a-container or not" as the general MO. Running this inside a container requires YET ANOTHER script to avoid rsyncing over the state in between steps.

[cblecker]

Is there a reason you're using pwd over KUBE_ROOT?

[thockin]

From an earlier rev. Will fix

[cblecker]

I like this solution :)

[thockin]

there are others that can do the same - gazelle and kazel popped up. Later.

[cblecker]

Cross build is broken: #51815

[cblecker]

KUBE_ROOT here instead of pwd?

[cblecker]

This looks pretty good. Pulling down and testing locally.

[cblecker]

Noticed #51819 while testing

[cblecker]

Okay, kicked the tires on this in a bunch of ways, and it looks pretty solid. Couple things, then lgtm:

[cblecker]

Suggested patch:

diff --git a/hack/godep-save.sh b/hack/godep-save.sh
index 3c04e341ab..400bdd0232 100755
--- a/hack/godep-save.sh
+++ b/hack/godep-save.sh
@@ -22,34 +22,35 @@ KUBE_ROOT=$(dirname "${BASH_SOURCE}")/..
 source "${KUBE_ROOT}/hack/lib/init.sh"
 source "${KUBE_ROOT}/hack/lib/util.sh"
 
-TMPPFX="_godep_tmp."
-
 kube::log::status "Ensuring prereqs"
 kube::util::ensure_single_dir_gopath
 kube::util::ensure_no_staging_repos_in_gopath
-
 kube::util::ensure_godep_version
+kube::util::ensure-temp-dir
+
+GODEP_TMPPFX="${KUBE_TEMP}/_godep_tmp."
+
 
-function cleanup() {
-    if [[ -d "${TMPPFX}vendor" ]]; then
-        kube::log::error "${TMPPFX}vendor exists, restoring it"
+function kube::godep::cleanup() {
+    if [[ -d "${GODEP_TMPPFX}vendor" ]]; then
+        kube::log::error "${GODEP_TMPPFX}vendor exists, restoring it"
         rm -rf vendor
-        mv "${TMPPFX}vendor" vendor
+        mv "${GODEP_TMPPFX}vendor" vendor
     fi
-    if [[ -d "${TMPPFX}Godeps" ]]; then
-        kube::log::error "${TMPPFX}Godeps exists, restoring it"
+    if [[ -d "${GODEP_TMPPFX}Godeps" ]]; then
+        kube::log::error "${GODEP_TMPPFX}Godeps exists, restoring it"
         rm -rf Godeps
-        mv "${TMPPFX}Godeps" Godeps
+        mv "${GODEP_TMPPFX}Godeps" Godeps
     fi
 }
-trap cleanup EXIT
+kube::util::trap_add kube::godep::cleanup EXIT
 
 # Clear old state, but save it in case of error
 if [[ -d vendor ]]; then
-    mv vendor "${TMPPFX}vendor"
+    mv vendor "${GODEP_TMPPFX}vendor"
 fi
 if [[ -d Godeps ]]; then
-    mv Godeps "${TMPPFX}Godeps"
+    mv Godeps "${GODEP_TMPPFX}Godeps"
 fi
 
 # Some things we want in godeps aren't code dependencies, so ./...
@@ -83,4 +84,4 @@ kube::log::status "Updating LICENSES file"
 hack/update-godep-licenses.sh >/dev/null
 
 # Clean up
-rm -rf "${TMPPFX}vendor" "${TMPPFX}Godeps"
+rm -rf "${GODEP_TMPPFX}vendor" "${GODEP_TMPPFX}Godeps"
diff --git a/hack/lib/util.sh b/hack/lib/util.sh
index 2b832335f1..4ddc7bbf99 100755
--- a/hack/lib/util.sh
+++ b/hack/lib/util.sh
@@ -85,7 +85,7 @@ kube::util::trap_add() {
     if [[ -z "${existing_cmd}" ]]; then
       new_cmd="${trap_add_cmd}"
     else
-      new_cmd="${existing_cmd};${trap_add_cmd}"
+      new_cmd="${trap_add_cmd};${existing_cmd}"
     fi
 
     # Assign the test

What this does:

• Uses ensure-temp-dir to create a system temp directory that gets cleaned up, out of tree (so that _godep_tmp dirs don't get left behind)
• Uses a more specific variable name
• Fixes ordering in the trap_add function so that more recently added traps will get completed first (such as restoring the vendor dir before the temp dir is cleaned up)

WDYT?

[thockin]

KUBE_TEMP is not guaranteed to be on the same device, which could turn the mv into a slow cp. We could instead do _output/local/tmp/$RANDOM or something, if you feel strongly.

Will apply parts of your patch and we can discuss the rest. PTAL

[cblecker]

This change requires a hack/update-staging-godeps.sh run on it to pick up in the staging repos too.

[thockin]

Good point, Maybe I should undo this. This is where godep kind of stinks. Which is a better choice?

[cblecker]

There's no effective change between the two commits, so ¯_(ツ)_/¯:
https://github.com/pmezard/go-difflib/compare/f78a839676152fd9f4863704f5d516195c18fc14...d8ed2627bdf02c080bf22230dbb337003b7aba2d?expand=1

[cblecker]

The other option here would be:

mkdir -p "${KUBE_ROOT}/_tmp/"
GODEP_TMPPFX="${KUBE_ROOT}/_tmp/_godep_save__tmp."

This would work too as /_tmp/ is in .gitignore and isn't rsync'd, but would be in tree so it would be on the same device.

[thockin]

Does anything clean up _tmp ? is _output/local/tmp better or worse? Is that rsynced?

[cblecker]

I'm not aware of anything that cleans up _tmp.

I think either of them would work, as the two are treated similarly by gitignore and rsync. The only folder that is touched by rsync is _output/dockerized/bin, and it's copied to _output/bin. _output is also used by the build-image and generating the release files.

_tmp on the other hand is used by doc generation, by jenkins, and by the godep-licences script:

kubernetes/hack/verify-godep-licenses.sh

Lines 31 to 41 in 44c5182

	# create a nice clean place to put our new godeps
	# must be in the user dir (e.g. KUBE_ROOT) in order for the docker volume mount
	# to work with docker-machine on macs
	mkdir -p "${KUBE_ROOT}/_tmp"
	_tmpdir="$(mktemp -d "${KUBE_ROOT}/_tmp/kube-godep-licenses.XXXXXX")"
	#echo "Created workspace: ${_tmpdir}"
	function cleanup {
	#echo "Removing workspace: ${_tmpdir}"
	rm -rf "${_tmpdir}"
	}
	trap cleanup EXIT

based on that, _tmp seems like the right candidate. also looks like some cleanup could be done in that verify script to match this one (but that could also be a separate PR)

[thockin]

pushed now with _tmp. We should probably add that to make clean though.

[cblecker]

Hmm.. this didn't happen for me locally. Going to clear out my gopath and try again:

W0901 20:52:55.095] godep: error restoring dep (golang.org/x/tools/go/vcs): Wanted to restore rev 1f1b3322f67af76803c942fd237291538ec68262, already restored rev 2382e3994d48b1d22acc2c86bcad0a2aff028e32 for another package in the repo
W0901 20:52:55.404] godep: Error restoring some deps. Aborting check.
W0901 20:52:55.411] !!! Error in ./hack/godep-restore.sh:35
W0901 20:52:55.411]   Error in ./hack/godep-restore.sh:35. 'GOPATH="${GOPATH}:${KUBE_ROOT}/staging" godep restore "$@"' exited with status 1

That looks to be from adding godep to Godeps

[thockin]

indeed. Let me pull off the last commit, we can deal with that on its own. It looks like godep is utterly failing to handle this. Surprise.

[thockin]

oh, this was my fault. I manually fixed one rev, but missed the other from the same repo.

[cblecker]

I0901 22:27:53.986] Godeps Verify failed.
I0901 22:27:53.986] If you're seeing this locally, run the below command to fix your directories:
I0901 22:27:53.986] patch -p0 < vendordiff.patch
I0901 22:27:53.987] (The above output can be saved as godepdiff.patch if you're not running this locally)
I0901 22:27:53.987] (The patch file should also be exported as a build artifact if run through CI)
I0901 22:27:53.987] Copying patch to artifacts..
I0901 22:27:53.987] Leaving /tmp/gopath.6afuma for you to examine or copy. Please delete it manually when finished. (rm -rf /tmp/gopath.6afuma)
I0901 22:27:53.987] FAILED   hack/make-rules/../../hack/verify-godeps.sh	1082s

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/51766/pull-kubernetes-verify/49544/artifacts/vendordiff.patch

[thockin]

had to revert one pwd because godep barfs on staging stuff during save (not too surprising - we're doing strange stuff)

[cblecker]

Oh! I think that's because previously we were pushd'ing into KUBE_ROOT before start. We still used KUBE_ROOT in the original save command below.

[thockin]

@cblecker any clues on what was up in your env?

[thockin]

@cblecker Now that 1.8 is locked and loaded, I'd like to get this in and see if it really hurts anyone besides you :)

[cblecker]

@thockin Sorry, day job has been getting in the way. I'll take another stab at it

[thockin]

/retest

bitbucket gave a 404. nice.

[cblecker]

Okay! Thanks for your patience @thockin. Debugged this, and the issue for me is the naming of this variable. Darwin uses TMPDIR as a system ENVVAR. When this got to the updating bazel step, the kube::util::go_install_from_commit function uses the system TMPDIR and everything blows up. If you rename this to something script-specific (e.g. TMPGODEPDIR in my testing) then everything seems to run just fine.

[thockin]

DONE!

[cblecker]

/retest

[cblecker]

/retest

[cblecker]

/lgtm
/approve

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 44873

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.