Fix test failures for various combinations of kubectl and Kubernetes versions #198

skaes · 2017-10-31T14:46:16Z

This patch fixes two different problems:

Kubernetes 1.8 has changed the behavior when secrets cannot be found.
Instead of aborting a deployment quickly, Kubernetes retries quite a
few times before it gives up. Removed the test case which checks for
a quick exit.
kubectl 1.8 produces slighty different log output for some of the test
cases. Made the matches version dependent. Another option would be to
only check the parts which are identical in both versions.

…versions This patch fixes two different problems: 1. Kubernetes 1.8 has changed the behavior when secrets cannot be found. Instead of aborting a deployment quickly, Kubernetes retries quite a few times before it gives up. Removed the test case which checks for a quick exit. 2. kubectl 1.8 produces slighty different log output for some of the test cases. Made the matches version dependent. Another option would be to only check the parts which are identical in both versions.

stefanmb · 2017-11-03T13:56:30Z

I can confirm this PR fixes all known test failures on k8s 1.8.2 (and also passes on 1.7.6 and 1.7.9). Test run here: https://buildkite.com/shopify/kubernetes-deploy-gem/builds/206

stefanmb · 2017-11-03T14:00:32Z

@skaes Could I ask you to fix the trivial style issues reported here? https://policial.shopify.io/Shopify/kubernetes-deploy/builds/357126

It'll make merging this easier. Thank you!

KnVerey

Thanks for the PR! I just have a few requests. 😄

KnVerey · 2017-11-03T22:18:42Z

lib/kubernetes-deploy/kubectl.rb

+      @version_info ||=
+        begin
+          response, _, status = run("version", "--output=yaml", use_namespace: false, log_failure: true)
+          raise "Could not retrieve kubectl client info" unless status.success?


Please raise KubectlError

Nit: kubectl client info -> Kubernetes version info (we get both the kubectl client and the server versions, as you clearly know from the methods below 😄 )

Sure thing. Did not know about KubectlError

KnVerey · 2017-11-03T22:21:41Z

lib/kubernetes-deploy/kubectl.rb

+          response, _, status = run("version", "--output=yaml", use_namespace: false, log_failure: true)
+          raise "Could not retrieve kubectl client info" unless status.success?
+          YAML.load(response)
+        rescue => e


why rescue and re-raise? If run itself raised something, I'd generally consider that unexpected and let it raise up

Fine with me. But the user might be confused if he sees for example an error from YAML.load.

Ah, I wasn't noticing the YAML.load (even though it's right there 😄 ). I'm fine either way on this one.

KnVerey · 2017-11-03T22:28:56Z

lib/kubernetes-deploy/kubectl.rb

+    end
+
+    def client_version
+      version_info["clientVersion"]["gitVersion"]


Let's make this return something that facilitates accurate comparison. For example, we could return instances of Gem::Version. Currently, comparisons like "v1.7.9" < "v1.7.10" will give the wrong result.

I figured that I'll never need an exact comparison. Only comparisons of the form x < 1.8 or x > 1.7. But I can change it, of course.

KnVerey · 2017-11-03T22:41:42Z

lib/kubernetes-deploy/runner.rb

      with_retries(2) do
-        _, _, st = kubectl.run("version", use_namespace: false, log_failure: true)
+        response, _, st = kubectl.run("version", "--output=yaml", use_namespace: false, log_failure: true)


As it stands, we don't actually need kubectl_version_info for anything in this class. We're using version here as an arbitrary command that actually hits the server, to confirm that the master IP we got from the kubeconfig was valid. By explicitly doing this early on, we can fail with a relevant error instead of an error about whichever step (e.g. namespace validation) happened to hit the server first.

In other words, we can switch this line to use your new kubectl.version_info, but let's leave the rest alone. Anything that does need the version in the future can use your new method directly.

KnVerey · 2017-11-03T22:49:59Z

test/integration/kubernetes_deploy_test.rb

-    end
-    assert_deploy_failure(result)
+  if KUBE_SERVER_VERSION < "v1.8.0"
+    # behavior in 1.8 has changed: kubernetes now times out instead of failing quickly


Hm, maybe this is something we can address in the gem. I've opened an issue to investigate: #203.

KnVerey · 2017-11-06T19:53:36Z

@skaes any chance you'd be able to update this PR in the next day? We've got our v1.8 CI ready to go now, so we're eager to have this in. 😄

skaes · 2017-11-07T19:49:18Z

@KnVerey I think I have addressed all your concerns with the latest commits. Would be great to see a new version soon!

BTW, any chance you might merge #140 soon? We do have StatefulSets too.

KnVerey

Changes look good, thanks! I'd appreciate it if you could fix the rubocop exceptions as @stefanmb mentioned, particularly the Avoid using rescue in its modifier form. (commented in line as well): https://policial.shopify.io/Shopify/kubernetes-deploy/builds/364227. I know it can be annoying, but we're trying to keep this project green. If you don't have time, LMK and I'll do it after merging.

KnVerey · 2017-11-07T21:02:51Z

lib/kubernetes-deploy/runner.rb

@@ -411,10 +411,8 @@ def confirm_context_exists
    def confirm_cluster_reachable
      success = false
      with_retries(2) do
-        _, _, st = kubectl.run("version", use_namespace: false, log_failure: true)
-        success = st.success?
+        success = kubectl.version_info rescue false


