Deploy method override annotation #753
Conversation
| @@ -95,11 +95,10 @@ def deploy_resources(resources, prune: false, verify:, record_summary: true) | |||
| applyables, individuals = resources.partition { |r| r.deploy_method == :apply } | |||
| # Prunable resources should also applied so that they can be pruned | |||
| pruneable_types = @prune_whitelist.map { |t| t.split("/").last } | |||
| applyables += individuals.select { |r| pruneable_types.include?(r.type) } | |||
| applyables += individuals.select { |r| pruneable_types.include?(r.type) && !r.deploy_method_override } | |||
There was a problem hiding this comment.
We need to fight against default behaviour, which makes me loath to ever merge this into master. Previously, we would run apply here to enforce pruning, but since the goal right now is to avoid pruning, we need to add this workaround.
The good news is that this only affects resources that are purposefully overridden, but I think it's enough of a landmine to give me some pause
There was a problem hiding this comment.
Would It be helpful to ever see a log warning when a pruneable resource is being overridden? I think if it's purposely overridden, then they know they're fighting the default. but wondering if it would give less pause and help to debug if that knowledge assumption is wrong.
| @@ -35,6 +35,8 @@ class KubernetesResource | |||
| If you have reason to believe it will succeed, retry the deploy to continue to monitor the rollout. | |||
| MSG | |||
|
|
|||
| ALLOWED_DEPLOY_METHOD_OVERRIDES = %w(create replace replace-force) | |||
There was a problem hiding this comment.
I needed to get rid of apply here because
- It's the default method, anyway
- https://github.com/Shopify/krane/blob/master/lib/krane/resource_deployer.rb#L98: If you try to override the deploy method of a prunable type, it will still get applied, regardless of whichever override method you choose.
Note that the latter point means overriding will prevent default pruning behaviour for the resource(s) whose deploy methods are being overridden. This isn't bad, in and of itself, but it's worth pointing out.
There was a problem hiding this comment.
Note that the latter point means overriding will prevent default pruning behaviour for the resource(s) whose deploy methods are being overridden. This isn't bad, in and of itself, but it's worth pointing out.
Worth making a note about this in a readme somewhere or nah?
timothysmith0609
requested review from
RyanBrushett
and removed request for
manuelfelipe
| @@ -95,11 +95,10 @@ def deploy_resources(resources, prune: false, verify:, record_summary: true) | |||
| applyables, individuals = resources.partition { |r| r.deploy_method == :apply } | |||
| # Prunable resources should also applied so that they can be pruned | |||
| pruneable_types = @prune_whitelist.map { |t| t.split("/").last } | |||
| applyables += individuals.select { |r| pruneable_types.include?(r.type) } | |||
| applyables += individuals.select { |r| pruneable_types.include?(r.type) && !r.deploy_method_override } | |||
There was a problem hiding this comment.
Would It be helpful to ever see a log warning when a pruneable resource is being overridden? I think if it's purposely overridden, then they know they're fighting the default. but wondering if it would give less pause and help to debug if that knowledge assumption is wrong.
| @@ -35,6 +35,8 @@ class KubernetesResource | |||
| If you have reason to believe it will succeed, retry the deploy to continue to monitor the rollout. | |||
| MSG | |||
|
|
|||
| ALLOWED_DEPLOY_METHOD_OVERRIDES = %w(create replace replace-force) | |||
There was a problem hiding this comment.
Note that the latter point means overriding will prevent default pruning behaviour for the resource(s) whose deploy methods are being overridden. This isn't bad, in and of itself, but it's worth pointing out.
Worth making a note about this in a readme somewhere or nah?
timothysmith0609
force-pushed
the
deploy_method_annotation_alpha
branch
from
963afcc
to
e1fe236
Compare
Added a note to the changelog, which should be sufficient until we decide whether to keep/toss this feature longterm |
|
Thanks Tim. ⛵ |
|
Code looks reasonable and on a positive note this will get resolved with server side apply (kubernetes/kubectl#712). I definitely think we need to leave a warning somewhere (possibly in the readme / changelog) that this disables pruning. A different approach would be to automatically detect large objects instead of using an annotation, that would make it clear this is a workaround for large objects not an generic annotation to chagne the deploy method. |
Concerns https://github.com/Shopify/shopify-core-redis-proxy/pull/170
What are you trying to accomplish with this PR?
We are reaching the
annotationsize limit for certainConfigMapresources. Note, this is not the 1MiB limit of theConfigMapdata, itself, but simply the annotation. One possible strategy to get around this is to preclude the use of thelast-applied-annotation(which is causing the bloat in size). To do this, we need to stop usingapplywhen deploying this resource and instead usereplace.To that end, this PR introduces an annotation-driven feature,
krane.shopify.io/alpha/deploy-method-override. This override accepts one ofapply, create, replace, replace-forceand is used to control the deploy method at the individual resource level. I chose this approach because: