Proposal: Enable purging resources in kubectl apply

[lukemarsden]

Proposal to resolve #19805

---

This change is

[googlebot]

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]

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

[errordeveloper]

This CLA check is a bit confusing..

[errordeveloper]

insert -part here, to reflect what's above

[bgrant0607]

cc @ghodss @kubernetes/kubectl

[bgrant0607]

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

[smarterclayton]

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

[smarterclayton]

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

[lukemarsden]

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]

cc @ethernetdan @colhom

[smarterclayton]

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]

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

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

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

1. If no namespace is specified, operate across default namespace only.
2. If --namespace is specified, only operate in that namespace.
3. 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.

2. 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]

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

@ghodss thanks for the review!

1. 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.
2. --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]

@errordeveloper title fixed, thanks!

[lukemarsden]

Updated the proposal with namespace bits, thanks @ghodss!

0a014ac

[k8s-bot]

GCE e2e build/test passed for commit 0a014ac.

• Test Results
• Build Log
• Test Artifacts
• Internal Jenkins Results

[bgrant0607]

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

[bgrant0607]

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]

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

[lukemarsden]

@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.

[ghodss]

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.

[bgrant0607]

@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.

[smarterclayton]

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

[bgrant0607]

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

[bgrant0607]

@lukemarsden Please re-read prior feedback.

[lukemarsden]

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

[bgrant0607]

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

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]

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

[mikedanese]

FYI, there is a WIP implementation here #33075

[bgrant0607]

#33075 merged.