fix bug on kubectl deleting uninitialized resources#51186
fix bug on kubectl deleting uninitialized resources#51186k8s-github-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
Hi @dixudx. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
pkg/kubectl/delete.go
Outdated
| } | ||
|
|
||
| _, err = reaper.updateReplicaSetWithRetries(namespace, name, func(rs *extensions.ReplicaSet) { | ||
| // set replicaset's initializers to nil |
There was a problem hiding this comment.
chasing disabling initializers on everything seems very error-prone. @smarterclayton, what would you expect to happen here?
There was a problem hiding this comment.
I definitely think that this is a bug in the reaper. Why do we still have the reaper?
A normal user isn't going to be able to perform this operation, so this isn't the correct fix. I think if the object hasn't been initialized yet the reaper should just call delete.
|
/ok-to-test |
|
@dixudx could you explain what's the reason of the deletion failure? Is it because kubectl is waiting for the controller's status (like observedGeneration, replicas etc) to update? If that's the case, you can early return in the Scale() call. That way you don't need an extra GET/UPDATE. The entire problem will be gone when we move |
@caesarxuchao Yes, current
Yes, this is a nicer way to solve this problem. That means we have to make special pass for @liggitt @caesarxuchao WDYT? |
|
@dixudx that looks right to me. @smarterclayton the removal of the reapers is tracked at: #50340 |
95bddf0 to
493f063
Compare
|
ping @caesarxuchao @smarterclayton @liggitt Updated. PTAL. Thanks. |
|
/retest |
|
Much simpler |
|
/retest |
pkg/kubectl/delete.go
Outdated
| d.Spec.Replicas = 0 | ||
| d.Spec.Paused = true | ||
| if d.Initializers != nil { | ||
| d.Initializers = nil |
There was a problem hiding this comment.
We can't assume kubectl user has the permission to set initializers. Just get the deployment at the beginnig at Stop and delete it immediately if initializers!=nil.
|
Could you add some tests? Maybe one test case for |
493f063 to
7448365
Compare
7448365 to
41c8799
Compare
|
@caesarxuchao Add the tests. PTAL. |
hack/make-rules/test-cmd-util.sh
Outdated
| # Pre-condition: no deployment exists | ||
| kube::test::get_object_assert deployment "{{range.items}}{{$id_field}}:{{end}}" '' | ||
| # Create a deployment | ||
| kubectl create -f hack/testdata/deployment-with-initializer.yaml "${kube_flags[@]}" |
There was a problem hiding this comment.
#51436 disabled the initializer field if the feature gate is off, perhaps you need to enable the feature gate in this test.
d5471b5 to
5e120cf
Compare
|
/lgtm |
|
/assign @liggitt @fabianofranz for approval Thanks. |
|
@caesarxuchao: GitHub didn't allow me to assign the following users: for, approval. Note that only kubernetes members can be assigned. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
This is a bug fix, so it's ok we don't get approval before the code freeze deadline. |
|
ping @liggitt @fabianofranz for approval. Thanks. |
|
/approve |
|
@fabianofranz @pwittrock for approval and approved-for-milestone tagging if this is required for 1.8 |
|
/test pull-kubernetes-e2e-kops-aws |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao, dixudx, fabianofranz, liggitt Associated issue: 51185 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 51186, 50350, 51751, 51645, 51837) |
|
@dixudx: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #51185Special notes for your reviewer:
/assign @caesarxuchao @ahmetb
Release note: