Warn about kubernetes minimum version #237

Fattouche · 2018-01-12T16:49:25Z

This fixes #152.

To be completely honest I have no idea if this is what you were looking for. Please feel free to tell me I am completely wrong 👍 , also any pointers in the right direction would be much appreciated :)

KnVerey

To be completely honest I have no idea if this is what you were looking for.

This is definitely on the right track! We should also add the warning in the other two tasks, RunnerTask and RestartTask.

KnVerey · 2018-01-12T18:38:17Z

lib/kubernetes-deploy/deploy_task.rb

@@ -99,6 +99,7 @@ def initialize(namespace:, context:, current_sha:, template_dir:, logger:, kubec
      @logger = logger
      @kubectl = kubectl_instance
      @bindings = bindings
+      @min_version = '1.6.0'


This should be a constant IMO

KnVerey · 2018-01-12T18:45:24Z

lib/kubernetes-deploy/deploy_task.rb

@@ -340,6 +341,10 @@ def deploy_resources(resources, prune: false, verify:, record_summary: true)
      return if resources.empty?
      deploy_started_at = Time.now.utc

+      if server_version < Gem::Version.new(@min_version)
+        @logger.warn("Minimum cluster version requirement of #{@min_version} not met.")
+      end


deploy_resources actually gets called several times during the deploy process. Let's check this during the "Initializing deploy" phase. I'm thinking confirm_cluster_reachable would be an appropriate point.

That message looks good, but I'd suggest adding something about what the requirement means, i.e. that kubernetes-deploy is not tested against earlier versions of Kubernetes and may not work as expected.

Btw one way to 🎩 this would be to start an old version of minikube and run PRINT_LOGS=1 bundle exec ruby -I test test/integration/kubernetes_deploy_test.rb -n/full_hello/.

Thanks, I was wondering how I could test this stuff!

Fattouche · 2018-01-15T18:59:14Z

lib/kubernetes-deploy/restart_task.rb

@@ -19,6 +20,10 @@ def initialize(deployment_name, response)
    HTTP_OK_RANGE = 200..299
    ANNOTATION = "shipit.shopify.io/restart"

+    def server_version
+      kubectl.server_version


I don't know how to go about getting the server_version in these tasks that don't already have kubectl defined. Is it okay to require Kubectl.rb and then just define kubectl like its done in deploy_task:

def kubectl @kubectl ||= Kubectl.new(namespace: @namespace, context: @context, logger: @logger, log_failure_by_default: true) end

I guess where my knowledge falls short is during run time. It looks like both restart_task and runner_task end up using deploy_task(atleast in testing) but does that mean that I can use the server_version function defined in deploy_task in the other classes? What I do now just seems like a lot of code duplication.
@KnVerey @stefanmb

looks like both restart_task and runner_task end up using deploy_task(atleast in testing)

I'm not sure what you're looking at here. The tasks share some tools (e.g. the resource watcher), but they're independent.

Is it okay to require Kubectl.rb and then just define kubectl like its done in deploy_task

Yes. What you have now now is the correct approach IMO. I personally wouldn't bother wrapping kubectl.server_version as server_version everywhere, but that doesn't matter at all. The reason the other two tasks don't already use kubectl extensively is that we were trying to stick to the Kubeclient gem instead. But I've checked pretty carefully, and it doesn't seem to expose any server metadata.

KnVerey · 2018-01-15T19:47:03Z

lib/kubernetes-deploy/deploy_task.rb

@@ -432,6 +432,10 @@ def confirm_context_exists
    end

    def confirm_cluster_reachable
+      if server_version < Gem::Version.new(MIN_KUBE_VERSION)


You'll want to do this after the rest of the code in this method; the rest of the code confirms that the server actually exists, so we should do that before trying to ask it what its version is.

KnVerey · 2018-01-15T19:52:51Z

lib/kubernetes-deploy/restart_task.rb

@@ -19,6 +20,10 @@ def initialize(deployment_name, response)
    HTTP_OK_RANGE = 200..299
    ANNOTATION = "shipit.shopify.io/restart"

+    def server_version
+      kubectl.server_version


looks like both restart_task and runner_task end up using deploy_task(atleast in testing)

I'm not sure what you're looking at here. The tasks share some tools (e.g. the resource watcher), but they're independent.

Is it okay to require Kubectl.rb and then just define kubectl like its done in deploy_task

Yes. What you have now now is the correct approach IMO. I personally wouldn't bother wrapping kubectl.server_version as server_version everywhere, but that doesn't matter at all. The reason the other two tasks don't already use kubectl extensively is that we were trying to stick to the Kubeclient gem instead. But I've checked pretty carefully, and it doesn't seem to expose any server metadata.

KnVerey · 2018-01-15T19:58:11Z

lib/kubernetes-deploy/restart_task.rb

-
+      if server_version < Gem::Version.new(MIN_KUBE_VERSION)
+        @logger.warn("Minimum cluster version requirement of #{MIN_KUBE_VERSION} not met. "\
+        "Using #{server_version} could result in unexpected behavior as it is no longer tested against")


You could make a common method (maybe KubernetesDeploy::Errors.server_version_warning(server_version)?) that all three tasks could call to avoid repeating the full string everywhere.

KnVerey · 2018-01-15T19:59:41Z

lib/kubernetes-deploy/kubectl.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true

 module KubernetesDeploy
+  MIN_KUBE_VERSION = '1.6.0'


I'd suggest putting this directly in kubernetes-deploy.rb since you're defining it on KubernetesDeploy, not on KubernetesDeploy::Kubectl.

KnVerey · 2018-01-15T20:00:31Z

lib/kubernetes-deploy/runner_task.rb

    def initialize(namespace:, context:, logger:)
      @logger = logger
      @namespace = namespace
      @kubeclient = build_v1_kubeclient(context)
      @context = context
+      @kubectl = nil


This won't do anything

Fattouche · 2018-01-15T20:53:26Z

lib/kubernetes-deploy/errors.rb

@@ -8,4 +8,12 @@ def initialize(name, context)
      super("Namespace `#{name}` not found in context `#{context}`")
    end
  end
+  class Errors
+    def self.server_version_warning(server_version, logger)


Does it make sense to put this inside its own Errors class, would it be sufficient to just keep it in the errors module to stay with the library(for module) and object(for class) model? This is the only time I have really written any Ruby apart from tutorials so I am not sure about this code architecture.
@KnVerey

The bindings parser is an example of a pattern you can use when instances don't really make sense: Make Errors a module that extend self. There's no state to hold here, so making it a module makes sense to me. What I had in mind in suggesting it could go in Errors was more extracting the message only though (i.e. it would be a constant, except that we need to pass in the actual server version):

def server_version_warning(server_version) "Minimum cluster version requirement of #{MIN_KUBE_VERSION} not met. "\ "Using #{server_version} could result in unexpected behavior as it is no longer tested against" end

An alternative if you want to extract everything you've got here would be to put the method in the Kubectl class and call it something like validate_server_version. That works for me too.

After reading up on the idea of extending self in a module instead of self.method inside a class you are totally right! Thanks for the tip 👍

KnVerey · 2018-01-15T22:52:05Z

lib/kubernetes-deploy/deploy_task.rb

@@ -441,6 +441,7 @@ def confirm_cluster_reachable
        end
      end
      raise FatalDeploymentError, "Failed to reach server for #{@context}" unless success
+      KubernetesDeploy::Errors.server_version_warning(server_version, @logger)


Since the current class here is also namespaced inside KubernetesDeploy, you don't need the prefix here

KnVerey · 2018-01-15T22:54:58Z

lib/kubernetes-deploy/errors.rb

@@ -8,4 +8,12 @@ def initialize(name, context)
      super("Namespace `#{name}` not found in context `#{context}`")
    end
  end
+  class Errors
+    def self.server_version_warning(server_version, logger)


The bindings parser is an example of a pattern you can use when instances don't really make sense: Make Errors a module that extend self. There's no state to hold here, so making it a module makes sense to me. What I had in mind in suggesting it could go in Errors was more extracting the message only though (i.e. it would be a constant, except that we need to pass in the actual server version):

def server_version_warning(server_version) "Minimum cluster version requirement of #{MIN_KUBE_VERSION} not met. "\ "Using #{server_version} could result in unexpected behavior as it is no longer tested against" end

An alternative if you want to extract everything you've got here would be to put the method in the Kubectl class and call it something like validate_server_version. That works for me too.

Initial warning about version

b022995

Fattouche requested a review from KnVerey January 12, 2018 16:49

code style

67f452a

KnVerey reviewed Jan 12, 2018

View reviewed changes

Fattouche added 2 commits January 12, 2018 16:03

Adding to other files

5f98639

Requiring kubectl

b92f0fe

Fattouche commented Jan 15, 2018

View reviewed changes

Fattouche requested a review from stefanmb January 15, 2018 18:59

KnVerey reviewed Jan 15, 2018

View reviewed changes

Suggestion changes

be7dcfe

Fattouche commented Jan 15, 2018

View reviewed changes

KnVerey reviewed Jan 15, 2018

View reviewed changes

Doing logic checks for server version in respective classes

f961b94

KnVerey approved these changes Jan 16, 2018

View reviewed changes

Fattouche merged commit ef200ee into master Jan 16, 2018

Fattouche deleted the warn_kube_version branch January 16, 2018 21:47

KnVerey mentioned this pull request Jan 18, 2018

WIP: Fail if server version is less than minimum required #189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn about kubernetes minimum version #237

Warn about kubernetes minimum version #237

Fattouche commented Jan 12, 2018

KnVerey left a comment

KnVerey Jan 12, 2018

KnVerey Jan 12, 2018

KnVerey Jan 12, 2018

Fattouche Jan 12, 2018

Fattouche Jan 15, 2018 •

edited

Loading

KnVerey Jan 15, 2018

KnVerey Jan 15, 2018

KnVerey Jan 15, 2018

KnVerey Jan 15, 2018

KnVerey Jan 15, 2018

KnVerey Jan 15, 2018

Fattouche Jan 15, 2018

KnVerey Jan 15, 2018

Fattouche Jan 16, 2018

KnVerey Jan 15, 2018

KnVerey Jan 15, 2018

Warn about kubernetes minimum version #237

Warn about kubernetes minimum version #237

Conversation

Fattouche commented Jan 12, 2018

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

Fattouche Jan 15, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fattouche Jan 15, 2018 •

edited

Loading