Proposal: Enable purging resources in `kubectl apply` #29551

lukemarsden · 2016-07-25T16:52:07Z

Proposal to resolve #19805

This change is

googlebot · 2016-07-25T16:52:08Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

lukemarsden · 2016-07-25T16:54:26Z

Re CLA issue: not sure how to add @errordeveloper to this PR. How do you add a person to a PR?

errordeveloper · 2016-07-25T17:06:46Z

This CLA check is a bit confusing..

errordeveloper · 2016-07-25T17:17:05Z

docs/proposals/kubectl-apply-purge-from-namespace.md

+kubectl apply --purge-from-namespace -f https://example.com/myapp-part1.json -f https://example.com/myapp-part2.json
+```
+
+If `myapp1.json` and `myapp2.json` are in the same namespaces and it is explicitly defined, removing either of the URLs from the invocations arguments will result in all the resources that URL has defined to be purged.


insert -part here, to reflect what's above

bgrant0607 · 2016-07-25T17:47:46Z

cc @ghodss @kubernetes/kubectl

bgrant0607 · 2016-07-25T17:48:44Z

@lukemarsden @errordeveloper Commits from multiple contributors will always fail the automated CLA check.

smarterclayton · 2016-07-25T18:53:30Z

Agree with general approach here - I think this is the simplest and most consistent way to solve this today.

smarterclayton · 2016-07-25T18:53:52Z

We would need to warn about using the generateName field inside of your applied files.

lukemarsden · 2016-07-26T10:24:14Z

Agree with general approach here - I think this is the simplest and most consistent way to solve this today.

Great, thanks!

We would need to warn about using the generateName field inside of your applied files.

Yes, but we think this is a general problem with apply? We could add a warning if applied files include generateName, however. I take it that your worry is that kubectl apply X twice, for X with generateName, would end up creating two copies of the resources therein. IOW, if generateName creates random names, it breaks the ability for a second instantiation of apply to refer to the resources created by a first.

See also: #1702 (comment)

philips · 2016-07-26T16:27:32Z

cc @ethernetdan @colhom

smarterclayton · 2016-07-27T18:07:51Z

It is a general problem, just want to make sure that a user is aware of it
in the edge case (we don't have to fix it here, just a TODO is ok).

On Tue, Jul 26, 2016 at 12:29 PM, Brandon Philips notifications@github.com
wrote:

oops, I meant to cc @colhom https://github.com/colhom. disregard
@aaronlevy https://github.com/aaronlevy and @derekchiang
https://github.com/derekchiang

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#29551 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7W9fdIXzOtIFFLcqWJS5PMw0p-tks5qZjXngaJpZM4JUUwZ
.

lukemarsden · 2016-08-17T11:47:47Z

@mikedanese @bgrant0607

I've simplified the proposal and updated it to reflect the discussion above. Please take another look: https://github.com/errordeveloper/kubernetes/blob/kubectl-apply-purge-from-namespace/docs/proposals/kubectl-apply-purge-missing-where.md

Thanks!

ghodss · 2016-08-17T16:53:55Z

This is very cool, thanks for the update! Couple questions:

How does this interact with namespaces? Maybe this was implicit but here's how I would imagine it:

If no namespace is specified, operate across default namespace only.
If --namespace is specified, only operate in that namespace.
If --all-namespaces is specified, operate across the entire cluster.

...with the label selector working as expected in all cases. I do think you could make a case that if you do --all-namespaces, adding a label selector should throw an error, since labels generally are designed to be scoped within namespaces. But I could be convinced either way on that.

If you just do --purge-missing-where "", do you then affect all objects? This makes sense to me, but I wonder if there should be a different option or change the option name for it to be more intuitive in both cases.

errordeveloper · 2016-08-18T08:58:43Z

@ghodss good question indeed, sounds like this should be clarified.

@lukemarsden I think the title of this PR no longer reflects the current revision of the proposal doc, doesn't it? It's really not so much about purging from namespaces any more.

lukemarsden · 2016-08-18T09:13:42Z

@ghodss thanks for the review!

I hadn't thought about this, but I concur with your reasoning. I will update the proposal with a section on namespace interactions, cnp'ing your suggestion. Personally I wouldn't add the --all-namespaces implies disabling the flag logic, because it makes the system less flexible. Users should be able to label (and purge) things across namespaces if they wish, even if it's not the recommended convention.
--purge-missing-where "" is a dangerous operation, I'm OK with it being a bit hidden/cryptic. Just as long as --purge-missing-where without the "" doesn't have the same effect (which it shouldn't, because it will require an argument). But maybe it could print a warning which can only be overridden with --force in this case? WDYT? I don't think it's quite necessary personally. --dry-run may help out here.

@bgrant0607 not urgent, but do you have any comments on the above?

lukemarsden · 2016-08-18T09:19:50Z

@errordeveloper title fixed, thanks!

lukemarsden · 2016-08-18T09:24:10Z

Updated the proposal with namespace bits, thanks @ghodss!

0a014ac

k8s-bot · 2016-08-18T10:00:33Z

GCE e2e build/test passed for commit 0a014ac.

bgrant0607 · 2016-08-19T16:38:24Z

docs/proposals/kubectl-apply-purge-missing-where.md

+
+## Interactions with namespaces
+
+* If no namespace is specified, operate across `default` namespace only.


Namespace behavior should be the same as all other kubectl commands, which means that namespace comes from the context by default.

bgrant0607 · 2016-08-19T16:41:55Z

delete has a --all flag to specify all resources in the namespace.
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/delete.go#L108

bgrant0607 · 2016-08-19T16:43:11Z

docs/proposals/kubectl-apply-purge-missing-where.md

+The usage may look like this (assuming the user uses the convention of labelling apps with the `app` label):
+
+```
+kubectl apply --purge-missing-where 'app=myapp' -f https://example.com/myapp.json


