Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

break up the test tarball #74065

Merged
merged 5 commits into from Feb 23, 2019

Conversation

@ixdy
Copy link
Member

ixdy commented Feb 14, 2019

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

x-ref kubernetes/enhancements#714

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

This comment has been minimized.

Copy link
Member Author

ixdy commented Feb 14, 2019

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

This comment has been minimized.

Copy link
Member Author

ixdy commented Feb 14, 2019

/unassign

@@ -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() {

This comment has been minimized.

@akutz

akutz Feb 14, 2019

Member

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

This comment has been minimized.

@fejta

fejta Feb 20, 2019

Contributor

I've gained so much bash expertise on this project

This comment has been minimized.

@BenTheElder

BenTheElder Feb 21, 2019

Member

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

@ixdy

This comment has been minimized.

Copy link
Member Author

ixdy commented Feb 20, 2019

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

/assign @BenTheElder @cblecker @fejta @spiffxp

@ixdy

This comment has been minimized.

Copy link
Member Author

ixdy commented Feb 20, 2019

also cc the release-engineering team:
@aleksandra-malinowska @calebamiles @mikedanese @tpepper

@fejta
Copy link
Contributor

fejta left a comment

/lgtm
/hold

@@ -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() {

This comment has been minimized.

@fejta

fejta Feb 20, 2019

Contributor

I've gained so much bash expertise on this project

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}/}" \

This comment has been minimized.

@fejta

fejta Feb 20, 2019

Contributor

What does this # mean?

This comment has been minimized.

@akutz

akutz Feb 20, 2019

Member

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.

This comment has been minimized.

@akutz

akutz Feb 20, 2019

Member

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.

This comment has been minimized.

@ixdy

ixdy Feb 20, 2019

Author Member

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

# 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?)

This comment has been minimized.

@akutz

akutz Feb 20, 2019

Member

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.

maxresdefault

This comment has been minimized.

@ixdy

ixdy Feb 21, 2019

Author Member

yeah, I blame @jbeda :) #1324

This comment has been minimized.

@ixdy

ixdy Feb 21, 2019

Author Member

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

@ixdy

This comment has been minimized.

Copy link
Member Author

ixdy commented Feb 21, 2019

/retest

@fejta
Copy link
Contributor

fejta left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 21, 2019

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

@ixdy ixdy force-pushed the ixdy:break-up-test-tarball branch from 4a5ea72 to c5c0e42 Feb 21, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 21, 2019

@spiffxp
Copy link
Member

spiffxp left a comment

/lgtm

# 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)

This comment has been minimized.

@spiffxp

spiffxp Feb 21, 2019

Member

who says bash can't do sets of tuples

This comment has been minimized.

@ixdy

ixdy Feb 22, 2019

Author Member

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

This comment has been minimized.

@BenTheElder

BenTheElder Feb 22, 2019

Member

who says indeed? :P

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 21, 2019

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)"

This comment has been minimized.

@BenTheElder
@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Feb 22, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 22, 2019

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

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

@ixdy

This comment has been minimized.

Copy link
Member Author

ixdy commented Feb 22, 2019

/hold cancel
:shipit:

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

16 checks passed

cla/linuxfoundation ixdy authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.