e2e-kubeadm: Allow pinning Kubernetes version. #2761

pipejakob · 2017-05-15T21:19:09Z

Add support for kubeadm e2e jobs to specify a version string to use that ultimately gets passed as kubeadm init --kubernetes-version <version>. This is important for e2e jobs running against release branches, instead of always using latest.

pipejakob · 2017-05-15T21:22:03Z

/assign @krzyzacy

pipejakob · 2017-05-15T21:50:31Z

Not sure if I'm accumulating too much kubeadm-specific logic into the kubernetes_e2e.py scenario. At any point, feel free to push back and I'll fork it off into its own scenario.

krzyzacy · 2017-05-16T00:33:33Z

yeah, sounds sane to move all kubeadm stuff to it's own scenario, or in kubetest/anywhere.go?

pipejakob · 2017-05-16T20:36:14Z

Sure. Just two questions:

Does this PR itself look okay? This (in addition to Fix e2e-kubeadm release-1.6 job. #2765) should bring the kubeadm release-1.6 job back to green, and this can become the basis of converting the relevant parts of kubernetes_e2e.py to a separate kubeadm_e2e.py scenario. If it's not too much trouble, I'd like to get this change in first, and then follow it up with a PR to extract all of the kubeadm logic to its own scenario instead of conflating the two.
Is there documentation on the significance of the different layers in test-infra, or guidelines around what functionality should go into each layer? I've been trying to just observe and follow suit for what belongs in kubetest vs. scenarios vs. jobs/config.json vs. env files vs. prow's config file, etc., but I still don't feel like I have a good handle on where something brand new should go, if I don't already have several examples setting precedence. (This may be a @fejta question)

krzyzacy · 2017-05-17T17:34:06Z

scenarios/kubernetes_e2e.py

+
+        k8s_version = mode.job_env().get('KUBEADM_KUBERNETES_VERSION')
+        if k8s_version:
+            opt += ' --kubernetes-anywhere-kubernetes-version %s' % k8s_version


update test :-)

krzyzacy · 2017-05-17T17:38:32Z

scenarios/kubernetes_e2e.py

-    def start(self, args):
-        """Runs e2e-runner.sh after setting env and installing prereqs."""
-        print >>sys.stderr, 'starts with local mode'
+    def job_env(self):


maybe this should be a property

krzyzacy · 2017-05-17T17:40:35Z

scenarios/kubernetes_e2e.py

@@ -377,6 +382,11 @@ def main(args):
            ' --kubernetes-anywhere-phase2-provider kubeadm' \
            ' --kubernetes-anywhere-cluster %s' \
            ' --kubernetes-anywhere-kubeadm-version %s' % (cluster, version)
+
+        k8s_version = mode.job_env().get('KUBEADM_KUBERNETES_VERSION')


docker mode does not have job_env() though... you might want to check mutual exclusive?

krzyzacy · 2017-05-17T17:41:43Z

It seems all you want for kubeadm is add bunch of flags to kubetest. You can create the scenario and pass those flags through config.json, like, kubernetes_kubeadm.py --kubeadm-k8s-version=latest-1.6, plus all your kubeadm jobs does not need to support docker mode.

But sgtm if you want to get this in first and refactor in a separate PR :-)

pipejakob · 2017-06-15T22:43:18Z

Looks like I need to convert my JENKINS_PUBLISHED_VERSION settings to --extract settings. Working on that now, and will retest.

pipejakob · 2017-06-15T23:42:56Z

Okay, I've converted this functionality from an environment variable in the .env files to instead be a new flag, which simplifies the implementation and tests, as well as keeps more of the per-job settings bundled together in jobs/config.json. Care to take another look?

pipejakob · 2017-06-16T20:52:00Z

(Rebased)

krzyzacy · 2017-06-16T21:01:54Z

lg overall, please fix the presubmit

krzyzacy · 2017-06-16T21:02:41Z

and will this make https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-e2e-kubeadm-gce-1-7/2?log#log better?

pipejakob · 2017-06-16T21:03:54Z

Sweet! Yes, this should fix both the release-1.6 and release-1.7 CI tests.

krzyzacy · 2017-06-16T21:21:59Z

it might also conflict with #3084, let's see who gets in first

pipejakob · 2017-06-16T21:22:56Z

The race is on. I'll go distract Erick while the presubmits finish.

pipejakob · 2017-06-16T21:30:07Z

I lost the race :-(. Rebasing now.

krzyzacy · 2017-06-16T21:39:29Z

cc @caesarxuchao this should fix kubeadm in 1.7 you asked earlier

krzyzacy · 2017-06-16T21:43:49Z

scenarios/kubernetes_e2e.py

@@ -473,6 +479,9 @@ def create_parser():
    parser.add_argument(
        '--kubeadm', choices=['ci', 'periodic', 'pull'])
    parser.add_argument(
+        '--kubeadm-kubernetes-version', default=None, help='Version of Kubernetes to use while '
+        'testing kubeadm (e.g. latest, latest-1.6, 1.6.4, etc)')


you can just pass in --kubernetes-anywhere-kubernetes-version=x.x.x from config.json, rest should be automatic, which means you don't need to create a new flag here.

I did consider this after I saw #3082. I have been intentionally trying to keep the options to the scenario to be kubeadm-based (and keeping kubernetes-anywhere an implementation detail under the covers). It's probably worth redoing the kubetest args to be consistent with these in the future, since kubeadm is the only thing we test using kubernetes-anywhere.

I would probably add some presubmit check say --kubeadm-kubernetes-version must set --kubeadm, rather than complicate the logic here? Since this is against #3082.

maybe make https://github.com/kubernetes/test-infra/blob/master/jenkins/bootstrap_test.py#L2159 more complicated :-)

I'm a little confused. What logic did you want removed from here? Or are you saying that we should add logic to make sure that if someone specifies --kubeadm-kubernetes-version and forgets to set --kubeadm, we should have an error, but that new logic should go into a presubmit test instead of a runtime check in the scenario?

pipejakob · 2017-06-16T21:44:11Z

Rebased again and fixed a bug where all of the kubernetes-anywhere flags had prepended spaces, which made kubetest reject them.

pipejakob · 2017-06-16T21:47:52Z

Okay I don't think that failure was because of me:

W0616 21:45:08.638] unexpected pipe read status: (error: 22): Invalid argument
W0616 21:45:08.638] Server presumed dead. Now printing '/root/.cache/bazel/_bazel_root/27a95fe2fc68aa1195ddf1a4bbaaea62/server/jvm.out':