Why mash --purge and the label selector together into a single flag? We have a convention for filtering commands by label selector already.

@bgrant0607 because the label selector is specific to the purge operation. Splitting the purge flag from the label selector, IMO, makes it more dangerous, because presumably --purge without a label selector would delete everything not in the provided manifests. Making --purge require -l would beg the question why not unify them? Furthermore the spelling of --purge-missing-where "condition" reads naturally to explain the semantics of what's happening IMO. --purge -l condition is less clear to new users. Furthermore -l makes sense as a filter on other ops because they are simple ops, like delete - it's obvious what's being filtered. But we're making apply a compound operation: "create some things and delete others", -l on its own isn't sufficiently descriptive to make clear what the label selector applies to (only the deletions). That's why I proposed the combined flag -- it's better UX IMO.

With the original design which was entirely label based, merging the flags certainly had merit. But now that we've introduced namespace as a filter as well, doesn't it break down a little? The filter is by selector, namespace, both or none, so at this point, having the purge flag merged specifically with the label selector doesn't seem to make as much sense.

If the goal is to protect against the danger of the flag, we should add a confirmation step which by default lists all resources which will be deleted and requires user confirmation to continue. Then we can add another flag like --continue to skip the prompt.

That way we protect against user error while maintaining our consistency of -l and --namespace.

@lukemarsden I disagree that the selector should only apply to prune/purge. I'd like to be able to use it to update a subset of the objects.

Also, we need to consider the overall usability of kubectl, not just this command. I prioritize consistency across commands fairly highly, so there's a high bar for departing from established precedent.

That pattern (confirmation for dangerous actions) is well enshrined via "--confirm" in other places.

bgrant0607 · 2016-08-19T16:46:03Z

@lukemarsden Please do not deviate from conventions established by other commands.

bgrant0607 · 2016-08-19T16:48:18Z

@lukemarsden Please re-read prior feedback.

lukemarsden · 2016-08-23T15:55:15Z

@bgrant0607 @ghodss @smarterclayton thanks for the feedback! I'll address this in another rev of the proposal shortly.

bgrant0607 · 2016-08-24T06:00:23Z

@lukemarsden Thanks.

Another way to look at this: Assume that apply will get some of the same flags, such as --selector, as other commands (e.g., delete, get, replace). How should --purge-missing interact with those flags?

bgrant0607 · 2016-08-24T06:05:29Z

Relevant flags:

--selector
--all
--all-namespaces

I assume that by default the deletion behavior will correspond to delete --cascade=true --ignore-not-found=true --now=false.

bgrant0607 · 2016-09-23T00:10:07Z

@lukemarsden Is this proposal still active? I would like to see it finished.

mikedanese · 2016-09-26T14:30:44Z

FYI, there is a WIP implementation here #33075

bgrant0607 · 2016-10-10T19:28:01Z

#33075 merged.

errordeveloper and others added 2 commits July 25, 2016 15:59

Proposal for managed namespaces

7437728

Rework proposal a bit

36e28e3

googlebot added the cla: no label Jul 25, 2016

lukemarsden mentioned this pull request Jul 25, 2016

kubectl apply should be able to delete objects missing from supplied config (aka prune) #19805

Closed

k8s-github-robot assigned thockin Jul 25, 2016

k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jul 25, 2016

errordeveloper reviewed Jul 25, 2016
View reviewed changes

mikedanese assigned mikedanese and bgrant0607 and unassigned thockin Jul 25, 2016

mikedanese added area/kubectl sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 25, 2016

bgrant0607 added cla: human-approved and removed cla: no labels Jul 25, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 17, 2016

lukemarsden added 2 commits August 17, 2016 12:44

Rename file

18de1a8

Clarify behavior.

24efbc9

This was referenced Aug 17, 2016

[k8s.io] Probing container should *not* be restarted with a /healthz http liveness probe [Conformance] {E2eNode Suite} #30714

Closed

[k8s.io] Deployment scaled rollout deployment should not block on annotation check {Kubernetes e2e suite} #30100

Closed

lukemarsden changed the title ~~Kubectl apply purge from namespace~~ Proposal: Enable purging resources in kubectl apply Aug 18, 2016

