Make delete actually stop resources by default. #7210

brendandburns · 2015-04-23T04:14:40Z

Refactor for shared code.

@bgrant0607 @ghodss @smarterclayton @eparis

This is a breaking change for kubectl delete ... behavior by default.

brendandburns · 2015-04-23T04:14:53Z

@deads2k too for good measure ;)

bgrant0607 · 2015-04-23T06:04:19Z

And cc @jlowdermilk

We could put a deprecated message on the stop command, like we did for rollingupdate.

deads2k · 2015-04-23T11:57:50Z

pkg/kubectl/cmd/delete.go

+	err := r.Visit(func(info *resource.Info) error {
+		found++
+		reaper, err := f.Reaper(info.Mapping)
+		cmdutil.CheckErr(err)


Probably don't want to fatal in here. Can you return the error instead?

Oh, sorry for the dupe. PR versus commit.

brendandburns · 2015-04-23T22:01:31Z

Comments addressed. I'm not convinced we should deprecate kubectl stop ... I think there is value in stop as a counter-point to run

We should alias them together, though.

thockin · 2015-04-23T22:41:11Z

Without reading the PR, SGTM. How do we do a "delete and abandon" ?

On Thu, Apr 23, 2015 at 3:01 PM, Brendan Burns notifications@github.com
wrote:

Comments addressed. I'm not convinced we should deprecate kubectl stop ...
I think there is value in stop as a counter-point to run

We should alias them together, though.

—
Reply to this email directly or view it on GitHub
#7210 (comment)
.

bgrant0607 · 2015-04-23T23:26:50Z

Delete and abandon is delete --recursive=false.

I'm fine with aliasing stop and delete, though we should document that: #5370.

j3ffml · 2015-04-24T00:25:37Z

LGTM. +1 to keeping stop as an alias for delete --recursive=true

liggitt · 2015-04-24T01:03:51Z

What happens if you delete something which doesn't support stop without adding --recursive=false? I like having the recursive option, but I'm not sure it should be the default

ghodss · 2015-04-24T03:39:42Z

pkg/kubectl/cmd/stop.go

@@ -86,3 +63,24 @@ func NewCmdStop(f *cmdutil.Factory, out io.Writer) *cobra.Command {
 	cmd.Flags().Bool("all", false, "[-all] to select all the specified resources")
 	return cmd
 }
+
+func DoStop(f *cmdutil.Factory, cmd *cobra.Command, args []string, filenames util.StringList, out io.Writer) error {


Nit... I think it's usually Run*, so RunStop.

ghodss · 2015-04-24T03:48:36Z

Is recursive the right word here? Obviously it fits for RC's but doesn't this indicate whether to wait for a graceful stop of pods too? (Where recursive would be less appropriate.) Or does it not apply to pods?

ghodss · 2015-04-24T03:51:48Z

If the outcome of this PR is that delete == stop, does it just make sense to make stop an alias to delete in this PR?

thockin · 2015-04-24T03:58:00Z

--cascade ?

On Thu, Apr 23, 2015 at 8:49 PM, Sam Ghods notifications@github.com wrote:

Is recursive the right word here? Obviously it fits for RC's but doesn't
this indicate whether to wait for a graceful stop of pods too? (Where
recursive would be less appropriate.) Or does it not apply to pods?

—
Reply to this email directly or view it on GitHub
#7210 (comment)
.

derekwaynecarr · 2015-04-24T04:24:56Z

+1 for --cascade

Sent from my iPhone

On Apr 23, 2015, at 11:58 PM, Tim Hockin notifications@github.com wrote:

--cascade ?

On Thu, Apr 23, 2015 at 8:49 PM, Sam Ghods notifications@github.com wrote:

Is recursive the right word here? Obviously it fits for RC's but doesn't
this indicate whether to wait for a graceful stop of pods too? (Where
recursive would be less appropriate.) Or does it not apply to pods?

—
Reply to this email directly or view it on GitHub
#7210 (comment)
.

—
Reply to this email directly or view it on GitHub.

ghodss · 2015-04-24T06:30:49Z

Aren't options like "--delete-only=true" or "--graceful=false" more appropriate? --cascade still sounds like there are more objects to deal with, whereas that's not true with pods, or other objects we introduce that exclusive to themselves have a graceful and non-graceful way to stop.

thockin · 2015-04-24T07:01:16Z

I think "graceful" and "cascade" are orthogonal ideas, and even though they
appear to be mutually exclusive now, we should not presume that they will
be forever. Consider a world where deleting a pod with --cascade=false
might leave the pod's data volumes alive, while --cascade=true would
destroy them. That's a very different concern than whether the pod gets 30
seconds of warning or not.

On Thu, Apr 23, 2015 at 11:31 PM, Sam Ghods notifications@github.com
wrote:

Aren't options like "--delete-only=true" or "--graceful=false" more
appropriate? --cascade still sounds like there are more objects to deal
with, whereas that's not true with pods, or other objects we introduce that
exclusive to themselves have a graceful and non-graceful way to stop.

—
Reply to this email directly or view it on GitHub
#7210 (comment)
.

googlebot · 2015-04-24T23:11:21Z

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.

googlebot · 2015-04-24T23:12:56Z

CLAs look good, thanks!

brendandburns · 2015-04-24T23:13:58Z

Switched to --cascade, @liggitt I added code so that if the flag is unset, and there is no reaper, then we skip back to the existing delete code.

I will add the alias in a different PR.

Please take another look.

Thanks!
--brendan

bgrant0607 · 2015-04-24T23:14:00Z

pkg/kubectl/cmd/delete_test.go

@@ -50,6 +50,7 @@ func TestDeleteObjectByTuple(t *testing.T) {

 	cmd := NewCmdDelete(f, buf)
 	cmd.Flags().Set("namespace", "test")
+	cmd.Flags().Set("recursive", "false")


Should be change to cascade (everywhere).

brendandburns · 2015-04-25T03:46:57Z

Comments addressed, please re-check.

Thanks!
--brendan

brendandburns · 2015-04-27T18:22:43Z

Friendly ping on this.

Thanks!
--brendan

j3ffml · 2015-04-27T18:29:15Z

pkg/kubectl/cmd/delete.go

+		reaper, err := f.Reaper(info.Mapping)
+		if err != nil {
+			// If the error is "not found" and the user didn't explicitly set "cascade" just delete.
+			if kubectl.IsNoSuchReaperError(err) && !cmd.Flags().Lookup("cascade").Changed {


This will crash (nil deref I think) if you call it from kubectl/cmd/stop, because stop does not define a "cascade" flag. This error handling should be moved into RunDelete.

done. (can't do it in RunDelete because I do want to Stop those things that support stop, and not delete everything just because something else can't be stopped)

Checking the flag for nil is kind of an ugly hack... How about this: change ReapResult to take a boolean 'defaultToDelete' which if true, makes ReapResult do a standard delete on resources for which there is no reaper available. That way you don't have to pass in the command object, and you can do the check for an explicit --cascade in RunDelete.

brendandburns · 2015-04-27T20:18:39Z

Comment addressed, please re-check.

--brendan

j3ffml · 2015-04-27T23:28:59Z

pkg/kubectl/cmd/delete.go

+		reaper, err := f.Reaper(info.Mapping)
+		if err != nil {
+			// If the error is "not found" and the user didn't explicitly set "cascade" just delete.
+			if kubectl.IsNoSuchReaperError(err) && cmd.Flags().Lookup("cascade") != nil && !cmd.Flags().Lookup("cascade").Changed {


Repeating since it may have been lost in outdated diff

Checking the flag for nil is kind of an ugly hack... How about this: change ReapResult to take a boolean defaultToDelete instead of passing the Command object. If defaultToDelete is true, do a standard delete on resources for which there is no reaper available. That way you can move the check for an explicit --cascade into RunDelete, and we're not checking for nonexistent flags as a proxy for "which toplevel command is calling me".

brendandburns · 2015-04-28T03:37:04Z

rebased.

j3ffml · 2015-04-28T05:40:56Z

There's an unresolved comment in the diff for pkg/kubectl/cmd/delete.go that seems to be getting attached to the outdated diff thread. Please take a look and let me know if you can't see it or object to suggested change.

j3ffml · 2015-04-28T21:33:01Z

pkg/kubectl/cmd/stop.go

+	if r.Err() != nil {
+		return r.Err()
+	}
+	return ReapResult(r, f, out, true)


Shouldn't this be false? By running stop user is explicitly requesting reaper.

j3ffml · 2015-04-28T21:33:39Z

lgtm aside from isDeleteDefault=false for RunStop.

Refactor for shared code.

brendandburns · 2015-04-28T21:37:16Z

comment addressed, ptal. unit tests are manually determined to be clean.

j3ffml · 2015-04-28T22:22:56Z

lgtm

Make delete actually stop resources by default.

googlebot added the cla: yes label Apr 23, 2015

deads2k reviewed Apr 23, 2015
View reviewed changes

brendandburns force-pushed the kubectl branch from c4886a9 to 7515972 Compare April 23, 2015 21:59

brendandburns force-pushed the kubectl branch 2 times, most recently from 8f583bb to aeba3ad Compare April 23, 2015 22:02

bgrant0607 self-assigned this Apr 23, 2015

ghodss reviewed Apr 24, 2015
View reviewed changes

brendandburns force-pushed the kubectl branch 2 times, most recently from f663c3c to 56d0ebe Compare April 24, 2015 23:11

googlebot added cla: no and removed cla: yes labels Apr 24, 2015

brendandburns force-pushed the kubectl branch from 56d0ebe to 7c88dcb Compare April 24, 2015 23:12

googlebot added cla: yes and removed cla: no labels Apr 24, 2015

bgrant0607 reviewed Apr 24, 2015
View reviewed changes

brendandburns force-pushed the kubectl branch from 7c88dcb to 49761ca Compare April 25, 2015 03:46

brendandburns force-pushed the kubectl branch from 49761ca to 23344c0 Compare April 25, 2015 03:53

j3ffml reviewed Apr 27, 2015
View reviewed changes

brendandburns force-pushed the kubectl branch from 23344c0 to 9a85cfa Compare April 27, 2015 20:17

bgrant0607 assigned j3ffml and unassigned bgrant0607 Apr 27, 2015

j3ffml reviewed Apr 27, 2015
View reviewed changes

brendandburns force-pushed the kubectl branch from 9a85cfa to 2656c54 Compare April 28, 2015 03:36

brendandburns force-pushed the kubectl branch from 2656c54 to 3be87ca Compare April 28, 2015 21:29

j3ffml reviewed Apr 28, 2015
View reviewed changes

Make delete actually stop resources by default.

e1256c0

Refactor for shared code.

brendandburns force-pushed the kubectl branch from 3be87ca to e1256c0 Compare April 28, 2015 21:37

j3ffml added a commit that referenced this pull request Apr 28, 2015

Merge pull request #7210 from brendandburns/kubectl

44e7109

Make delete actually stop resources by default.

j3ffml merged commit 44e7109 into kubernetes:master Apr 28, 2015

brendandburns unassigned j3ffml Aug 12, 2015

liggitt mentioned this pull request Jan 5, 2017

federation: kubectl --cascade should be false by default for federation #38897

Closed

irfanurrehman mentioned this pull request Oct 31, 2017

federation: kubectl --cascade should be false by default for federation kubernetes-retired/federation#89

Closed

Make delete actually stop resources by default. #7210

Make delete actually stop resources by default. #7210

Conversation

brendandburns commented Apr 23, 2015

brendandburns commented Apr 23, 2015

bgrant0607 commented Apr 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Apr 23, 2015

thockin commented Apr 23, 2015

bgrant0607 commented Apr 23, 2015

j3ffml commented Apr 24, 2015

liggitt commented Apr 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghodss commented Apr 24, 2015

ghodss commented Apr 24, 2015

thockin commented Apr 24, 2015

derekwaynecarr commented Apr 24, 2015

ghodss commented Apr 24, 2015

thockin commented Apr 24, 2015

googlebot commented Apr 24, 2015

googlebot commented Apr 24, 2015

brendandburns commented Apr 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Apr 25, 2015

brendandburns commented Apr 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Apr 27, 2015

Choose a reason for hiding this comment

brendandburns commented Apr 28, 2015

j3ffml commented Apr 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j3ffml commented Apr 28, 2015

brendandburns commented Apr 28, 2015

j3ffml commented Apr 28, 2015