Fix force deletion

[micxer]

To force a deletion the grace period has to be set to 0, not 1.

This fixes #1882.

[derailed]

@micxer Hum... We are currently using 1 as the default based on this issue: #1680. This should just delay sigterm to sigkill by 1 sec and hence I would expect your pods to get axed?? Does this happen on all pods or just specific pods?

[micxer]

We lately had an issue with misbehaving nodes and had to kill quite some number of pods in Terminating state with force. So I would say the force version didn't work on any pod not just specific ones. I can't verify this on regular pods as they get terminated normally even without using force.

I changed the value in the PR to 0 since I had a look at the source code and couldn't find the force flag. Then I saw you're setting the grace period to 1. I then had a look at what kubectl does when using --force and lo and behold, it sets the grace period to 0. So I figured this must be the issue as using kubectl manually with --force is working as expected.

[micxer]

Any news on this? I had a second look into the diff between v0.24.7 my colleagues are using and the master branch and can't find a change that could have caused this, at least not in internal/dao/generic.go.

[micxer]

New version still without the fix 😢

[Adrian-St]

@derailed, this is definitely broken, not sure if this is the right fix those?
Force delete works fine in v0.26.2 but stops working in v0.26.3, maybe you know better what exactly changed between those versions? I couldn't find anything obvious in the commits.

[Noksa]

We are waiting for this fix :]

[micxer]

@derailed, this is definitely broken, not sure if this is the right fix those? Force delete works fine in v0.26.2 but stops working in v0.26.3, maybe you know better what exactly changed between those versions? I couldn't find anything obvious in the commits.

Are you really sure this is the case? The changeset doesn't show any change like that.

[micxer]

@derailed There are a few pointers supporting my theory:

https://github.com/kubernetes/kubectl/blob/ca8dddc9820b374e4cb0f4b7974970899fafe848/pkg/cmd/delete/delete.go#L52-L59 talks about setting the grace-period to 1 which is what you referenced. However this is only automatically done in the "now" case. What this bug report and the fix is about is the "force" case.

https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/delete/delete_flags.go#L141 talks about the same cli parameter but also states that setting grace-period to 0 is only allowed when --force is being passed as well. From the debug output I saw for kubectl it actually sets the 0 itself when you add --force to the delete pod command. There is even a whole bunch of test for these scenarios at https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/delete/delete_test.go#L266.

So please merge this PR, it's definitely the correct solution according to the sources of kubectl!

[micxer]

Just built a version based on v0.27.2 and only including my fix. Works as intended.

[derailed]

@micxer I think there are 2 use cases here aka forced deletion vs kill. This is the motivation for --now that was added to kubectl cli to enforce the later. I want to thank you for your research and for surfacing this issue!! Tho your implementation is correct it would not handle the kill use case per #1680 thus always setting gracePeriod to 0. I have experimented with this a bit and think it's best to pass in the gracePeriod vs a boolean so that the call site can dictate the correct behavior.
Does this make sense?

[micxer]

Sounds valid. Thanks for the fix!

[Adrian-St]

@micxer, Yeah I tried it with different versions and those are the ones where I found that the behaviour changed. But it was human testing, so maybe I did a mistake, as long as it works now I'm happy! Thanks @derailed for merging the fix in :)