Specify namespace interactions, thanks to @ghodss

0a014ac

bgrant0607 reviewed Aug 19, 2016
View reviewed changes

mikedanese mentioned this pull request Sep 7, 2016

Use a Deployment for kube-dns #32018

Merged

MrHohn mentioned this pull request Sep 28, 2016

Migrating addons from RCs to Deployments. #33698

Closed

bgrant0607 closed this Oct 10, 2016

errordeveloper deleted the kubectl-apply-purge-from-namespace branch October 11, 2016 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Enable purging resources in `kubectl apply` #29551

Proposal: Enable purging resources in `kubectl apply` #29551

lukemarsden commented Jul 25, 2016 •

edited by k8s-oncall

Loading

googlebot commented Jul 25, 2016

lukemarsden commented Jul 25, 2016 •

edited

Loading

errordeveloper commented Jul 25, 2016

errordeveloper Jul 25, 2016

bgrant0607 commented Jul 25, 2016

bgrant0607 commented Jul 25, 2016

smarterclayton commented Jul 25, 2016

smarterclayton commented Jul 25, 2016

lukemarsden commented Jul 26, 2016 •

edited

Loading

philips commented Jul 26, 2016 •

edited

Loading

smarterclayton commented Jul 27, 2016

lukemarsden commented Aug 17, 2016

ghodss commented Aug 17, 2016

errordeveloper commented Aug 18, 2016

lukemarsden commented Aug 18, 2016 •

edited

Loading

lukemarsden commented Aug 18, 2016

lukemarsden commented Aug 18, 2016 •

edited

Loading

k8s-bot commented Aug 18, 2016

bgrant0607 Aug 19, 2016

bgrant0607 commented Aug 19, 2016

bgrant0607 Aug 19, 2016

lukemarsden Aug 20, 2016

ghodss Aug 22, 2016

bgrant0607 Aug 22, 2016

smarterclayton Aug 22, 2016 via email

bgrant0607 commented Aug 19, 2016

bgrant0607 commented Aug 19, 2016

lukemarsden commented Aug 23, 2016

bgrant0607 commented Aug 24, 2016

bgrant0607 commented Aug 24, 2016

bgrant0607 commented Sep 23, 2016

mikedanese commented Sep 26, 2016

bgrant0607 commented Oct 10, 2016


		## Interactions with namespaces

		* If no namespace is specified, operate across `default` namespace only.

Proposal: Enable purging resources in kubectl apply #29551

Proposal: Enable purging resources in kubectl apply #29551

Conversation

lukemarsden commented Jul 25, 2016 • edited by k8s-oncall Loading

googlebot commented Jul 25, 2016

lukemarsden commented Jul 25, 2016 • edited Loading

errordeveloper commented Jul 25, 2016

errordeveloper Jul 25, 2016

Choose a reason for hiding this comment

bgrant0607 commented Jul 25, 2016

bgrant0607 commented Jul 25, 2016

smarterclayton commented Jul 25, 2016

smarterclayton commented Jul 25, 2016

lukemarsden commented Jul 26, 2016 • edited Loading

philips commented Jul 26, 2016 • edited Loading

smarterclayton commented Jul 27, 2016

lukemarsden commented Aug 17, 2016

ghodss commented Aug 17, 2016

errordeveloper commented Aug 18, 2016

lukemarsden commented Aug 18, 2016 • edited Loading

lukemarsden commented Aug 18, 2016

lukemarsden commented Aug 18, 2016 • edited Loading

k8s-bot commented Aug 18, 2016

bgrant0607 Aug 19, 2016

Choose a reason for hiding this comment

bgrant0607 commented Aug 19, 2016

bgrant0607 Aug 19, 2016

Choose a reason for hiding this comment

lukemarsden Aug 20, 2016

Choose a reason for hiding this comment

ghodss Aug 22, 2016

Choose a reason for hiding this comment

bgrant0607 Aug 22, 2016

Choose a reason for hiding this comment

smarterclayton Aug 22, 2016 via email

Choose a reason for hiding this comment

bgrant0607 commented Aug 19, 2016

bgrant0607 commented Aug 19, 2016

lukemarsden commented Aug 23, 2016

bgrant0607 commented Aug 24, 2016

bgrant0607 commented Aug 24, 2016

bgrant0607 commented Sep 23, 2016

mikedanese commented Sep 26, 2016

bgrant0607 commented Oct 10, 2016

Proposal: Enable purging resources in `kubectl apply` #29551

Proposal: Enable purging resources in `kubectl apply` #29551

lukemarsden commented Jul 25, 2016 •

edited by k8s-oncall

Loading

lukemarsden commented Jul 25, 2016 •

edited

Loading

lukemarsden commented Jul 26, 2016 •

edited

Loading

philips commented Jul 26, 2016 •

edited

Loading

lukemarsden commented Aug 18, 2016 •

edited

Loading

lukemarsden commented Aug 18, 2016 •

edited

Loading