Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable per-resource timeout customization via an annotation #232

Merged
merged 4 commits into from
Jan 9, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ This repo also includes related tools for [running tasks](#kubernetes-run) and [
* [Installation](#installation)
* [Usage](#usage)
* [Using templates and variables](#using-templates-and-variables)
* [Customizing behaviour with annotations](#customizing-behaviour-with-annotations)
* [Running tasks at the beginning of a deploy](#running-tasks-at-the-beginning-of-a-deploy)
* [Deploying Kubernetes secrets (from EJSON)](#deploying-kubernetes-secrets-from-ejson)

Expand Down Expand Up @@ -120,6 +121,9 @@ All templates must be YAML formatted. You can also use ERB. The following local
You can add additional variables using the `--bindings=BINDINGS` option. For example, `kubernetes-deploy my-app cluster1 --bindings=color=blue,size=large` will expose `color` and `size` in your templates.


### Customizing behaviour with annotations

- `kubernetes-deploy.shopify.io/timeout-override` (any resource): Override the tool's hard timeout for one specific resource. Both full ISO8601 durations and the time portion of ISO8601 durations are valid. Value must be between 1 second and 24 hours. Example values: 45s / 3m / 1h / PT0.25H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding: "This feature does not work as expected for the deployment resources because progressDeadlineSeconds takes precedence over hard timeouts and have a server side default value for deployments."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with more details on compatibility


### Running tasks at the beginning of a deploy

Expand Down
2 changes: 2 additions & 0 deletions lib/kubernetes-deploy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require 'active_support/core_ext/string/strip'
require 'active_support/core_ext/hash/keys'
require 'active_support/core_ext/array/conversions'
require 'active_support/duration'
require 'colorized_string'

require 'kubernetes-deploy/version'
Expand All @@ -17,6 +18,7 @@
require 'kubernetes-deploy/statsd'
require 'kubernetes-deploy/concurrency'
require 'kubernetes-deploy/bindings_parser'
require 'kubernetes-deploy/duration_parser'

module KubernetesDeploy
KubernetesDeploy::StatsD.build
Expand Down
39 changes: 39 additions & 0 deletions lib/kubernetes-deploy/duration_parser.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

module KubernetesDeploy
##
# This class is a less strict extension of ActiveSupport::Duration::ISO8601Parser.
# In addition to full ISO8601 durations, it can parse unprefixed ISO8601 time components (e.g. '1H').
# It is also case-insensitive.
# For example, this class considers the values "1H", "1h" and "PT1H" to be valid and equivalent.

class DurationParser
class ParsingError < ArgumentError; end

def initialize(value)
@iso8601_str = value.to_s.strip.upcase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe if you add:

@iso8601_str = "PT" + @iso8601_str unless @iso8601_str.start_with?("P")

You get all of the features you want from just normal ActiveSupport::Duration.parse(@iso8601_str) without needing to reach into the parser or write calculate_total_seconds.

The only issue is that the error message on a failed parse becomes Invalid ISO 8601 duration: "PTFOO", which is not really what the user provided.

You could maybe solve this with a sketchy Find&Replace on the returned message... maybe

Or you could do a more complicated thing where you do:

  begin
    ActiveSupport::Duration.parse(@iso8601_str)
  rescue ActiveSupport::Duration::ISO8601Parser::ParsingError => e
    begin
      ActiveSupport::Duration.parse("PT" + @iso8601_str)
    rescue ActiveSupport::Duration::ISO8601Parser::ParsingError
      raise ParsingError, e.message
    end
  end

Where you do the blind parse, and if it fails you always try jamming PT on the front.
If that succeeds, we return that one, and if it also fails, we return the first failure and discard the second (We raise the e.message, which is the first exception, in the second rescue block and don't even capture the second exception)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the retry-and-append solution! The only thing that bugs me about it is that we'll be retying in what I'm expecting to be the average case. I can't imagine that's meaningfully expensive, but for the purposes of reflecting intent, I'd slightly prefer to have the first try use the PT + version.

end

def parse!
if @iso8601_str.blank?
raise ParsingError, "Cannot parse blank value"
end

parser = ActiveSupport::Duration::ISO8601Parser.new(@iso8601_str)
parser.mode = :time unless @iso8601_str.start_with?("P")
parts = parser.parse!
ActiveSupport::Duration.new(calculate_total_seconds(parts), parts)
rescue ActiveSupport::Duration::ISO8601Parser::ParsingError => e
raise ParsingError, e.message
end

private

# https://github.com/rails/rails/blob/19c450d5d99275924254b2041b6ad470fdaa1f93/activesupport/lib/active_support/duration.rb#L79-L83
def calculate_total_seconds(parts)
parts.inject(0) do |total, (part, value)|
total + value * ActiveSupport::Duration::PARTS_IN_SECONDS[part]
end
end
end
end
43 changes: 38 additions & 5 deletions lib/kubernetes-deploy/kubernetes_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

module KubernetesDeploy
class KubernetesResource
attr_reader :name, :namespace, :context, :validation_error_msg
attr_reader :name, :namespace, :context
attr_writer :type, :deploy_started_at

TIMEOUT = 5.minutes
Expand All @@ -22,6 +22,8 @@ class KubernetesResource
If you have reason to believe it will succeed, retry the deploy to continue to monitor the rollout.
MSG

TIMEOUT_OVERRIDE_ANNOTATION = "kubernetes-deploy.shopify.io/timeout-override"

def self.build(namespace:, context:, definition:, logger:)
opts = { namespace: namespace, context: context, definition: definition, logger: logger }
if KubernetesDeploy.const_defined?(definition["kind"])
Expand All @@ -39,9 +41,17 @@ def self.timeout
end

def timeout
return timeout_override if timeout_override.present?
self.class.timeout
end

def timeout_override
return @timeout_override if defined?(@timeout_override)
@timeout_override = DurationParser.new(timeout_annotation).parse!.to_i
rescue DurationParser::ParsingError
@timeout_override = nil
end

def pretty_timeout_type
"timeout: #{timeout}s"
end
Expand All @@ -59,20 +69,26 @@ def initialize(namespace:, context:, definition:, logger:)
@logger = logger
@definition = definition
@statsd_report_done = false
@validation_error_msg = nil
@validation_errors = []
end

def validate_definition
@validation_error_msg = nil
@validation_errors = []
validate_timeout_annotation

command = ["create", "-f", file_path, "--dry-run", "--output=name"]
_, err, st = kubectl.run(*command, log_failure: false)
return true if st.success?
@validation_error_msg = err
@validation_errors << err
false
end

def validation_error_msg
@validation_errors.join("\n")
end

def validation_failed?
@validation_error_msg.present?
@validation_errors.present?
end

def id
Expand Down Expand Up @@ -277,6 +293,23 @@ def to_s

private

def validate_timeout_annotation
return if timeout_annotation.nil?

override = DurationParser.new(timeout_annotation).parse!
if override <= 0
@validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: Value must be greater than 0"
elsif override > 24.hours
@validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: Value must be less than 24h"
end
rescue DurationParser::ParsingError => e
@validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: #{e}"
end

def timeout_annotation
@definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION)
end

def file
@file ||= create_definition_tempfile
end
Expand Down
2 changes: 2 additions & 0 deletions test/fixtures/hello-cloud/unmanaged-pod.yml.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ apiVersion: v1
kind: Pod
metadata:
name: unmanaged-pod-<%= deployment_id %>
annotations:
kubernetes-deploy.shopify.io/timeout-override: "42s"
labels:
type: unmanaged-pod
name: unmanaged-pod-<%= deployment_id %>
Expand Down
1 change: 1 addition & 0 deletions test/integration/kubernetes_deploy_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def test_full_hello_cloud_set_deploy_succeeds

assert_logs_match_all([
"Deploying ConfigMap/hello-cloud-configmap-data (timeout: 30s)",
%r{Deploying Pod/unmanaged-pod-[-\w]+ \(timeout: 42s\)}, # annotation timeout override
"Hello from the command runner!", # unmanaged pod logs
"Result: SUCCESS",
"Successfully deployed 17 resources"
Expand Down
47 changes: 47 additions & 0 deletions test/unit/kubernetes-deploy/duration_parser_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# frozen_string_literal: true
require 'test_helper'

class DurationParserTest < KubernetesDeploy::TestCase
def test_parses_correct_iso_durations_with_prefixes
assert_equal 300, new_parser("PT300S").parse!
assert_equal 300, new_parser("PT5M").parse!
assert_equal 900, new_parser("PT0.25H").parse!
assert_equal 110839937, new_parser("P3Y6M4DT12H30M5S").parse!
end

def test_parses_correct_iso_durations_without_prefixes
assert_equal 300, new_parser("300S").parse!
assert_equal 300, new_parser("5M").parse!
assert_equal 900, new_parser("0.25H").parse!
assert_equal(-60, new_parser("-1M").parse!)
end

def test_parse_is_case_insensitive
assert_equal 30, new_parser("30S").parse!
assert_equal 30, new_parser("30s").parse!
end

def test_parse_raises_expected_error_for_blank_values
["", " ", nil].each do |blank_value|
assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, "Cannot parse blank value") do
new_parser(blank_value).parse!
end
end
end

def test_extra_whitespace_is_stripped_from_values
assert_equal 30, new_parser(" 30S ").parse!
end

def test_parse_raises_expected_error_when_value_is_invalid
assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, 'Invalid ISO 8601 duration: "FOO"') do
new_parser("foo").parse!
end
end

private

def new_parser(value)
KubernetesDeploy::DurationParser.new(value)
end
end
Loading