Use shared builds for some e2e jobs #4105

BenTheElder · 2017-08-17T22:15:20Z

This will allow some jobs to reuse the same kubernetes build. In particular our PR e2e jobs each do a build and then kubetest --extract=local ... instead of one shared build.

TODO:

Make run_after_success jobs able to find builds
This is done by logging the path to a text file in GCS similar to pr-logs/directory since we can't just move all the build locations. This file is at gs://some-shared-bucket/$PULL_REFS/(bazel-)?build-location.txt
add test(s) to make sure the config does not contain new invalid flag combinations
make jobs fall back to local builds
Configure PR e2e jobs to use a shared build step

FOLLOW-UP:

verify that this works for the test job, expand to all PR e2e jobs

This is a little ugly but after a lot of discussion on-and-offline (#4047, #4074, ...) it seems to be the best solution for the moment. We may want to revisit the entire run_after_success system at some point in the future.

krzyzacy · 2017-08-17T22:55:27Z

scenarios/kubernetes_build.py

+    gcs_build = "gs://"+bucket+dest+latest
+    # and then write it to GCS
+    gcs_shared = args.gcs_shared + os.getenv('PULL_REFS', '') + '/build-location.txt'
+    upload_string(gcs_shared, gcs_build)


I'd make the extra steps optional, since only pr build job need to upload to shared builds for now.

Maybe consider only do this when PULL_REFS is valid.

krzyzacy · 2017-08-17T22:56:52Z

scenarios/kubernetes_e2e.py

+        if args.use_shared_build:
+            link += args.use_shared_build + '-'
+        link += 'build-location.txt'
+        url = link.replace("gs://", "https://storage.googleapis.com/")


s/"/'/ (and other places), and I think gsutil cat might be better

fixing the strings, I'm not sure if gsutil cat is going to be any better.

krzyzacy · 2017-08-17T22:58:58Z

scenarios/kubernetes_e2e.py

+        url = link.replace("gs://", "https://storage.googleapis.com/")
+        build_loc = urllib2.urlopen(url).read()
+        # tell kubetest to extract from this location
+        args.kubetest_args += "--extract="+build_loc


I think you'll need to add a test to assert other --extract does not exist in config.json, or what's the expect behavior?

BenTheElder · 2017-08-17T23:01:30Z

--extract should be allowed in other jobs (eg --extract=latest), but when using shared builds it needs to be set to the build location dynamically

…

On Thu, Aug 17, 2017 at 3:59 PM, Sen Lu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In scenarios/kubernetes_e2e.py <#4105 (comment)> : > @@ -476,7 +477,17 @@ def main(args): runner_args.append( '--gcp-service-account=%s' % mode.add_service_account(args.service_account)) - if args.build is not None: + if args.use_shared_build is not None: + # find shared build location from GCS + link = args.gcs_shared + os.getenv('PULL_REFS', '') + '/' + if args.use_shared_build: + link += args.use_shared_build + '-' + link += 'build-location.txt' + url = link.replace("gs://", "https://storage.googleapis.com/") + build_loc = urllib2.urlopen(url).read() + # tell kubetest to extract from this location + args.kubetest_args += "--extract="+build_loc I think you'll need to add a test to assert other --extract does not exist in config.json, or what's the expect behavior? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4105 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4Bq2qTJ0bpWUVvlimzQnV2f9jOXEEvks5sZMW0gaJpZM4O64Lr> .

krzyzacy · 2017-08-17T23:04:56Z

We support multiple --extract flags, it will extract multiple times and it means different things in kubetest, mostly for upgrade from one version to another :-)