/retest

Add support for kubeadm e2e jobs to specify a version string to use that ultimately gets passed as "kubeadm init --kubernetes-version <version>". This is important for e2e jobs running against release branches, instead of always using "latest".

pipejakob · 2017-06-16T22:37:38Z

@krzyzacy and I chatted offline. I've pulled out the new kubeadm flag in favor of just using the kubetest kubernetes-anywhere flag instead. I'm still testing the jobs locally, but please have a look!

krzyzacy · 2017-06-16T22:39:49Z

/lgtm

krzyzacy · 2017-06-16T22:40:09Z

hope it will fix things :-)

pipejakob · 2017-06-16T22:43:27Z

Well I was going to finish testing before merging, but I guess we'll roll forward if any problems are found. 😄

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

k8s-ci-robot assigned krzyzacy May 15, 2017

krzyzacy reviewed May 17, 2017

View reviewed changes

pipejakob mentioned this pull request Jun 13, 2017

Release branches e2e's should use k8s from release branches kubernetes/kubeadm#237

Closed

pipejakob force-pushed the pin-k8s-version branch 2 times, most recently from 735b1f7 to ec0b124 Compare June 15, 2017 22:37

pipejakob force-pushed the pin-k8s-version branch 3 times, most recently from 4672969 to 04c768d Compare June 15, 2017 23:28

pipejakob force-pushed the pin-k8s-version branch from 04c768d to ebc12f8 Compare June 16, 2017 02:18

pipejakob force-pushed the pin-k8s-version branch from ebc12f8 to 9fac2f9 Compare June 16, 2017 20:52

pipejakob force-pushed the pin-k8s-version branch from 9fac2f9 to b2f2d90 Compare June 16, 2017 21:18

pipejakob force-pushed the pin-k8s-version branch from b2f2d90 to 1bb693c Compare June 16, 2017 21:39

pipejakob force-pushed the pin-k8s-version branch from 1bb693c to b948f18 Compare June 16, 2017 21:43

krzyzacy reviewed Jun 16, 2017

View reviewed changes

pipejakob force-pushed the pin-k8s-version branch from b948f18 to 3909d9d Compare June 16, 2017 22:33

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

krzyzacy merged commit 59792dc into kubernetes:master Jun 16, 2017

luxas mentioned this pull request Jun 20, 2017

kubeadm e2e targeted at HEAD should use control plane images from master kubernetes/kubeadm#309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e-kubeadm: Allow pinning Kubernetes version. #2761

e2e-kubeadm: Allow pinning Kubernetes version. #2761

pipejakob commented May 15, 2017

pipejakob commented May 15, 2017

pipejakob commented May 15, 2017

krzyzacy commented May 16, 2017

pipejakob commented May 16, 2017

krzyzacy May 17, 2017

krzyzacy May 17, 2017

krzyzacy May 17, 2017

krzyzacy commented May 17, 2017 •

edited

Loading

pipejakob commented Jun 15, 2017

pipejakob commented Jun 15, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

pipejakob commented Jun 16, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

krzyzacy Jun 16, 2017 •

edited

Loading

pipejakob Jun 16, 2017

krzyzacy Jun 16, 2017

krzyzacy Jun 16, 2017

pipejakob Jun 16, 2017

pipejakob commented Jun 16, 2017

pipejakob commented Jun 16, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

pipejakob commented Jun 16, 2017

e2e-kubeadm: Allow pinning Kubernetes version. #2761

e2e-kubeadm: Allow pinning Kubernetes version. #2761

Conversation

pipejakob commented May 15, 2017

pipejakob commented May 15, 2017

pipejakob commented May 15, 2017

krzyzacy commented May 16, 2017

pipejakob commented May 16, 2017

krzyzacy May 17, 2017

Choose a reason for hiding this comment

krzyzacy May 17, 2017

Choose a reason for hiding this comment

krzyzacy May 17, 2017

Choose a reason for hiding this comment

krzyzacy commented May 17, 2017 • edited Loading

pipejakob commented Jun 15, 2017

pipejakob commented Jun 15, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

pipejakob commented Jun 16, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

krzyzacy Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

pipejakob Jun 16, 2017

Choose a reason for hiding this comment

krzyzacy Jun 16, 2017

Choose a reason for hiding this comment

krzyzacy Jun 16, 2017

Choose a reason for hiding this comment

pipejakob Jun 16, 2017

Choose a reason for hiding this comment

pipejakob commented Jun 16, 2017

pipejakob commented Jun 16, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

krzyzacy commented Jun 16, 2017

pipejakob commented Jun 16, 2017

krzyzacy commented May 17, 2017 •

edited

Loading

krzyzacy Jun 16, 2017 •

edited

Loading