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 1 commit
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-seconds` (any resource): Override the tool's hard timeout for one specific resource. Value must be a positive number of seconds (digits only).

### Running tasks at the beginning of a deploy

Expand Down
29 changes: 24 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-seconds"
Copy link
Contributor

@devonboyer devonboyer Dec 22, 2017

Choose a reason for hiding this comment

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

Would it be possible to support more ways to specify the duration like 10s or 5m? Or perhaps just make the annotation kubernetes-deploy.shopify.io/timeout-override and state that for the time being the only acceptable value is seconds?

I am bringing this up because a timeout of 15m (which may not be unreasonable for Redis) would be 900s which isn't as easy for a human to parse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. I'll update it to accept "s" or "m", and to assume that unqualified values are seconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well if you're going to have m and s, you might as well have h...

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend ISO8601 format (to be used with ActiveSupport::Duration::ISO8601Parse) for time durations. It's what I used in my open PR on resource discovery.


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,14 @@ def self.timeout
end

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

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

def pretty_timeout_type
"timeout: #{timeout}s"
end
Expand All @@ -59,20 +66,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_annotations

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 +290,12 @@ def to_s

private

def validate_annotations
return unless timeout_override.present?
return if timeout_override.strip.match(/\A\d+\z/) && timeout_override.to_i > 0
@validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation must contain digits only and must be > 0"
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-seconds: "42"
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
88 changes: 86 additions & 2 deletions test/unit/kubernetes-deploy/kubernetes_resource_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@

class KubernetesResourceTest < KubernetesDeploy::TestCase
class DummyResource < KubernetesDeploy::KubernetesResource
def initialize(*)
definition = { "kind" => "DummyResource", "metadata" => { "name" => "test" } }
def initialize(definition_extras: {})
definition = { "kind" => "DummyResource", "metadata" => { "name" => "test" } }.merge(definition_extras)
super(namespace: 'test', context: 'test', definition: definition, logger: @logger)
end

def exists?
true
end

def file_path
"/tmp/foo/bar"
end
end

def test_service_and_deployment_timeouts_are_equal
Expand Down Expand Up @@ -53,8 +57,88 @@ def test_fetch_events_returns_empty_hash_when_kubectl_results_empty
assert_operator events, :empty?
end

def test_can_override_hardcoded_timeout_via_an_annotation
basic_resource = DummyResource.new
assert_equal 5.minutes, basic_resource.timeout

customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60"))
assert_equal 60, customized_resource.timeout

customized_resource = DummyResource.new(definition_extras: build_timeout_metadata(" 60 "))
assert_equal 60, customized_resource.timeout
end

def test_blank_timeout_annotation_is_ignored
stub_kubectl_response("create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything, resp: "{}")

customized_resource = DummyResource.new(definition_extras: build_timeout_metadata(""))
customized_resource.validate_definition
refute customized_resource.validation_failed?, "Blank annotation with was invalid"
assert_equal 5.minutes, customized_resource.timeout
end

def test_validation_of_timeout_annotation
expected_cmd = ["create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything]
error_msg = "kubernetes-deploy.shopify.io/timeout-override-seconds annotation " \
"must contain digits only and must be > 0"

stub_kubectl_response(*expected_cmd, resp: "{}")
customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("sixty"))
customized_resource.validate_definition
assert customized_resource.validation_failed?, "Annotation with 'sixty' was valid"
assert_equal error_msg, customized_resource.validation_error_msg

stub_kubectl_response(*expected_cmd, resp: "{}")
customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("-1"))
customized_resource.validate_definition
assert customized_resource.validation_failed?, "Annotation with '-1' was valid"
assert_equal error_msg, customized_resource.validation_error_msg

stub_kubectl_response(*expected_cmd, resp: "{}")
customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("0"))
customized_resource.validate_definition
assert customized_resource.validation_failed?, "Annotation with '0' was valid"
Copy link
Contributor

Choose a reason for hiding this comment

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

This case is not Annotation with '0'

assert_equal error_msg, customized_resource.validation_error_msg

stub_kubectl_response(*expected_cmd, resp: "{}")
customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("10m"))
customized_resource.validate_definition
assert customized_resource.validation_failed?, "Annotation with '10m' was valid"
assert_equal error_msg, customized_resource.validation_error_msg
end

def test_annotation_and_kubectl_error_messages_are_combined
stub_kubectl_response(
"create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything,
resp: "{}",
err: "Error from kubectl: Something else in this template was not valid",
success: false
)

customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("bad"))
customized_resource.validate_definition
assert customized_resource.validation_failed?, "Expected resource to be invalid"

expected = <<~STRING.strip
kubernetes-deploy.shopify.io/timeout-override-seconds annotation must contain digits only and must be > 0
Error from kubectl: Something else in this template was not valid
STRING
assert_equal expected, customized_resource.validation_error_msg
end

private

def build_timeout_metadata(value)
{
"metadata" => {
"name" => "customized",
"annotations" => {
KubernetesDeploy::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION => value
}
}
}
end

def assert_includes_dummy_events(events, first:, second:)
unless first || second
assert_operator events, :empty?
Expand Down