federation: kubectl --cascade should be false by default for federation #89
Comments
irfanurrehman
added
area/federation
area/kubectl
priority/important-soon
|
A hacky way to do this is for kubectl to first determine if it is talking to federation apiserver and if yes, then set the default to false, else keep it to true. Open to other suggestions on how to implement this. |
|
why would we want orphaning to be the default for federation? I didn't think federation allowed adopting cluster-level resources, so if a federated resource originated with the federation server, it seems natural that it would end there as well |
|
@LiGgit federation does adopt resources. @nikhiljindal the hacky way is a documentation challenge though. It is difficult for obvious reasons to generate the help text dynamically. Example: people might run |
|
cascade has potential to be disruptive in a single cluster as well, yet it was chosen as the default there. |
|
@liggitt I can't speak to why the disruptive option was chosen as the default in the single cluster case (do you perhaps know?), however avoiding global outages is one of the primary use cases of Federation, so making global cascading deletion the default for Federation does not seem wise. Other than the consistency argument, can you think of any good reasons why global cascading deletion should be the default? |
|
@nikhiljindal, Maybe kubectl can determine, if it is talking to federation apiserver or kubernetes apiserver by checking the support for cluster object. is there a better way? |
|
I think changing CLI defaults based on the server being spoken to is very difficult to understand, and seems like a bad precedent to set |
|
otherwise, we could introduce a new flag for federation object cascade deletion maybe with suitable name |
|
sorry, ignore my previous comment. as we are discussing default behaviour |
|
@liggitt Do you have answers to my questions above? kubernetes/kubernetes#38897 (comment) |
|
I think the reasons are identical to the ones made for local cascading being on by default (I wasn't a fan of it there either). See kubernetes/kubernetes#7210 |
|
How about having kubectl to ask user for confirmation when the command is directed to federation apiserver? I never saw kubectl asking for user confirmation for anything so this may be a silly proposition. |
|
yeah, none of those are interactive commands |
|
I discussed this with @bgrant0607 and he didnt like the idea of changing the default based on which server is kubectl talking to. He pointed out that total outage is a problem in kubernetes as well and we need generic ways to prevent that rather than change the default in federation. Cascading deletion with kubectl is pretty broken in federation right now. Some resources do part cascading deletion and some dont at all. We need to fix this soon. |
|
#10179 |
|
The whole point of the approach with federation is that it looks like a K8s cluster. Therefore, behavior must be the same, by definition. |
|
I strongly disagree Brian. More later... On Jan 25, 2017 12:31 AM, "Brian Grant" notifications@github.com wrote:
|
|
TL;DR:
PS: It might be quicker to resolve this with a brief verbal discussion. I'll email you my phone number. Detail: Why is cascading deletion the default in kubernetes (both in the API, where DeleteOptions.orphanDependents defaults to false, and in kubectl, where --cascade defaults to true )? Given that the overwhelming majority of familiar command line tools and API's default to false for destructive cascading and/or recursive deletions (e.g. rm, rmdir, libc remove, etcd delete, consol delete and just about any other one I can think of), I assume that we have a good reason for being inconsistent with the user expectation that that creates (principle of least surprise etc, etc)? The reasoning behind not having such destructive operations be the default are fairly obvious, and the reason why it is not the norm in API's and command line tools. I've looked through the issues and PR's you quoted above, and I can't find any explanation of why this default behavior was adopted in Kubernetes. What I can find is objections to it being the default behavior (e.g. @liggitt in kubernetes/kubernetes#7210 (comment)). And now a clarification: cluster federation's primary purpose is to be able to easily manage applications across multiple clusters, the main use case of which is avoiding application outages (although there are of course other use cases, like geo-locality, hybrid cloud etc). The fact that cluster federation implements the kubernetes API is somewhat of an implementation detail, and in fact there were some fairly vigorous arguments that it should not. In the end we decided to try to remain true to the Kubernetes API as far is is practical, for reasons of user familiarity and tool compatibility, but recognized that that will not always be tenable/sensible. I think that this is a case where there is a strong reason not to be consistent with the kubernetes API and tool default to perform destructive cascading deletion. StatefulSet provides another example of a similar case where full cascading deletion is not the default (the underlying persistent volumes are explicitly not deleted when statefulsets are deleted). I'm not for the moment concerned about exactly how we go about implementing non-default cascading deletion (and I share your distaste for special-casing it in kubectl). I think we should first decide what the default behavior should be. Full disclosure: Based on what I currently know, it seems that both kubernetes and federation API's should default to "orphanChildren=true" (i.e. cascadingDelete=false) at both the API and kubectl level, with an option to explicitly trigger cascading deletion when required. This would be both safer and more consistent with other similar API's and tools. |
|
Some history may be traced from #1535. Cascading by default is the behavior expected by the vast majority of users. Most users want to think of Deployments, ReplicaSets, etc. as atomic entities. Originally we orphaned by default and it was a significant cause of user friction. The default in kubectl is not changing without evidence that it is the cause of catastrophies and without consensus among the broader user base, since it would increase user friction and break pretty much every use of kubectl in the ecosystem. I'd be happy for a default preference to be added that users could change if someone would add a reasonable preference mechanism. #10693 The API couldn't be changed to cascade by default because GC was added much, much later, and doing so would break compatibility. Until we added GC, lack of cascading deletion continued to be a pain point for clients other than kubectl. It's part of the broader effort to make the API simpler for common operations. #12143 Note that when we add new resources under existing ones, cascading deletion must be applied to them by default so as not to break existing clients. If/when we have a v2 API, I'll make cascading deletion the default in the API as well. If federation were not compatible with standard Kubernetes, then many tools, scripts, clients, etc. wouldn't work. Additionally, this issue isn't unique to federation and shouldn't be solved in a one-off manner. Deletion-prevention could be solved with authorization policy, which would prevent catastrophic updates as well, or with a generalized deletion-prevention mechanism, which is desired for system components in individual clusters #10179. Cascading deletion was also added when deleting namespaces. That is the default behavior, since the namespace cannot be removed while it still contains resources. |
|
@bgrant0607 OK, I now understand your basic argument to be that ease of use trumps safety (where safety in this case means requiring a relatively simple positive indication in the API or tool, along the lines of --cascade/--recursive/--force to confirm that large, non-obvious destructive operations will be performed automatically, like terminating an application globally, in this case, or deleting a bunch of valuable persistent data in the case of statefulset persistent volumes). That's contrary to what most other API's and tools have decided (see examples above), despite their motivations being very similar ("Id' like to think of a directory tree as being a single logical thing, and be able to simply do 'rmdir mydir', but it doesn't work - I have to specify '--recurse --force'"). Having read through the various threads you've sent me, it also seems to be contrary to what many other Kubernetes contributors have opined (for example @thockin in kubernetes/kubernetes#1535 (comment) and there are several others like it). And it's contrary to what I think :-) However, as you say, reverting those decisions now will be disruptive. For federation, I guess we will have to figure out another way to reduce the likelihood of accidental global cascading deletions and the resulting global service outages. That solution will probably be more complex and/or dangerous for users than what's proposed above, but so be it. |
|
@quinton-hoole It sounds like you want an equivalent of |
|
@quinton-hoole I'll also point out that tools like helm will be able to tear down even more resources in a single command line. |
|
@bgrant0607 The alias you mention is definitely one solution. Although rm actually works exactly the way I want, in the absence of the alias (i.e. by default). It's not clear to me how we could automatically populate most or all user's preferences (both for kubectl, and the API) with the safer options. If we can work out a way to do that, I'd be less concerned. But making it the default in the API and tool seems simpler and more sensible. As for helm, I have no problem whatsoever with being able to wreck havoc with a single command line invocation, so long as it's obvious what I'm doing, and not the default, flagless behavior. I can delete an entire file server with 'rm --recursive --force /' but not with 'rm /' :-) |
|
To clarify, cascading deletion is not the default in the API (neither in kubernetes nor in federation). |
|
@nikhiljindal @bgrant0607 Yes, thanks for bringing that up. I was about to ask for clarification. As far as I can tell from both the API documentation (https://kubernetes.io/docs/api-reference/v1/definitions/#_v1_deleteoptions), and the code, cascading deletion is the default in the API Am I misunderstanding something? |
|
@quinton-hoole Yes, that documentation is confusing/wrong. It should be API-dependent. |
|
It looks like the API doc marks the default value of every boolean variable to be false, and boolean is the only type that has a default value. We should remove the "default" column. @pwittrock is default values fixed in the new versions of API docs? "correct" documentation on GC: The description is true if user doesn't manually set OwnerRef. As Brian said, default orphaning/cascading deletion is per API group. As of today, the default GC policy is "orphan" for RC, RS and Deployment. For other resources, the default GC policy is "cascading", however, because no ownerRefs are automatically set to point to these resources, so no GC action will be triggered by their deletion. |
|
The new API docs don't have a default value in the documentation. I think this was because I noticed the information was missing / incorrect. https://kubernetes.io/docs/resources-reference/v1.5/#deleteoptions-v1 I don't see the default value for orphanDependents in the swagger: https://raw.githubusercontent.com/kubernetes/kubernetes/master/api/openapi-spec/swagger.json A simple fix for the new api docs is to include the default value in the description text. |
|
As per the discussion above, defaults for cascading deletion in federation will be same as in kubernetes i.e |
|
Please don't close this yet. It's not resolved. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Friday Dec 16, 2016 at 20:18 GMT
Originally opened as kubernetes/kubernetes#38897
Ref kubernetes/kubernetes#33612.
Cascading deletion is true by default for kubectl.
For federation, we want it to be false since deleting a federation resource with cascade=true will delete the resource from all clusters as well which can lead to a global outage. So we want --cascade to be false by default but users can pass --cascade=true if they do want cascading deletion.
cc @kubernetes/sig-federation-misc @kubernetes/sig-cli-misc @quinton-hoole @caesarxuchao
The text was updated successfully, but these errors were encountered: