kops: Add a kops-updown job #681

zmerlynn · 2016-09-27T18:24:37Z

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

This change is

fejta · 2016-09-28T01:46:11Z

jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-kops-aws.yaml

+            {provider-env}
+            {job-env}
+            {post-env}
+            if ! curl -fsS --retry 3 -o "$WORKSPACE/kops.url" "https://storage.googleapis.com/kops-ci/bin/latest-ci.txt"; then


This logic seems too complicated to live inside the yaml...

Will you move this into a script and add it to the kubekins-e2e image like we do with e2e-runner.sh, e2e.go, etc?

https://github.com/kubernetes/test-infra/blob/master/jenkins/e2e-image/Dockerfile

The part that's really messy without doing this inline is setting the NODEUP_URL. I need to save both the NODEUP_URL export and the kops binary. I can do it ssh-agent -c style and have the binary spit out the necessary exports, but it's dirty.

We should talk about this. I think it defeats the purpose. To most of us writing these jobs, you've got the yaml, which is super transparent, you've got e2e-runner.sh, which is anything but transparent, and then you hit e2e.go. This is a super minor setup/download, all things considered, but you didn't want it in e2e.go, where it would be obvious and easy to maintain in go. It's not easy to maintain in e2e-runner.sh, which is actually the next most obvious place to put it, because I need to plumb an env variable through. It's not easy to maintain in a separate binary, because I end up writing yaml code that looks like this:

if ! kops-download.sh $WORKSPACE/kops $WORKSPACE/kops.env; then echo "Can't retrieve kops" >&2 exit 1 fi . $WORKSPACE/kops.env

That's about two lines less complicated than the yaml code I wrote already, which is only noisy because of the rigorous error checking that a ton of the other yaml shell code doesn't do. Furthermore, neither case is dockerized, which is kind of bad, but not the worst thing in the world. So, in conclusion, I think this should either stay in the yaml or move down into e2e.go, and not get lost in some inscrutable morass in the middle.

As for what I mean by error checking, this section of bash is easily rewritten as:

export KOPS_URL=$(curl -fsS --retry 3 https://storage.googleapis.com/kops-ci/bin/latest-ci.txt)" curl -fsS --retry 3 -o "${WORKSPACE}/kops" "${KOPS_URL}/linux/amd64/kops" export NODEUP_URL="${KOPS_URL}/linux/amd64/nodeup"

But it's missing the error checking, because bash eats errors on export FOO=$() even on errexit.

The example code you give is still too complex for yaml. Conditional logic is complicated and therefore belongs in its own script, which jenkins job can call unconditionally:

kops-runner.sh

if ! kops-download.sh $WORKSPACE/kops; then exit 1 fi # Whatever complex stuff you want . $WORKSPACE/kops.env timeout {runner}

kops-aws.yaml

export KUBERNETES_PROVIDER="aws" # Pretty soon we will disallow this complexity too bash <(curl -fsS --retry 3 "https://raw.githubusercontent.com/kubernetes/test-infra/master/jenkins/kops-runner.sh") # Pretty soon we will automatically and always checkout test-infra at master

Now it is trivial for me to test out changes: call kops-runner.sh

I did it a little differently than you described and just overrode the e2e-runner.sh in dockerized-e2e-runner.sh, then bundled it in the Docker image. WDYT? (Testing the normal path in kubernetes/kubernetes#33700 right now.)

fejta · 2016-09-28T01:47:20Z

jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-kops-aws.yaml

+    builders:
+        - activate-gce-service-account
+        - shell: |
+            {provider-env}


Any interest in saving us all some pain and place provider-env and job-env here inline?

I'm going to have more jobs, but moving the provider-env is easy enough.

fejta · 2016-09-28T01:48:33Z

jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-kops-aws.yaml

+            job-env: |
+                export GINKGO_TEST_ARGS="--ginkgo.focus=\[k8s.io\]\sNetworking.*\[Conformance\]"
+                export GINKGO_PARALLEL="y"
+                export PROJECT="fake-project-ignored"


Hmm? Sadly I think this will cause our tooling to validate that fake-project-ignored has the right IAM bindings :)

It's really not easy for me to test this, but the last time I ran something through e2e-runner without PROJECT set correctly, it shit a brick. Do you want to create a project that we can assign to non-projects?

Oh, I can fix e2e-runner. It just needs a provider check around the PROJECT check.

fejta · 2016-09-28T01:49:49Z

jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-kops-aws.yaml

+            days-to-keep: 7
+    triggers:
+        - reverse:
+            jobs: '{trigger-job}'


Consider removing as many of these as appropriate (we only trigger one way, we only define one version of emails)

I've looked at this a little more, and this is basically stock boilerplate for any build triggered job template. If you want a wider cleanup, let's address it in another PR, but https://github.com/kubernetes/test-infra/blob/master/jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-gce.yaml and https://github.com/kubernetes/test-infra/blob/master/jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-gke.yaml both follow this. Am I missing something?

kubernetes-e2e-gce.yaml has multiple values for trigger-job:

kubernetes-e2e-gce uses kubernetes-build while kubernetes-e2e-gce-federation uses kubernetes-federation-build

Your job has only one value for trigger-job right now, therefore the intermediate variable is unnecessary.

If you write jobs: 'kubernetes-build' the next person has the information right in front of their eyes instead of needing to go hunt down where trigger-job is defined.

zmerlynn · 2016-09-28T18:57:12Z

I addressed some of the review comments and fixed up kubernetes-e2e-aws.yaml similarly. I'm happy to discuss offline where to put the download code.

fejta · 2016-09-28T23:48:59Z

jenkins/dockerized-e2e-runner.sh

@@ -61,6 +61,12 @@ if [[ "${KUBE_RUN_FROM_OUTPUT:-}" =~ ^[yY]$ ]]; then
    )
 fi

+if [[ -n "${KUBE_E2E_RUNNER:-}" ]]; then
+    docker_extra_args+=(\
+      --entrypoint="${KUBE_E2E_RUNNER}" \


fejta · 2016-09-28T23:49:25Z

jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-aws.yaml

    builders:
        - activate-gce-service-account
        - shell: |
-            {provider-env}
+            export KUBERNETES_PROVIDER="aws"


Yay! This looks MUCH better!!

fejta · 2016-09-28T23:49:59Z

jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-kops-aws.yaml

+    builders:
+        - activate-gce-service-account
+        - shell: |
+            export AWS_CONFIG_FILE="${{WORKSPACE}}/.aws/credentials"


This one too :-D

fejta · 2016-09-28T23:50:38Z

jenkins/job-configs/kubernetes-jenkins/kubernetes-e2e-kops-aws.yaml

+            {provider-env}
+            {job-env}
+            {post-env}
+            if ! curl -fsS --retry 3 -o "$WORKSPACE/kops.url" "https://storage.googleapis.com/kops-ci/bin/latest-ci.txt"; then


fejta · 2016-09-28T23:51:02Z

jenkins/e2e-image/Dockerfile

@@ -44,10 +44,11 @@ ENV FAIL_ON_GCP_RESOURCE_LEAK=true \
 # JENKINS_AWS_CREDENTIALS_FILE

 ADD ["e2e-runner.sh", \
+    "kops-e2e-runner.sh", \


Do you plan to add this file to the PR?

Woops. Fixed!

fejta · 2016-09-29T00:02:19Z

LGTM

fejta · 2016-09-29T00:03:19Z

Note that we will need to push a new kubekins-e2e image for this (talk to @ixdy)

zmerlynn · 2016-09-29T00:09:10Z

Yup. I can do it, or someone else can. If this seems fine, merge and I can push a new image and suggest a new tag later (but it may not be until Monday if I'm doing it).

fejta · 2016-09-29T05:35:28Z

Given that various pieces here do and do not run from head I'd suggest we wait until Monday morning when you are back to a) merge this PR and b) create the new image.

I suspect if we merge now the aws tests will spend the rest of the week in a fail loop

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

zmerlynn · 2016-10-03T16:24:55Z

I just rebased and I'm retesting this in kubernetes/kubernetes#33700 again. I'll merge when that seems clean, then propose a new tag.

zmerlynn · 2016-10-03T16:56:34Z

Merging, #33700 is collecting greens.

Automatic merge from submit-queue. fix typo from unit test to unit tests

* Ignore blank line when counting pending jobs. * Fix race condition by looking at jobs, not pods. Pods may show up in all sort of ordering, so we may see 1 pod out of N completed, while the rest haven't been scheduled. If we look at jobs themselves and compare expected with completions we won't be surprised by the race. * Use custom column to avoid kubectl versioning. * Fix run-away line.

zmerlynn assigned fejta and spxtr Sep 27, 2016

googlebot added the cla: yes label Sep 27, 2016

zmerlynn mentioned this pull request Sep 27, 2016

e2e.go: Add -deployment, add a kops deployment method kubernetes/kubernetes#33518

Merged

fejta reviewed Sep 28, 2016

View reviewed changes

zmerlynn force-pushed the add-kops-updown branch from b4a9642 to 1116a9f Compare September 28, 2016 20:16

zmerlynn mentioned this pull request Sep 28, 2016

DO NOT MERGE: Testing kops e2e-image kubernetes/kubernetes#33700

Closed

zmerlynn force-pushed the add-kops-updown branch 2 times, most recently from f561e5b to 76852d2 Compare September 28, 2016 23:14

fejta reviewed Sep 28, 2016

View reviewed changes

zmerlynn force-pushed the add-kops-updown branch from 76852d2 to ed84680 Compare September 28, 2016 23:58

fejta added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2016

fejta assigned ixdy Sep 29, 2016

kops: Add a kops-updown job

9bc6f1c

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

zmerlynn force-pushed the add-kops-updown branch from ed84680 to 9bc6f1c Compare October 3, 2016 16:18

zmerlynn merged commit 2ef42a4 into kubernetes:master Oct 3, 2016

zmerlynn deleted the add-kops-updown branch October 3, 2016 16:56

zmerlynn mentioned this pull request Oct 3, 2016

Bump e2e tag to v20161003-2ef42a4 #731

Merged

ostromart pushed a commit to ostromart/test-infra that referenced this pull request Jul 26, 2019

fix typo from unit test to unit tests (kubernetes#681)

a3504e2

Automatic merge from submit-queue. fix typo from unit test to unit tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kops: Add a kops-updown job #681

kops: Add a kops-updown job #681

zmerlynn commented Sep 27, 2016 •

edited by thockin

Loading

fejta Sep 28, 2016

zmerlynn Sep 28, 2016

zmerlynn Sep 28, 2016

zmerlynn Sep 28, 2016 •

edited

Loading

fejta Sep 28, 2016 •

edited

Loading

zmerlynn Sep 28, 2016 •

edited

Loading

fejta Sep 28, 2016

fejta Sep 28, 2016

zmerlynn Sep 28, 2016

fejta Sep 28, 2016 •

edited

Loading

zmerlynn Sep 28, 2016

zmerlynn Sep 28, 2016

fejta Sep 28, 2016 •

edited

Loading

zmerlynn Sep 28, 2016

fejta Sep 28, 2016

zmerlynn Sep 28, 2016

zmerlynn commented Sep 28, 2016

fejta Sep 28, 2016

fejta Sep 28, 2016

fejta Sep 28, 2016

fejta Sep 28, 2016

fejta Sep 28, 2016 •

edited

Loading

zmerlynn Sep 28, 2016

fejta commented Sep 29, 2016

fejta commented Sep 29, 2016

zmerlynn commented Sep 29, 2016

fejta commented Sep 29, 2016

zmerlynn commented Oct 3, 2016 •

edited

Loading

zmerlynn commented Oct 3, 2016

kops: Add a kops-updown job #681

kops: Add a kops-updown job #681

Conversation

zmerlynn commented Sep 27, 2016 • edited by thockin Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmerlynn Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

fejta Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

zmerlynn Sep 28, 2016 • 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

fejta Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta Sep 28, 2016 • 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

zmerlynn commented Sep 28, 2016

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

fejta Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta commented Sep 29, 2016

fejta commented Sep 29, 2016

zmerlynn commented Sep 29, 2016

fejta commented Sep 29, 2016

zmerlynn commented Oct 3, 2016 • edited Loading

zmerlynn commented Oct 3, 2016

zmerlynn commented Sep 27, 2016 •

edited by thockin

Loading

zmerlynn Sep 28, 2016 •

edited

Loading

fejta Sep 28, 2016 •

edited

Loading

zmerlynn Sep 28, 2016 •

edited

Loading

fejta Sep 28, 2016 •

edited

Loading

fejta Sep 28, 2016 •

edited

Loading

fejta Sep 28, 2016 •

edited

Loading

zmerlynn commented Oct 3, 2016 •

edited

Loading