break up the test tarball #74065

ixdy · 2019-02-14T08:05:05Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it: implements sig-testing/20190118-breaking-apart-the-kubernetes-test-tarball:

produce kubernetes-test-portable.tar.gz and kubernetes-test-{OS}-{ARCH}.tar.gz
continue to produce kubernetes-test.tar.gz with an embedded DEPRECATION_NOTICE
- this can be disabled in the make-based build system by setting KUBE_BUILD_MONDO_TEST_TARBALL=n
update cluster/get-kube-binaries.sh to prefer downloading the split test tarballs
also clean up some dead code in get-kube-binaries.sh

Does this PR introduce a user-facing change?:

Split up the mondo `kubernetes-test` tarball into `kubernetes-test-portable` and `kubernetes-test-{OS}-{ARCH}` tarballs.

/sig testing release
/priority important-soon

ixdy · 2019-02-14T22:22:44Z

I think this is basically ready for review now. The bazel stuff somewhat conflicts with #73930, so I'll clean up the mess in whichever PR loses the race.

cc @spiffxp @tpepper @kubernetes/bash-firefighters @akutz @amwat

ixdy · 2019-02-14T22:26:29Z

/unassign

akutz · 2019-02-14T22:28:58Z

build/lib/release.sh

@@ -466,39 +471,99 @@ function kube::release::package_kube_manifests_tarball() {
  kube::release::create_tarball "${package_name}" "${release_stage}/.."
 }

