Global deploy 2 #602

dturn · 2019-10-22T16:50:04Z

What are you trying to accomplish with this PR?
Second attempt to build a global deploy task.

How is this accomplished?
Three major differences from #574.

The global_deploy_task.rb doesn't inherit from deploy_task.rb.
I created a concern(module) for the code that should be shared. There is still duplicate code, but what's duplicated is either commonly duplicated in the codebase (kubectl) or small snippets that might look different in the future without causing harm to the other task.
New class introduced: ResourceDeployer.

ToDo

Write more tests
Rebsae after the rename PR merged

What could go wrong?
This doesn't support pruning, that will come in a follow-up PR.

Something subtle and untested breaks (or changes) behavior in deploy_test.rb due to it being in the new concern or class.

lib/kubernetes-deploy/deploy_task.rb

dturn · 2019-10-23T17:47:00Z

lib/kubernetes-deploy/resource_deployer.rb

+    include Krane::TemplateReporting
+
+    delegate :logger, to: :@task_config
+    attr_reader :statsd_tags


Needed by KubernetesDeploy::StatsD::MeasureMethods

dturn · 2019-10-23T20:26:19Z

lib/kubernetes-deploy/kubernetes_resource.rb

@@ -317,7 +317,7 @@ def debug_message(cause = nil, info_hash = {})
    def fetch_events(kubectl)
      return {} unless exists?
      out, _err, st = kubectl.run("get", "events", "--output=go-template=#{Event.go_template_for(type, name)}",
-        log_failure: false)
+        log_failure: false, use_namespace: !global?)


kubectl api-resouces indicates that events are NAMESPACED. Running kubectl get events returns events about Nodes which are also global, so I believe this is the correct behavior.

KnVerey

I haven't had a look at the tests yet, but I like where this is going 👌

lib/krane/cli/global_deploy_command.rb

lib/krane/cli/krane.rb

lib/krane/global_deploy_task.rb

KnVerey · 2019-10-25T22:57:57Z

lib/kubernetes-deploy/kubernetes_resource.rb

@@ -40,7 +40,7 @@ class KubernetesResource
    SERVER_DRY_RUNNABLE = false

    class << self
-      def build(namespace:, context:, definition:, logger:, statsd_tags:, crd: nil, global_names: [])
+      def build(namespace: nil, context:, definition:, logger:, statsd_tags:, crd: nil, global_names: [])


If we put the ClusterResourceDisovery stuff in the TaskConfig, this could have a much smaller signature, without namespace being oddly optional: build(task_config:, definition:, statsd_tags:)

Yeah, we've got a tracking issue for making build take a task_config.

lib/kubernetes-deploy/resource_cache.rb

lib/kubernetes-deploy/resource_deployer.rb

KnVerey · 2019-10-25T23:13:14Z

lib/kubernetes-deploy/resource_deployer.rb

+        end
+
+        output_is_sensitive = resources.any?(&:sensitive_template_content?)
+        global_mode = resources.all?(&:global?)


So in the mixed-mode scenario currently possible under the old deploy task, we pass a namespace arg that is just ignored by kubectl as appropriate? I guess that's what we've always done, and it works. 🤔

lib/krane/global_deploy_task.rb

lib/krane/deploy_task_config_validator.rb

lib/krane/global_deploy_task.rb

KnVerey · 2019-10-30T23:23:33Z

lib/krane/global_deploy_task.rb

+
+    def check_initial_status(resources)
+      cache = Krane::ResourceCache.new(@task_config)
+      Krane::Concurrency.split_across_threads(resources) { |r| r.sync(cache) }


We'll want to prewarm here when Ryan's PR merges.

lib/krane/task_config.rb

test/integration-serial/serial_deploy_test.rb

test/unit/resource_cache_test.rb

test/exe/global_deploy_test.rb

test/unit/krane/resource_deployer_test.rb

dturn · 2019-11-04T19:35:53Z

Pruning draft PR #612

