From d00a57085ed160c89f5d24a8140d0ca9b6a9e35b Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Wed, 6 Nov 2019 15:11:20 -0800 Subject: [PATCH] PR feedback and improvements --- lib/krane/deploy_task_config_validator.rb | 4 --- lib/krane/deprecated_deploy_task.rb | 2 +- lib/krane/global_deploy_task.rb | 10 +++--- test/helpers/fixture_deploy_helper.rb | 32 +++++++----------- test/integration-serial/serial_deploy_test.rb | 33 +++++++++++++++++-- test/integration/global_deploy_test.rb | 16 +++++++-- test/integration/krane_test.rb | 4 +++ test/unit/krane/resource_deployer_test.rb | 9 +++-- 8 files changed, 71 insertions(+), 39 deletions(-) diff --git a/lib/krane/deploy_task_config_validator.rb b/lib/krane/deploy_task_config_validator.rb index 05d0247bd..d6b7fd1a7 100644 --- a/lib/krane/deploy_task_config_validator.rb +++ b/lib/krane/deploy_task_config_validator.rb @@ -1,10 +1,6 @@ # frozen_string_literal: true -require 'krane/concerns/template_reporting' - module Krane class DeployTaskConfigValidator < TaskConfigValidator - include Krane::TemplateReporting - def initialize(protected_namespaces, allow_protected_ns, prune, *arguments) super(*arguments) @protected_namespaces = protected_namespaces diff --git a/lib/krane/deprecated_deploy_task.rb b/lib/krane/deprecated_deploy_task.rb index f44af8dd5..36b7c2348 100644 --- a/lib/krane/deprecated_deploy_task.rb +++ b/lib/krane/deprecated_deploy_task.rb @@ -301,6 +301,7 @@ def validate_configuration(allow_protected_ns:, prune:) measure_method(:validate_configuration) def validate_resources(resources) + validate_globals(resources) Krane::Concurrency.split_across_threads(resources) do |r| r.validate_definition(kubectl, selector: @selector) end @@ -320,7 +321,6 @@ def validate_resources(resources) end raise FatalDeploymentError, "Template validation failed" end - validate_globals(resources) end measure_method(:validate_resources) diff --git a/lib/krane/global_deploy_task.rb b/lib/krane/global_deploy_task.rb index 7279d6868..4cbd67101 100644 --- a/lib/krane/global_deploy_task.rb +++ b/lib/krane/global_deploy_task.rb @@ -33,10 +33,10 @@ class GlobalDeployTask # @param global_timeout [Integer] Timeout in seconds # @param selector [Hash] Selector(s) parsed by Krane::LabelSelector # @param template_paths [Array] An array of template paths - def initialize(context:, global_timeout: nil, selector: nil, filenames: []) + def initialize(context:, global_timeout: nil, selector: nil, filenames: [], logger: nil) template_paths = filenames.map { |path| File.expand_path(path) } - @task_config = TaskConfig.new(context, nil) + @task_config = TaskConfig.new(context, nil, logger) @template_sets = TemplateSets.from_dirs_and_files(paths: template_paths, logger: @task_config.logger) @global_timeout = global_timeout @@ -116,7 +116,7 @@ def validate_configuration errors = [] errors += task_config_validator.errors errors += @template_sets.validate - errors << "Selector is required" unless @selector.present? + errors << "Selector is required" unless @selector.to_h.present? unless errors.empty? add_para_from_list(logger: logger, action: "Configuration invalid", enum: errors) raise TaskConfigurationError @@ -159,7 +159,7 @@ def validate_globals(resources) namespaced_names = FormattedLogger.indent_four(namespaced_names.join("\n")) logger.summary.add_paragraph(ColorizedString.new("Namespaced resources:\n#{namespaced_names}").yellow) - raise FatalDeploymentError, "Deploying namespaced resource is not allowed from this command." + raise FatalDeploymentError, "This command cannot deploy namespaced resources" end def discover_resources @@ -186,7 +186,7 @@ def cluster_resource_discoverer end def statsd_tags - %W(context:#{@context}) + %W(context:#{context}) end def kubectl diff --git a/test/helpers/fixture_deploy_helper.rb b/test/helpers/fixture_deploy_helper.rb index e1daff69b..d475fb1f6 100644 --- a/test/helpers/fixture_deploy_helper.rb +++ b/test/helpers/fixture_deploy_helper.rb @@ -45,8 +45,8 @@ def deploy_fixtures(set, subset: nil, **args) # extra args are passed through to def deploy_global_fixtures(set, subset: nil, **args) fixtures = load_fixtures(set, subset) raise "Cannot deploy empty template set" if fixtures.empty? - args[:selector] = ["test=#{@namespace}", args[:selector]].compact.join(",") - destroyable = namespace_globals(fixtures) + args[:selector] ||= "test=#{@namespace}" + namespace_globals(fixtures) yield fixtures if block_given? @@ -56,8 +56,6 @@ def deploy_global_fixtures(set, subset: nil, **args) success = global_deploy_dirs_without_profiling(target_dir, **args) end success - ensure - delete_globals(destroyable) end def deploy_raw_fixtures(set, wait: true, bindings: {}, subset: nil, render_erb: false) @@ -113,12 +111,14 @@ def global_deploy_dirs_without_profiling(dirs, verify_result: true, prune: false filenames: Array(dirs), global_timeout: global_timeout, selector: Krane::LabelSelector.parse(selector), + logger: logger, ) - deploy.instance_eval("@task_config").instance_variable_set("@logger", logger) deploy.run( verify_result: verify_result, prune: prune ) + ensure + delete_globals(Array(dirs)) end # Deploys all fixtures in the given directories via KubernetesDeploy::DeployTask @@ -181,28 +181,20 @@ def build_kubectl(log_failure_by_default: true, timeout: '5s') end def namespace_globals(fixtures) - fixtures.each_with_object({}) do |(_, kinds_map), hash| - kinds_map.each do |kind, resources| - hash[kind] ||= [] + fixtures.each do |_, kinds_map| + kinds_map.each do |_, resources| resources.each do |resource| - resource["metadata"]["name"] += @namespace + resource["metadata"]["name"] = (resource["metadata"]["name"]+@namespace)[0..63] resource["metadata"]["labels"] ||= {} resource["metadata"]["labels"]["test"] = @namespace - hash[kind] << resource["metadata"]["name"] end end end end - def delete_globals(kinds_map) - kind_to_client = { "StorageClass" => storage_v1_kubeclient } - kinds_map&.each do |kind, names| - client = kind_to_client[kind] - raise "No known client for #{kind}" unless client - existing = client.send("get_#{kind.underscore.pluralize}").map { |r| r.dig(:metadata, :name) } - (names & existing).each do |name| - client.send("delete_#{kind.underscore}", name) - end - end + def delete_globals(dirs) + kubectl = build_kubectl + paths = dirs.flat_map { |d| ["-f", d] } + kubectl.run("delete", *paths, log_failure: false, use_namespace: false) end end diff --git a/test/integration-serial/serial_deploy_test.rb b/test/integration-serial/serial_deploy_test.rb index f8fa77419..dec54f1bd 100644 --- a/test/integration-serial/serial_deploy_test.rb +++ b/test/integration-serial/serial_deploy_test.rb @@ -291,6 +291,29 @@ def test_all_expected_statsd_metrics_emitted_with_essential_tags end end + def test_all_expected_statsd_metrics_emitted_global_deploy + metrics = capture_statsd_calls do + assert_deploy_success(deploy_global_fixtures('globals')) + end + + assert_equal(1, metrics.count { |m| m.type == :_e }, "Expected to find one event metric") + + %w( + KubernetesDeploy.validate_configuration.duration + KubernetesDeploy.discover_resources.duration + KubernetesDeploy.initial_status.duration + KubernetesDeploy.validate_resources.duration + KubernetesDeploy.apply_all.duration + KubernetesDeploy.normal_resources.duration + KubernetesDeploy.sync.duration + KubernetesDeploy.all_resources.duration + ).each do |expected_metric| + metric = metrics.find { |m| m.name == expected_metric } + refute_nil metric, "Metric #{expected_metric} not emitted" + assert_includes metric.tags, "context:#{KubeclientHelper::TEST_CONTEXT}", "#{metric.name} is missing context tag" + end + end + def test_cr_deploys_without_rollout_conditions_when_none_present_deprecated assert_deploy_success(deploy_fixtures("crd", subset: %w(widgets_deprecated.yml))) assert_deploy_success(deploy_fixtures("crd", subset: %w(widgets_cr.yml))) @@ -503,6 +526,9 @@ def test_apply_failure_with_sensitive_resources_hides_template_content refute_logs_match("kind: Deployment") # content of the sensitive template end + # test_global_deploy_black_box_failure is in test/integration/krane_test.rb + # because it does not modify global state. The following two tests modify + # global state and must be run in serially def test_global_deploy_black_box_success setup_template_dir("globals") do |target_dir| flags = "-f #{target_dir} --selector app=krane" @@ -512,7 +538,7 @@ def test_global_deploy_black_box_success assert_predicate(status, :success?) end ensure - storage_v1_kubeclient.delete_storage_class("testing-storage-class") + build_kubectl.run("delete", "-f", fixture_path("globals"), use_namespace: false, log_failure: false) end def test_global_deploy_black_box_timeout @@ -525,11 +551,12 @@ def test_global_deploy_black_box_timeout assert_equal(status.exitstatus, 70) end ensure - storage_v1_kubeclient.delete_storage_class("testing-storage-class") + build_kubectl.run("delete", "-f", fixture_path("globals"), use_namespace: false, log_failure: false) end def test_global_deploy_validation_catches_namespaced_cr assert_deploy_success(global_deploy_dirs_without_profiling('test/fixtures/crd/mail.yml', selector: "app=krane")) + reset_logger assert_deploy_failure(global_deploy_dirs_without_profiling('test/fixtures/crd/mail_cr.yml', selector: "app=krane")) assert_logs_match_all([ "Phase 1: Initializing deploy", @@ -538,7 +565,7 @@ def test_global_deploy_validation_catches_namespaced_cr "Discovering resources:", "- Mail/my-first-mail", "Result: FAILURE", - "Deploying namespaced resource is not allowed from this command.", + "This command cannot deploy namespaced resources", "Namespaced resources:", "my-first-mail (Mail)", ]) diff --git a/test/integration/global_deploy_test.rb b/test/integration/global_deploy_test.rb index 6f9f5f7c4..c2d4d57e8 100644 --- a/test/integration/global_deploy_test.rb +++ b/test/integration/global_deploy_test.rb @@ -2,7 +2,6 @@ require 'integration_test_helper' class GlobalDeployTest < Krane::IntegrationTest - include StatsDHelper def test_global_deploy_task_success assert_deploy_success(deploy_global_fixtures('globals')) @@ -67,12 +66,23 @@ def test_global_deploy_task_success_verify_false ]) end + def test_global_deploy_task_empty_selector_validation_failure + assert_deploy_failure(deploy_global_fixtures('globals', selector: "")) + assert_logs_match_all([ + "Phase 1: Initializing deploy", + "Result: FAILURE", + "Configuration invalid", + "- Selector is required", + ]) + end + def test_global_deploy_task_success_selector - assert_deploy_success(deploy_global_fixtures('globals', selector: "app=krane")) + selector = "app=krane" + assert_deploy_success(deploy_global_fixtures('globals', selector: selector)) assert_logs_match_all([ "Phase 1: Initializing deploy", - "Using resource selector test=", + "Using resource selector #{selector}", "All required parameters and files are present", "Discovering resources:", " - StorageClass/testing-storage-class", diff --git a/test/integration/krane_test.rb b/test/integration/krane_test.rb index b3296f3a0..eaf87136a 100644 --- a/test/integration/krane_test.rb +++ b/test/integration/krane_test.rb @@ -87,6 +87,10 @@ def test_deploy_black_box_timeout end end + # test_global_deploy_black_box_success and test_global_deploy_black_box_timeout + # are in test/integration-serial/serial_deploy_test.rb because they modify + # global state + def test_global_deploy_black_box_failure setup_template_dir("resource-quota") do |target_dir| flags = "-f #{target_dir} --selector app=krane" diff --git a/test/unit/krane/resource_deployer_test.rb b/test/unit/krane/resource_deployer_test.rb index b56960b5f..98b6d2854 100644 --- a/test/unit/krane/resource_deployer_test.rb +++ b/test/unit/krane/resource_deployer_test.rb @@ -48,7 +48,7 @@ def test_deploy_verify_false_no_timeout def test_deploy_failure_error resource = build_mock_resource(final_status: "failure") watcher = mock("ResourceWatcher") - watcher.expects(:run).returns(true) + watcher.expects(:run) Krane::ResourceWatcher.expects(:new).returns(watcher) assert_raises(Krane::FatalDeploymentError) do resource_deployer.deploy!([resource], true, false) @@ -67,9 +67,12 @@ def test_predeploy_priority_resources_respects_pre_deploy_list resource = build_mock_resource watcher = mock("ResourceWatcher") watcher.expects(:run).returns(true) + # ResourceDeployer only creates a ResourceWatcher if one or more resources + # are deployed. See test_predeploy_priority_resources_respects_empty_pre_deploy_list + # for counter example Krane::ResourceWatcher.expects(:new).returns(watcher) priority_list = [kind] - resource_deployer.predeploy_priority_resources([resource], priority_list) + resource_deployer(kubectl_times: 0).predeploy_priority_resources([resource], priority_list) end def test_predeploy_priority_resources_respects_empty_pre_deploy_list @@ -87,7 +90,7 @@ def resource_deployer(kubectl_times: 2, prune_whitelist: []) end @deployer = Krane::ResourceDeployer.new(current_sha: 'test-sha', statsd_tags: [], task_config: task_config, prune_whitelist: prune_whitelist, - max_watch_seconds: 60, selector: nil) + max_watch_seconds: 1, selector: nil) end def build_mock_resource(final_status: "success", hits_to_complete: 0, name: "web-pod")