(It does not simply overwrite, if that's what you are expecting)

BenTheElder · 2017-08-17T23:10:52Z

@krzyzacy Added an entry to this PR's todo list to ensure that the flags are valid in the config, I will do that now.

BenTheElder · 2017-08-18T21:26:11Z

@spxtr @krzyzacy PTAL. If this makes enough sense I can start migrating some job configs to use run_after_success + --use-shared_build.
EDIT: I have a big refactor of the job configs in #4002 that I'd also like to get in first.

krzyzacy · 2017-08-18T21:27:26Z

Please also add some tests to assert the intended behavior.

BenTheElder · 2017-08-21T21:33:49Z

@krzyzacy PTAL, I've updated the tests, fixed a few things, and configured the non-blocking PR GPU job to run after a build step.

krzyzacy · 2017-08-21T21:49:51Z

scenarios/kubernetes_build.py

+        dest += args.suffix
+    build_tar = tarfile.open("_output/release-tars/kubernetes.tar.gz")
+    latest = build_tar.getmember("kubernetes/version").read().strip()
+    gcs_build = 'gs://'+bucket+dest+latest


this does not give you the right path - maybe use os.path.join

and I would suggest you add a unittest for both bazel and build scenario as well

krzyzacy · 2017-08-21T21:59:26Z

scenarios/kubernetes_e2e.py

+        try:
+            build_loc = read_gcs_path(gcs_path)
+            # tell kubetest to extract from this location
+            args.kubetest_args.append("--extract="+build_loc)


nit: single quote, and below

also, extra space: " + build_loc

krzyzacy · 2017-08-21T22:35:51Z

/assign @ixdy

I might have more bikeshed comments but overall the idea should work. I'd also let @ixdy have a chance to go through it overall as well, as he knows more about the build stuff here :-)

ixdy · 2017-08-22T22:47:38Z

scenarios/kubernetes_bazel.py

@@ -37,6 +37,13 @@ def check_output(*cmd):
    print >>sys.stderr, 'Run:', cmd
    return subprocess.check_output(cmd)

+def upload_string(gcs_path, text):
+    """Uploads s to gcs_path"""


ixdy · 2017-08-22T22:51:54Z

scenarios/kubernetes_bazel.py

+                # log push-build location to path child jobs can find
+                # (gs://<shared-bucket>/$PULL_REFS/bazel-build-location.txt)
+                pull_refs = os.getenv('PULL_REFS', '')
+                gcs_shared = args.gcs_shared + pull_refs + '/bazel-build-location.txt'


it'd probably be safer to use os.path.join here so that you don't end up with something weird if --gcs_shared is missing a trailing slash. i.e. change this to

gcs_shared = os.path.join(args.gcs_shared, pull_refs, 'bazel-build-location.txt`)

ixdy · 2017-08-22T22:55:51Z

scenarios/kubernetes_build.py

+            dest += args.suffix
+        build_tar = tarfile.open("_output/release-tars/kubernetes.tar.gz")
+        latest = build_tar.getmember("kubernetes/version").read().strip()
+        gcs_build = os.path.join('gs://', bucket, dest, latest)


duplicating the logic of push-build.sh seems pretty fragile... I'd rather either we remove that logic from push-build.sh or add an option to push-build.sh to create this pointer file. @david-mcmahon

It's definitely fragile, I'm considering if we can just enable this feature for bazel build jobs and skip doing it for non-bazel builds since it's now scoped to just PR jobs anyhow. I'm pretty sure all jobs shouldn't need the special logic for finding builds.

Discussed out-of-band, I think we don't need to support this for non-bazel builds so for now I will remove this.

ixdy · 2017-08-22T23:00:28Z

scenarios/kubernetes_e2e.py

+            args.build = None
+        except urllib2.URLError as err:
+            print >>sys.stderr, 'Failed to get shared build location: %s'%err.reason
+            print >>sys.stderr, 'Falling back to local build...'


why fallback? this makes things complicated and may result in the job timing out instead.

Fallback just seemed like a safer way to make this change. I can remove it.

BenTheElder · 2017-08-23T00:12:43Z

NOTE: this needs to port over #4160 once that is in.
Edit: this is done now.

BenTheElder · 2017-08-28T19:34:28Z

ping @ixdy @krzyzacy, this has reduced scope now and should be more straightforward. PTAL

krzyzacy · 2017-08-28T20:01:52Z

jenkins/bootstrap_test.py

+                extract_in_args = False
+                build_in_args = False
+                for arg in args:
+                    if arg.startswith("--use-shared-build"):


done, also below

krzyzacy · 2017-08-28T20:05:05Z

scenarios/kubernetes_e2e.py

+        gcs_path = os.path.join(args.gcs_shared, os.getenv('PULL_REFS', ''), build_file)
+        try:
+            # tell kubetest to extract from this location
+            args.kubetest_args.append('--extract=' + read_gcs_path(gcs_path))


how about print gcs_path so that we can have more debug clue on failures

krzyzacy · 2017-08-28T20:08:41Z

scenarios/kubernetes_e2e.py

+        '--use-shared-build', nargs='?', default=None, const='',
+        help='Use prebuilt kubernetes binaries if set, optionally specifying strategy')
+    parser.add_argument(
+        '--gcs-shared',


bikeshed: maybe --shared-build-location?

this flag can be used for other shared files and isn't actually the build location but where the "symlink" to the build is.

krzyzacy · 2017-08-28T20:48:21Z

scenarios/kubernetes_e2e.py

+            build_file += args.use_shared_build + '-'
+        build_file += 'build-location.txt'
+        gcs_path = os.path.join(args.gcs_shared, os.getenv('PULL_REFS', ''), build_file)
+        print >>sys.stderr, "Getting shared build location from: "+gcs_path


last nitpick for the quotes!

I seriously need gofmt, but for python.
done.

krzyzacy · 2017-08-28T20:48:51Z

scenarios/kubernetes_e2e.py

+        help='Use prebuilt kubernetes binaries if set, optionally specifying strategy')
+    parser.add_argument(
+        '--gcs-shared',
+        default="gs://kubernetes-jenkins/shared-results/",


here as well 😈

we really need a gofmt, but for python :|

krzyzacy · 2017-08-28T20:51:08Z

scenarios/kubernetes_e2e_test.py

+        # normal path
+        with Stub(kubernetes_e2e, 'check_env', self.fake_check_env):
+            with Stub(kubernetes_e2e, 'read_gcs_path', always_kubernetes):
+                kubernetes_e2e.main(args)


oh, here in the test, I would like to see the gcs path pass into read_gcs_path is correct, both with normal build and bazel build.

krzyzacy · 2017-08-28T20:56:51Z

and please squash :-)

BenTheElder · 2017-08-28T21:53:07Z

Should be green, fixed nits. Will sqaush once everything checks out.

krzyzacy · 2017-08-28T22:01:35Z

/lgtm

weee, let's see how it will work!

k8s-ci-robot · 2017-08-28T22:03:22Z

@BenTheElder: I updated Prow config for you!

In response to this:

This will allow some jobs to reuse the same kubernetes build. In particular our PR e2e jobs each do a build and then kubetest --extract=local ... instead of one shared build.

TODO:

Make run_after_success jobs able to find builds

This is done by logging the path to a text file in GCS similar to pr-logs/directory since we can't just move all the build locations. This file is at gs://some-shared-bucket/$PULL_REFS/(bazel-)?build-location.txt

add test(s) to make sure the config does not contain new invalid flag combinations

make jobs fall back to local builds

Configure PR e2e jobs to use a shared build step

FOLLOW-UP:

verify that this works for the test job, expand to all PR e2e jobs

This is a little ugly but after a lot of discussion on-and-offline (#4047, #4074, ...) it seems to be the best solution for the moment. We may want to revisit the entire run_after_success system at some point in the future.

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.

rohitagarwal003 · 2017-08-28T23:58:10Z

prow/config.yaml

+    run_after_success:
+    - name: pull-kubernetes-e2e-gce-gpu
+      agent: jenkins
+      always_run: true


How does always_run and run_after_success interact?
How does run_after_success and /test all interact?

ping @krzyzacy @BenTheElder

(I think @BenTheElder chatted with @mindprince offline, but worth document the behavior as well)

@spxtr knows this better, this has since been reverted and is only being used on a canary job while we sort some other things out.

ref #4475 (comment)
We should definitely doc this more. I've since been a bit confused about the behavior myself.

see also some discussion in sig-testing slack: https://kubernetes.slack.com/archives/C09QZ4DQB/p1504904920000295

BenTheElder requested review from krzyzacy and spxtr August 17, 2017 22:15

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2017

BenTheElder force-pushed the upload_build_path branch 2 times, most recently from fa9d4cb to 82e0fa6 Compare August 17, 2017 22:53

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 17, 2017

krzyzacy reviewed Aug 17, 2017

View reviewed changes

BenTheElder force-pushed the upload_build_path branch from 82e0fa6 to ef7942d Compare August 17, 2017 23:03

krzyzacy requested a review from ixdy August 17, 2017 23:10

spxtr mentioned this pull request Aug 17, 2017

prow: size plugin #3847

Merged

BenTheElder force-pushed the upload_build_path branch from 921fa6d to e7bd923 Compare August 18, 2017 06:00

BenTheElder force-pushed the upload_build_path branch from e7bd923 to 28a1e6e Compare August 21, 2017 18:42

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 21, 2017

BenTheElder changed the title ~~[WIP - DO NOT MERGE] use shared builds for some e2e jobs~~ Use shared builds for some e2e jobs Aug 21, 2017

krzyzacy reviewed Aug 21, 2017

View reviewed changes

k8s-ci-robot assigned ixdy Aug 21, 2017

ixdy reviewed Aug 22, 2017

View reviewed changes

add options for shared builds to build and e2e scenarios

9fc10f6

BenTheElder added 7 commits August 22, 2017 17:29

add common build job for pr e2es and switch gpu job to use it

9c513f1

fix nits

7340b08

make gpu job use bazel build

0e55964

don't fallback to local build, use os.path.join, fix method docstrings

164b7cf

revert changes to kubernetes_build scenario

a1fd089

refactor kubernetes_e2e_test

f44a65e

don't skip_report in gpu jobs

6c38a39

BenTheElder force-pushed the upload_build_path branch from 642056c to 6c38a39 Compare August 23, 2017 00:30

krzyzacy reviewed Aug 28, 2017

View reviewed changes

nits again, log gcs_path

fe9b021

krzyzacy reviewed Aug 28, 2017

View reviewed changes

fix nits

6ec4fb5

we really need a gofmt, but for python :|

krzyzacy reviewed Aug 28, 2017

View reviewed changes

BenTheElder added 3 commits August 28, 2017 14:21

check expected path

5c08a96

test shared builds with and without bazel

e16f56e

don't redefine method in test

ad22ac6

k8s-ci-robot assigned krzyzacy Aug 28, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2017

BenTheElder merged commit b75c234 into kubernetes:master Aug 28, 2017

rohitagarwal003 reviewed Aug 29, 2017

View reviewed changes

spiffxp mentioned this pull request Sep 21, 2017

Decouple --build and --extract #3357

Closed

BenTheElder deleted the upload_build_path branch October 6, 2017 01:51

Use shared builds for some e2e jobs #4105

Use shared builds for some e2e jobs #4105

Conversation

BenTheElder commented Aug 17, 2017 • edited Loading

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

BenTheElder commented Aug 17, 2017 via email

krzyzacy commented Aug 17, 2017 • edited Loading

BenTheElder commented Aug 17, 2017

BenTheElder commented Aug 18, 2017 • edited Loading

krzyzacy commented Aug 18, 2017

BenTheElder commented Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

krzyzacy Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy commented Aug 21, 2017

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

Choose a reason for hiding this comment

BenTheElder commented Aug 23, 2017 • edited Loading

BenTheElder commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy Aug 28, 2017 • edited Loading

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

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

krzyzacy commented Aug 28, 2017

BenTheElder commented Aug 28, 2017

krzyzacy commented Aug 28, 2017

k8s-ci-robot commented Aug 28, 2017

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

BenTheElder commented Aug 17, 2017 •

edited

Loading

krzyzacy commented Aug 17, 2017 •

edited

Loading

BenTheElder commented Aug 18, 2017 •

edited

Loading

BenTheElder commented Aug 21, 2017 •

edited

Loading

krzyzacy Aug 21, 2017 •

edited

Loading

BenTheElder commented Aug 23, 2017 •

edited

Loading

krzyzacy Aug 28, 2017 •

edited

Loading