fix using stale pod when evict failed and retry - #133461

Please note that we're already in Test Freeze for the release-1.34 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.34.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Mon Aug 11 10:25:10 UTC 2025.

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Welcome @justlorain!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @justlorain. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/cc @ardaguclu

/ok-to-test
Thank you for opening this PR. I think, we definitely need new tests to verify the behavior. Test should be failing before the patch and should be passing after the patch.

/ok-to-test Thank you for opening this PR. I think, we definitely need new tests to verify the behavior. Test should be failing before the patch and should be passing after the patch.

Sure, I'll add the relevant tests.

Thanks for the PR.

Looking more closely at the code in drain.go, I have a few thoughts... Not about your changes, but about the entire evictPods func.

Mainly, I'm unclear why refreshPod is even needed.

The code clearly isn't working as intended right now, since refreshPod defaults to false and is never set to true (even the verify CI job detects it as a problem now it looks like with "ineffectual assignment to refreshPod" on line 309).

But I think we can achieve the desired behavior of refreshing the pod by simply moving the getPodFn() call to the end of the for loop block and then getting rid of the refreshPod variable altogether.

If the code makes it through the error handling without returning or breaking, then (I think) we should always attempt to re-get the pod.

So the first time through the loop, we use the original pod (set activePod := pod before the for loop). And then at the end of the for loop block, if the code execution makes it there, call getPodFn() and update activePod if it succeeds.

What do you think? Does this make sense or am I missing something? I feel like this logic would be more straightforward since it relies on regular code flow instead of having to manage and check a boolean.

Thanks for the PR.

Looking more closely at the code in drain.go, I have a few thoughts... Not about your changes, but about the entire evictPods func.

Mainly, I'm unclear why refreshPod is even needed.

The code clearly isn't working as intended right now, since refreshPod defaults to false and is never set to true (even the verify CI job detects it as a problem now it looks like with "ineffectual assignment to refreshPod" on line 309).

But I think we can achieve the desired behavior of refreshing the pod by simply moving the getPodFn() call to the end of the for loop block and then getting rid of the refreshPod variable altogether.

If the code makes it through the error handling without returning or breaking, then (I think) we should always attempt to re-get the pod.

So the first time through the loop, we use the original pod (set activePod := pod before the for loop). And then at the end of the for loop block, if the code execution makes it there, call getPodFn() and update activePod if it succeeds.

What do you think? Does this make sense or am I missing something? I feel like this logic would be more straightforward since it relies on regular code flow instead of having to manage and check a boolean.

Thanks for your review! I think your approach is completely reasonable and intuitive for me. I've tried to implement a new commit based on it - could you please check if it aligns with your thinking?

I think the changes look good.

A few more things:

1. Can you write a unit test for this? This will help demonstrate it is working as expected and prevent a future regression.
2. The lint CI job flagged a couple of problems you'll need to handle since you touched those Fprintf lines. You can use _, _ = or //nolint:errcheck on the Fprintf lines to handle it.
3. I don't know if the other failed CI tests are legit or not. You can re-run the tests but if the failures are legit you'll need to check why.

I think the changes look good.

A few more things:

1. Can you write a unit test for this? This will help demonstrate it is working as expected and prevent a future regression.
2. The lint CI job flagged a couple of problems you'll need to handle since you touched those Fprintf lines. You can use _, _ = or //nolint:errcheck on the Fprintf lines to handle it.
3. I don't know if the other failed CI tests are legit or not. You can re-run the tests but if the failures are legit you'll need to check why.

Hi, I've added the relevant unit tests and resolved the CI issues. Could you please review it again?

Excellent work.

The changes you made look good and thank you for writing the unit tests.

The only thing I noticed is that the unit test takes 15s to run. Can you make some changes to improve the test speed?

One approach might be to change the hard-coded 5 * time.Second (in drain.go) into a variable so the unit test can change it.

For example:

var drainErrorRetryDelay = 5 * time.Second

And then something like this in the two places (here and here) where it is used. For example:

fmt.Fprintf(d.ErrOut, "error when evicting pods/%q -n %q (will retry after %v): %v\n", activePod.Name, activePod.Namespace, drainErrorRetryDelay, err)
time.Sleep(drainErrorRetryDelay)

And finally, in the unit test, you can then reduce the delay to 1ms and also adjust the globalTimeout variable to make the test run faster:

drainErrorRetryDelay = 1 * time.Millisecond
globalTimeout := drainErrorRetryDelay * 2

What do you think?

Excellent work.

The changes you made look good and thank you for writing the unit tests.

The only thing I noticed is that the unit test takes 15s to run. Can you make some changes to improve the test speed?

One approach might be to change the hard-coded 5 * time.Second (in drain.go) into a variable so the unit test can change it.

For example:

var drainErrorRetryDelay = 5 * time.Second

And then something like this in the two places (here and here) where it is used. For example:

fmt.Fprintf(d.ErrOut, "error when evicting pods/%q -n %q (will retry after %v): %v\n", activePod.Name, activePod.Namespace, drainErrorRetryDelay, err)
time.Sleep(drainErrorRetryDelay)

And finally, in the unit test, you can then reduce the delay to 1ms and also adjust the globalTimeout variable to make the test run faster:

drainErrorRetryDelay = 1 * time.Millisecond
globalTimeout := drainErrorRetryDelay * 2

What do you think?

I'm thinking if would be better to introduce the retry delay as a field of the Helper.

Another consideration I have is whether it's necessary to make the retry delay setting a new option for the drain command. In that case, having it as a field of Helper would also maintain consistency with the style of other options. However, this consideration seems to have already moved beyond the scope of what this PR is discussing.

What do you think?

I'm thinking if would be better to introduce the retry delay as a field of the Helper.

That seems like it should work too. In that case though, it would be up to the caller to set the retry duration. Not a problem for kubectl, but if any other tool uses this drain helper, it will need to know to set that retry duration to 5 seconds or else the behavior would be different. I do think the existing retry behavior is broken right now anyway though, so it's probably not an issue.

Another consideration I have is whether it's necessary to make the retry delay setting a new option for the drain command. In that case, having it as a field of Helper would also maintain consistency with the style of other options. However, this consideration seems to have already moved beyond the scope of what this PR is discussing.

Yeah, I agree and don't think we need to expose retry delay as a command option at this point. If it is something that someone really needs for some reason, they can open an issue for it and we can decide whether to add it then. I think you're right... let's keep this PR focused on fixing the bug.

The only thing I noticed is that the unit test takes 15s to run. Can you make some changes to improve the test speed?

Attempted to introduce EvictErrorRetryDelay field in Helper to resolve this. Could you please check again to see if there are any issues?

LGTM label has been added.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, justlorain

The full list of commands accepted by this bot can be found here.

The pull request process is described here