+# Builds tarballs for each test platform containing the appropriate binaries.
+function kube::release::package_test_platform_tarballs() {


I'm just marveling at the fact that you can have colons in a function name in BASH. I had no idea.

I've gained so much bash expertise on this project

There's some really amazing bash in this project. And also plenty of horrifying stuff #72956 😛

ixdy · 2019-02-20T17:14:00Z

I'm not sure who to assign here... I feel bad for overburdening the same folks each time.

/assign @BenTheElder @cblecker @fejta @spiffxp

fejta

/lgtm
/hold

fejta · 2019-02-20T20:00:24Z

build/lib/release.sh

@@ -466,39 +471,99 @@ function kube::release::package_kube_manifests_tarball() {
  kube::release::create_tarball "${package_name}" "${release_stage}/.."
 }

+# Builds tarballs for each test platform containing the appropriate binaries.
+function kube::release::package_test_platform_tarballs() {


I've gained so much bash expertise on this project

fejta · 2019-02-20T22:01:46Z

build/lib/release.sh

+    local platform_tag=${platform/\//-} # Replace a "/" for a "-"
+    local release_stage="${RELEASE_STAGE}/test/${platform_tag}/kubernetes"
+    mkdir -p "${release_stage}/test/bin"
+    cp "${KUBE_TEST_SERVER_BINARIES[@]/#/${LOCAL_OUTPUT_BINPATH}/${platform}/}" \


What does this # mean?

Hi @fejta,

I mean, it looks like string manipulation, but dang it's nasty.

From https://www.tldp.org/LDP/abs/html/arrays.html:

string=abcABC123ABCabc echo ${string[@]} # abcABC123ABCabc

And from http://tldp.org/LDP/abs/html/parameter-substitution.html:

${var#Pattern} Remove from $var the shortest part of $Pattern that matches the front end of $var.

I read it as removing the string evaluated from /${LOCAL_OUTPUT_BINPATH}/${platform}/ from all of the elements in ${KUBE_TEST_SERVER_BINARIES}.

I suppose it makes sense if they're trying to use the cp command's ability to copy multiple source files into a single destination directory:

cp [FLAGS] source_file ... target_directory

Maybe trying to remove all but the file name for each element of ${KUBE_TEST_SERVER_BINARIES} in order to copy them into "${release_stage}/test/bin/"?

That's my best guess without spending more time looking at it.

Hi @fejta,

From the same arrays link as above:

# Substring Removal # Removes shortest match from front of string(s). echo ${arrayZ[@]#f*r} # one two three five five # ^ # Applied to all elements of the array. # Matches "four" and removes it.

This seems to be the same pattern being used and that I sussed out above. Still, with so many variables in play, I haven't traced out what the actual result is.

@akutz was on the right path, but this is actually a different operator - /#, not #.

From the arrays link:

arrayZ=( one two three four five five ) # Substring Replacement # Replace front-end occurrences of substring. echo ${arrayZ[@]/#fi/XY} # one two three four XYve XYve # ^ # Applied to all elements of the array.

Because the match string is empty (there's no string after # before the next /), we end up just prepending the following string. (It's extra confusing because we have /s in the "replacement" string.)

It's documented in a few other places it's used in this file, e.g.

kubernetes/build/lib/release.sh

Lines 137 to 141 in 61b676a

# This fancy expression will expand to prepend a path

# (${LOCAL_OUTPUT_BINPATH}/${platform}/) to every item in the

# KUBE_CLIENT_BINARIES array.

cp "${client_bins[@]/#/${LOCAL_OUTPUT_BINPATH}/${platform}/}" \

"${release_stage}/client/bin/"

I can add a similar comment for this usage.

(Have I mentioned that the bash side of this PR was far more difficult than the bazel side?)

Thank you @ixdy. I didn't realize that /# behaved differently. I don't tend to use Bash string manipulation at all, let alone Bash arrays. I usually stick with POSIX-compliant rules and defer to sed for replacements.

yeah, I blame @jbeda :) #1324

definitely a cool trick, though.
#1324 (comment)

ixdy · 2019-02-21T00:59:10Z

/retest

fejta

/lgtm

build/lib/release.sh

spiffxp

/lgtm

spiffxp · 2019-02-21T23:54:46Z

cluster/get-kube-binaries.sh

+    # Loop over only the unique tuples
+    for TUPLE in $(printf "%s\n" "${TEST_PLATFORM_TUPLES[@]}" | sort -u); do
+        OS=$(echo "${TUPLE}" | cut -d/ -f1)
+        ARCH=$(echo "${TUPLE}" | cut -d/ -f2)


who says bash can't do sets of tuples

I could probably use IFS="/" read here instead, but I felt this was more readable.

who says indeed? :P

BenTheElder · 2019-02-22T08:08:35Z

build/lib/release.sh

+  kube::release::create_tarball "${portable_tarball_name}" "${release_stage}/.."
+
+  if [[ "${KUBE_BUILD_MONDO_TEST_TARBALL}" =~ [yY] ]]; then
+    kube::log::status "Building tarball: test mondo (deprecated by KEP sig-testing/20190118-breaking-apart-the-kubernetes-test-tarball)"


thanks, FWIW you can get a sorta short-ish link to the file:
https://git.k8s.io/enhancements/keps/sig-testing/20190118-breaking-apart-the-kubernetes-test-tarball.md

spiffxp · 2019-02-22T20:21:31Z

/approve

k8s-ci-robot · 2019-02-22T20:28:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ixdy, spiffxp

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:

~~build/OWNERS~~ [ixdy]
~~cluster/OWNERS~~ [spiffxp]

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

ixdy · 2019-02-22T21:32:42Z

/hold cancel

k8s-ci-robot assigned ixdy Feb 14, 2019

k8s-ci-robot requested review from jbeda and zmerlynn February 14, 2019 08:05

ixdy force-pushed the break-up-test-tarball branch from 11762f6 to ab915e8 Compare February 14, 2019 20:33

ixdy changed the title ~~WIP: break up the test tarball~~ break up the test tarball Feb 14, 2019

ixdy force-pushed the break-up-test-tarball branch from ab915e8 to 2fe408e Compare February 14, 2019 22:17

k8s-ci-robot unassigned ixdy Feb 14, 2019

akutz reviewed Feb 14, 2019

View reviewed changes

k8s-ci-robot assigned BenTheElder Feb 20, 2019

fejta reviewed Feb 20, 2019

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 20, 2019

fejta reviewed Feb 21, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019

BenTheElder reviewed Feb 21, 2019

View reviewed changes

build/lib/release.sh Outdated Show resolved Hide resolved

ixdy added 5 commits February 21, 2019 13:17

bash build system: create split test tarballs

640a879

bazel: create split test tarballs

3469579

Support split test tarballs in get-kube-binaries.sh

56949c7

cleanup: we always need to download client and server tarballs

b360f95

add comments to explain fancy bash expressions

c5c0e42

ixdy force-pushed the break-up-test-tarball branch from 4a5ea72 to c5c0e42 Compare February 21, 2019 21:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019

spiffxp approved these changes Feb 21, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2019

BenTheElder reviewed Feb 22, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2019

ixdy mentioned this pull request Feb 22, 2019

Break apart the kubernetes test tarball kubernetes/enhancements#714

Closed

k8s-ci-robot merged commit fd7acc3 into kubernetes:master Feb 23, 2019

ixdy mentioned this pull request Feb 23, 2019

bazel: initial support for cross-compilation #73930

Merged

ixdy mentioned this pull request Mar 7, 2019

update split test tarballs KEP: status -> implemented kubernetes/enhancements#880

Merged

mrIncompetent mentioned this pull request Mar 7, 2019

update conformance test image kubermatic/kubermatic#2955

Merged

tpepper mentioned this pull request Sep 17, 2019

Deprecated mondo test tarball needs removed #82767

Closed

ixdy mentioned this pull request Sep 24, 2019

Stop building the kubernetes-test mondo tarball #83093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

break up the test tarball #74065

break up the test tarball #74065

ixdy commented Feb 14, 2019 •

edited

ixdy commented Feb 14, 2019

ixdy commented Feb 14, 2019

akutz Feb 14, 2019

fejta Feb 20, 2019

BenTheElder Feb 21, 2019

ixdy commented Feb 20, 2019

fejta left a comment

fejta Feb 20, 2019

fejta Feb 20, 2019

akutz Feb 20, 2019

akutz Feb 20, 2019

ixdy Feb 20, 2019 •

edited

akutz Feb 20, 2019

ixdy Feb 21, 2019

ixdy Feb 21, 2019

ixdy commented Feb 21, 2019

fejta left a comment

spiffxp left a comment

spiffxp Feb 21, 2019

ixdy Feb 22, 2019

BenTheElder Feb 22, 2019

BenTheElder Feb 22, 2019

spiffxp commented Feb 22, 2019

k8s-ci-robot commented Feb 22, 2019

ixdy commented Feb 22, 2019

	# This fancy expression will expand to prepend a path
	# (${LOCAL_OUTPUT_BINPATH}/${platform}/) to every item in the
	# KUBE_CLIENT_BINARIES array.
	cp "${client_bins[@]/#/${LOCAL_OUTPUT_BINPATH}/${platform}/}" \
	"${release_stage}/client/bin/"

break up the test tarball #74065

break up the test tarball #74065

Conversation

ixdy commented Feb 14, 2019 • edited

ixdy commented Feb 14, 2019

ixdy commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixdy commented Feb 20, 2019

fejta left a 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

ixdy Feb 20, 2019 • 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

ixdy commented Feb 21, 2019

fejta left a comment

Choose a reason for hiding this comment

spiffxp left a 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

spiffxp commented Feb 22, 2019

k8s-ci-robot commented Feb 22, 2019

ixdy commented Feb 22, 2019

ixdy commented Feb 14, 2019 •

edited

ixdy Feb 20, 2019 •

edited