-
Notifications
You must be signed in to change notification settings - Fork 115
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
Krane global deploy #574
Krane global deploy #574
Conversation
@logger = logger | ||
delegate :namespace, :context, :logger, to: :@task_config | ||
|
||
def initialize(task_config:, namespace_tags: []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a pass at using task_config
this can be reverted if too much is going on in the pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about doing this the other way around: moving all of the stuff in this class into TaskConfig? Everything in here is effectively collected global context about the task's execution environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a reasonable change for a follow-up PR. My original concern was that TaskConfig should actually be the holder of static information, but I don't actually think there would be issues with it having dynamic abilities.
addc809
to
c36ebcc
Compare
Neither needs to be in the initial PR, but we should chat more about whether they're required for the release. I was actually thinking that label selector might be mandatory for a sane global command, given that you're pretty much guaranteed to not be the only actor modifying the global namespace. As for pruning, I'm not sure whether it should be the default, but our value prop definitely goes down without it, having it off by default would be inconsistent, and making it the default would be a horrifying breaking change if it isn't that way to start. One thing I'm sure about is that if we do have pruning, using a label selector with it should be mandatory.
Won't that be impossible once we 🔥 the kubernetes-deploy version, since the krane version should not be capable at all of deploying globals? |
selector I believe is trivial to add back, so I'll do that here. Pruning is not, so that's going to need to be a second PR either way. I totally agree that pruning requires selector. I think requiring the selector is reasonable. I'll keep thinking about pruning default.
I did not explain what I was thinking very well at all:
|
I think it could work if you make |
1bbc1cb
to
bd32085
Compare
de4c4a1
to
474fdea
Compare
aab7866
to
ca6b65f
Compare
|
||
deploy.run!( | ||
verify_result: options["verify-result"], | ||
prune: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will get flipped to true in a follow-up PR #589
8545bcc
to
ed87492
Compare
e47e556
to
6f4e178
Compare
6f4e178
to
1e501bf
Compare
max_watch_seconds: nil, selector: nil, template_paths: [], template_dir: nil, protected_namespaces: nil, | ||
render_erb: true, allow_globals: false) | ||
template_dir = File.expand_path(template_dir) if template_dir | ||
template_paths = (template_paths.map { |path| File.expand_path(path) } << template_dir).compact | ||
|
||
@logger = logger || KubernetesDeploy::FormattedLogger.build(namespace, context) | ||
@template_sets = TemplateSets.from_dirs_and_files(paths: template_paths, logger: @logger) | ||
@task_config = KubernetesDeploy::TaskConfig.new(context, namespace, @logger) | ||
@task_config = KubernetesDeploy::TaskConfig.new(context, namespace, @logger, allow_globals && namespace.blank?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allow_globals && namespace.blank?
is the result of having to support the legacy behavior of kubernetes-deploy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also make https://github.com/Shopify/kubernetes-deploy/pull/574/files#diff-4c33bbb177fdf882c41d892f3bde9570R144 be allow_globals && namespace.blank?
I'm not very sure about the @allow_globals
and @global_mode
in TaskConfig.
But, this PR looks good to me 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allow_globals
at this point is only used to determine if we need to display a warning when you use exe/kubernetes-deploy
with a global resource. We should be able to kill it when we delete exe/kubernetes-deploy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me 👍
Can we wait for #585 before shipping this one? I will help with rebase when the former is merged 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still feel strongly that DeployTask
should not be used as though it is an abstract superclass.
Do you have an issue for actually supporting (via proper resource classes) some of the most common global resource types as part of the launch of this new feature? If we don't have any, the verify result option isn't really doing its work.
@@ -122,15 +122,15 @@ def server_version | |||
# @param protected_namespaces [Array<String>] Array of protected Kubernetes namespaces (defaults | |||
# to KubernetesDeploy::DeployTask::PROTECTED_NAMESPACES) | |||
# @param render_erb [Boolean] Enable ERB rendering | |||
def initialize(namespace:, context:, current_sha:, logger: nil, kubectl_instance: nil, bindings: {}, | |||
def initialize(namespace: nil, context:, current_sha:, logger: nil, kubectl_instance: nil, bindings: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a big smell to me that a bunch of these arguments (bindings, namespace, protected_namespaces) aren't/shouldn't be supported for GlobalDeployTask
, and the allow_globals
is always true for one subclass and always false for the other (once we do the rename). It's true that this PR achieves adding the new task with a minimal amount of code. But by doing this way, you're exchanging diff minimization for cognitive overhead for future users (who need to know these oddities about the arguments) and maintainers (who need to simultaneously consider the two types of possible work being executed when changing this task). In your earlier comments you mentioned you planned to create an abstract superclass for the deploy tasks. Did that not work out?
@@ -161,14 +161,17 @@ def run(*args) | |||
# @param prune [Boolean] Enable deletion of resources that do not appear in the template dir | |||
# | |||
# @return [nil] | |||
def run!(verify_result: true, allow_protected_ns: false, prune: true) | |||
def run!(verify_result: true, allow_protected_ns: false, prune: true, | |||
task_config_validator: DeployTaskConfigValidator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validator selection is an internal detail of the class and should not be configurable by users. This is another symptom of not implementing the abstract superclass (or two separate tasks that leverage common components, or some other option that gives us separation in the code).
end | ||
global_names = FormattedLogger.indent_four(global_names.join("\n")) | ||
|
||
if allow_globals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially since the big rename PR looks so imminent, I'd suggest waiting to merge the globals command into a version that doesn't need to support the legacy behaviour. If you're using a version where we're giving you a deploy command, you should absolutely not be using the regular deploy command to ship globals.
def initialize(protected_namespaces, allow_protected_ns, prune, *arguments) | ||
super(*arguments, skip: [:validate_namespace_exists]) | ||
@protected_namespaces = protected_namespaces | ||
@allow_protected_ns = allow_protected_ns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two vars don't make any sense for the global command
# frozen_string_literal: true | ||
module KubernetesDeploy | ||
class GlobalDeployTaskConfigValidator < TaskConfigValidator | ||
def initialize(protected_namespaces, allow_protected_ns, prune, *arguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also validate that the selector is present, or do we get that for free? Specifically, what happens if you pass in an empty string selector, either via CLI or via ruby? It wasn't required on the regular command, so we might not have cared to check.
@@ -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?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 It surprises me, but is true, that events can be both namespaced and not. Are we actually formulating a query that works here? Just removing the namespace arg doesn't seem to make kubectl fetch the non-namespaced version:
kubectl get events -v=9
GET https://kubernetes.docker.internal:6443/api/v1/namespaces/default/events?limit=500 200 OK in 19 milliseconds
@@ -53,7 +53,7 @@ def fetch_by_kind(kind) | |||
resource_class = KubernetesResource.class_for_kind(kind) | |||
output_is_sensitive = resource_class.nil? ? false : resource_class::SENSITIVE_TEMPLATE_CONTENT | |||
raw_json, _, st = @kubectl.run("get", kind, "--chunk-size=0", attempts: 5, output: "json", | |||
output_is_sensitive: output_is_sensitive) | |||
output_is_sensitive: output_is_sensitive, use_namespace: !global_mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessarily an on/off mode for the cache. It needs to be on a per-resource basis. To give a concrete example, Dale's PR needs to fetch nodes for the purposes of validating daemonSet deploys.
|
||
def initialize(context, namespace, logger = nil) | ||
def initialize(context, namespace, logger = nil, global_mode = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "global mode" anything beyond the absence of the namespace?
@task_config = task_config | ||
@kubectl = kubectl | ||
@kubeclient_builder = kubeclient_builder | ||
@errors = nil | ||
@validations = only || DEFAULT_VALIDATIONS | ||
@validations = (only || DEFAULT_VALIDATIONS) - skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kinda surprised to see that in addition to subclassing, we also need the ability to modify the defaults with both only
and skip
options. Am I reading correctly that the only
option is just used by tests? Could this work:
class TaskConfigValidator
def validations
DEFAULT_VALIDATIONS
end
end
class SomeValidator < TaskConfigValidator
def validations
DEFAULT_VALIDATIONS - [:validation_to_skip] # or + something, or just a specific list
end
end
Then the superclass isn't messing around with constructing the list, when it should in practice be a static property of each subclass.
|
||
unless st.success? | ||
if st.success? | ||
@task_config.namespace_definition = JSON.parse(definition, symbolize_names: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't intuitive that validate_namespace_exists
would have the side-effect of setting a property on an entirely different object. Could we make task_config
capable of caching/returning the namespace object, and raising errors we can catch here?
Closing for #602 |
What are you trying to accomplish with this PR?
Part of the krane 1.0 work is refusing to allow
krane deploy
to deploy global resources. See #522 for more of an explanation.To allow our users to have a better experience than just
kubectl apply
we're adding a new commandkrane global-deploy
. This command will not support erb rendering, for that you can pipe the results ofkrane render
. This version does not support pruning or--label-selector
thought we would accept the feature in future versions.How is this accomplished?
Add a krane global-deploy command and a krane/global_deploy_task. The global_deploy_task is a thin wrapper around `kubernetes_deploy/deploy_task.
What could go wrong?
This assumes the approach we want is to share deploy_task with the namespaced version and only override the parts we want.
We could chop up the deploy task and then make the deploy_task and global_deploy_task be more fully featured.