PR feedback and improvements

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

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)
 

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<String>] 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

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

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)",
     ])

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",

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"

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")