RyanBrushett

Couple questions but looks pretty good.

lib/krane/cluster_resource_discovery.rb

lib/krane/global_deploy_task.rb

test/fixtures/globals/storage_classes.yml

test/helpers/fixture_deploy_helper.rb

test/unit/krane/resource_deployer_test.rb

timothysmith0609 · 2019-11-06T18:05:06Z

One more note is to fix the linter error being thrown in CI

KnVerey

Almost there!

lib/krane/deploy_task_config_validator.rb

lib/krane/deprecated_deploy_task.rb

KnVerey · 2019-11-06T17:12:49Z

lib/krane/task_config.rb

+    def global_kinds
+      @global_kinds ||= begin
+        cluster_resource_discoverer = ClusterResourceDiscovery.new(task_config: self)
+        cluster_resource_discoverer.global_resource_kinds


maybe you missed the question in my last pass: this seems to be the only place this method is called. Why not move the method body here and not expose it on two different classses?

This changes a in the pruning PR. I'd like to defer this question till that PR.

test/helpers/fixture_deploy_helper.rb

test/integration-serial/serial_deploy_test.rb

test/integration/global_deploy_test.rb

Use resource_deployer for normal deploys Add a common module Add tests

KnVerey · 2019-11-07T16:04:24Z

lib/krane/global_deploy_task.rb

      template_paths = filenames.map { |path| File.expand_path(path) }

-      @task_config = TaskConfig.new(context, nil)
+      @task_config = TaskConfig.new(context, nil, logger)


Let's default this like the old task does, so they don't have to pass one in.

KnVerey

Just this one last thing, then 🚢

d7de6cc#r343733769

KnVerey · 2019-11-07T16:11:47Z

test/integration/global_deploy_test.rb

+      %r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
+      "Don't know how to monitor resources of type StorageClass.",
+      %r{Assuming StorageClass\/testing-storage-class[\w-]+ deployed successfully.},
+      %r{Successfully deployed in 0.[\d]+s: StorageClass\/testing-storage-class},


Oh, this needs to be relaxed:

Successfully deployed in 0.[\d]+s: StorageClass/testing-storage-class)' not found in the following logs:
Because it took more than a second for whatever reason

KnVerey · 2019-11-07T16:12:05Z

test/integration/global_deploy_test.rb

+      %r{Deploying StorageClass\/testing-storage-class[\w-]+ \(timeout: 300s\)},
+      "Don't know how to monitor resources of type StorageClass.",
+      %r{Assuming StorageClass\/testing-storage-class[\w-]+ deployed successfully.},
+      %r{Successfully deployed in 0.[\d]+s: StorageClass\/testing-storage-class},


Same here I imagine

dturn force-pushed the global-deploy-2 branch 2 times, most recently from fb31d05 to e357af0 Compare October 22, 2019 18:56

dturn commented Oct 22, 2019

View reviewed changes

lib/kubernetes-deploy/deploy_task.rb Outdated Show resolved Hide resolved

dturn force-pushed the global-deploy-2 branch from e357af0 to 175b1f9 Compare October 22, 2019 23:29

dturn mentioned this pull request Oct 23, 2019

Krane global deploy #574

Closed

dturn commented Oct 23, 2019

View reviewed changes

dturn force-pushed the global-deploy-2 branch from 93b0379 to 25510ba Compare October 23, 2019 17:59

dturn marked this pull request as ready for review October 23, 2019 18:10

dturn requested review from KnVerey and a team October 23, 2019 18:11

dturn force-pushed the global-deploy-2 branch from 25510ba to 118b289 Compare October 23, 2019 20:24

dturn commented Oct 23, 2019

View reviewed changes

dturn force-pushed the global-deploy-2 branch 2 times, most recently from 90bced6 to ea2a014 Compare October 24, 2019 21:39

KnVerey reviewed Oct 25, 2019

View reviewed changes

dirceu mentioned this pull request Oct 28, 2019

Add first version of the migration guide to Krane 1.0 #607

Merged

dturn force-pushed the global-deploy-2 branch 7 times, most recently from 1000c75 to ae09dbc Compare October 29, 2019 17:38

dturn commented Oct 29, 2019

View reviewed changes

lib/krane/global_deploy_task.rb Outdated Show resolved Hide resolved

dturn force-pushed the global-deploy-2 branch 4 times, most recently from 60397c5 to 09c68db Compare October 29, 2019 18:48

dturn requested a review from KnVerey October 29, 2019 18:59

KnVerey reviewed Oct 31, 2019

View reviewed changes

dturn force-pushed the global-deploy-2 branch 2 times, most recently from 5994edb to 9467bc6 Compare November 2, 2019 20:50

dturn force-pushed the global-deploy-2 branch from cd36320 to f57de6d Compare November 4, 2019 22:45

RyanBrushett reviewed Nov 5, 2019

View reviewed changes

lib/krane/cluster_resource_discovery.rb Show resolved Hide resolved

lib/krane/global_deploy_task.rb Show resolved Hide resolved

timothysmith0609 reviewed Nov 6, 2019

View reviewed changes

timothysmith0609 approved these changes Nov 6, 2019

View reviewed changes

test/unit/krane/resource_deployer_test.rb Outdated Show resolved Hide resolved

test/unit/krane/resource_deployer_test.rb Outdated Show resolved Hide resolved

dturn force-pushed the global-deploy-2 branch 2 times, most recently from 62c4cdc to 3cc40d8 Compare November 6, 2019 18:11

KnVerey reviewed Nov 6, 2019

View reviewed changes

dturn force-pushed the global-deploy-2 branch from d00a570 to 4717631 Compare November 6, 2019 23:47

dturn added 7 commits November 6, 2019 16:34

Global command with a ResourceDeployer class

9f1509a

Use resource_deployer for normal deploys Add a common module Add tests

Move validations back into (Global)DeployTask

4f18c8c

Test updates

96d3af0

Add more global integration tests

f5df217

Fix class name

437976d

Add an extra test around namespaced CRs

57f3306

PR feedback and improvements

d7de6cc

dturn force-pushed the global-deploy-2 branch from 4717631 to 7b616f1 Compare November 7, 2019 00:40

Fix up tests from rebasing

fa17169

dturn force-pushed the global-deploy-2 branch from 7b616f1 to fa17169 Compare November 7, 2019 00:57

KnVerey reviewed Nov 7, 2019

View reviewed changes

KnVerey approved these changes Nov 7, 2019

View reviewed changes

KnVerey reviewed Nov 7, 2019

View reviewed changes

Update to task config defaulting

ca5dc27

dturn merged commit 42f0a97 into master Nov 7, 2019

dirceu mentioned this pull request Nov 11, 2019

Make task arguments match the new flag names #619

Merged

dturn mentioned this pull request Nov 12, 2019

Krane Global Deploy #588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global deploy 2 #602

Global deploy 2 #602

dturn commented Oct 22, 2019 •

edited

Loading

dturn Oct 23, 2019

dturn Oct 23, 2019

KnVerey left a comment

KnVerey Oct 25, 2019

dturn Oct 28, 2019

KnVerey Oct 25, 2019

KnVerey Oct 30, 2019

dturn commented Nov 4, 2019

RyanBrushett left a comment

timothysmith0609 commented Nov 6, 2019

KnVerey left a comment

KnVerey Nov 6, 2019

dturn Nov 7, 2019

KnVerey Nov 7, 2019

KnVerey left a comment

KnVerey Nov 7, 2019

KnVerey Nov 7, 2019

Global deploy 2 #602

Global deploy 2 #602

Conversation

dturn commented Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

dturn commented Nov 4, 2019

RyanBrushett left a comment

Choose a reason for hiding this comment

timothysmith0609 commented Nov 6, 2019

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

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

dturn commented Oct 22, 2019 •

edited

Loading