[Bug Fix] Allow Status Updates for Pods that can be deleted#42779
Conversation
|
This change is |
|
/release-note-none |
|
7 runs (of all 3 BC tests) without a flake. Ill do another run, but I am seeing consistent deletion times of around 30 seconds. |
| // syncPod syncs the given status with the API server. The caller must not hold the lock. | ||
| func (m *manager) syncPod(uid types.UID, status versionedPodStatus) { | ||
| if !m.needsUpdate(uid, status) { | ||
| if !m.needsUpdate(uid, status) && !m.couldBeDeleted(uid, status.status) { |
There was a problem hiding this comment.
Add a comments explains the logic?
|
another 6 runs without a flake. |
|
/assign @yujuhong @dchen1107 @derekwaynecarr |
| func (m *manager) syncPod(uid types.UID, status versionedPodStatus) { | ||
| if !m.needsUpdate(uid, status) { | ||
| // always allow status updates if the pod can be deleted. | ||
| if !m.needsUpdate(uid, status) && !m.couldBeDeleted(uid, status.status) { |
There was a problem hiding this comment.
This is still fragile since you have to keep the code at two places in sync. Maybe add another method dedicated for the check?
There was a problem hiding this comment.
What about moving it inside needsUpdate? Then it would only be in one place.
|
/approve Given comments in issue and that it is a bug |
|
/lgtm |
|
/lgtm @dashpole Can you also introduce a pod deletion test to node e2e? I observed a lot of deletion regression lately. Hope the test can help us to catch the regression earlier. Thanks! |
|
@dchen1107: you can't LGTM a PR unless you are an assignee. DetailsIn response to this comment:
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. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: dashpole, dchen1107, smarterclayton, yujuhong Needs approval from an approver in each of these OWNERS Files:
We suggest the following people: |
|
@k8s-bot bazel test this |
|
@k8s-bot cvm gce e2e test this |
|
I ran another set of GC tests on the latest version of this PR just as a sanity check. It passed. |
|
All tests are passed. I plan to manually merge this to stop the bleeding submit-queue in 5 mins. Of course there are other flakiness. |
Fixes #42685 (comment) (at least part of it), #42776.
Running e2e node garbage collection tests on this. Ill report in when I have results.
In the meantime, please review the change.
cc @yujuhong @dchen1107 @derekwaynecarr