Please rescue specific exceptions (Psych::SyntaxError / KubectlError)

KnVerey · 2017-11-07T21:09:27Z

Actually, CI just finished and we have a problem on 1.6.4:


[2017-11-07T21:00:05Z] [WARN][2017-11-07 21:00:05 +0000]	The following command failed: kubectl version --output\=yaml --context\=minikube --request-timeout\=5s
--
  | [2017-11-07T21:00:05Z] [WARN][2017-11-07 21:00:05 +0000]	Error: unknown flag: --output
  | [2017-11-07T21:00:05Z]
  | [2017-11-07T21:00:05Z]
  | [2017-11-07T21:00:05Z] Examples:
  | [2017-11-07T21:00:05Z]   # Print the client and server versions for the current context
  | [2017-11-07T21:00:05Z]   kubectl version
  | [2017-11-07T21:00:05Z]
  | [2017-11-07T21:00:05Z] Options:
  | [2017-11-07T21:00:05Z]   -c, --client=false: Client version only (no server required).
  | [2017-11-07T21:00:05Z]       --short=false: Print just the version number.
  | [2017-11-07T21:00:05Z]
  | [2017-11-07T21:00:05Z] Usage:
  | [2017-11-07T21:00:05Z]   kubectl version [options]
  | [2017-11-07T21:00:05Z]
  | [2017-11-07T21:00:05Z] Use "kubectl options" for a list of global command-line options (applies to all commands).

KnVerey · 2017-11-07T21:24:35Z

As for the StatefulSet PR, it is almost ready to ship, but we actually need your PR to merge first so that we can exclude rollingUpdate tests on < v1.7. 🙏

I sincerely hop this will make the code style checker happy.

kubectl before version 1.7 don't support the --output parameter.

skaes · 2017-11-08T08:27:05Z

Hopefully this is good to go now.

KnVerey · 2017-11-08T18:02:19Z

CI run was green: https://buildkite.com/shopify/kubernetes-deploy-gem/builds/225. Thanks again for the contribution!

stefanmb · 2017-11-08T18:07:13Z

@KnVerey That run did not include 1.8, but it was passing previously.

KnVerey · 2017-11-10T18:28:49Z

@skaes FYI we released version 0.13.0 yesterday, which includes the StatefulSet support. 😄

KnVerey requested review from KnVerey and kirs October 31, 2017 20:31

stefanmb approved these changes Nov 3, 2017

View reviewed changes

stefanmb mentioned this pull request Nov 3, 2017

Update CI Setup for k8s 1.7.x, 1.8.x #200

Merged

KnVerey suggested changes Nov 3, 2017

View reviewed changes

skaes added 5 commits November 7, 2017 19:55

Merge branch 'master' into fix-test-failures-for-1.8

812e385

use Gem::Version for kubectl.client_version and kubectl.server_version

36b7f56

eliminate duplicate code for retrieving version info

aecc038

avoid re-raising an exception

6cc7fa8

restarts can take double digit seconds

f7b7386

KnVerey approved these changes Nov 7, 2017

View reviewed changes

skaes added 5 commits November 8, 2017 08:22

deleted an empty line

f8aab3b

explicitly rescue KubectlError

1a970e2

semantics preserving change required by enforcing coding conventions

8e28cc5

break up some long lines into two lines

cea7b5d

I sincerely hop this will make the code style checker happy.

parse kubectl version response using regular expressions

7ed5957

kubectl before version 1.7 don't support the --output parameter.

KnVerey merged commit 4ead9cc into Shopify:master Nov 8, 2017

karanthukral deployed to rubygems November 9, 2017 19:58 Active

skaes deleted the fix-test-failures-for-1.8 branch November 11, 2017 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test failures for various combinations of kubectl and Kubernetes versions #198

Fix test failures for various combinations of kubectl and Kubernetes versions #198

skaes commented Oct 31, 2017

stefanmb commented Nov 3, 2017

stefanmb commented Nov 3, 2017

KnVerey left a comment

KnVerey Nov 3, 2017

skaes Nov 7, 2017

KnVerey Nov 3, 2017

skaes Nov 7, 2017

KnVerey Nov 7, 2017

KnVerey Nov 3, 2017

skaes Nov 7, 2017

KnVerey Nov 3, 2017

skaes Nov 7, 2017

KnVerey Nov 3, 2017

KnVerey commented Nov 6, 2017

skaes commented Nov 7, 2017

KnVerey left a comment

KnVerey Nov 7, 2017

KnVerey commented Nov 7, 2017

KnVerey commented Nov 7, 2017

skaes commented Nov 8, 2017

KnVerey commented Nov 8, 2017

stefanmb commented Nov 8, 2017

KnVerey commented Nov 10, 2017

Fix test failures for various combinations of kubectl and Kubernetes versions #198

Fix test failures for various combinations of kubectl and Kubernetes versions #198

Conversation

skaes commented Oct 31, 2017

stefanmb commented Nov 3, 2017

stefanmb commented Nov 3, 2017

KnVerey 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

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

KnVerey commented Nov 6, 2017

skaes commented Nov 7, 2017

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Nov 7, 2017

KnVerey commented Nov 7, 2017

skaes commented Nov 8, 2017

KnVerey commented Nov 8, 2017

stefanmb commented Nov 8, 2017

KnVerey commented Nov 10, 2017