KEP4664: Guarantee PodDisruptionBudget When Preemption Happens

[AxeZhan]

• One-line PR description: Guarantee PodDisruptionBudget When Preemption Happens.

• Issue link: Guarantee PodDisruptionBudget When Preemption Happens #4664

• Other comments:
  This is the first pr for Guarantee PodDisruptionBudget When Preemption Happens #4664. But notice that Guarantee PodDisruptionBudget When Preemption Happens #4664 is only taking over Guarantee PodDisruptionBudget When Preemption Happens #3280. That's why this pr only contains a few updates on the design details(Most part of it has already been reviewed and approved)

[AxeZhan]

This part is confusing to me. Are we tring to introduce /evictions endpoint in this KEP ?

IIRC, we don't using eviction in preemption right now. https://github.com/kubernetes/kubernetes/blob/4a668bcf143c0652882d2e59051dcf0cd843c24c/pkg/scheduler/util/utils.go#L137

Besides, I don't think we should use /evictions endpoint here.
During the preemption, we'll separate victims into two parts: nonViolatingCandidates, violatingCandidates.

Since we already know in advance which pods' deletion would violate the PDB.

1. Using eviction on nonViolatingCandidates will equal to delete it directly(?)
2. Using eviction on violatingCandidates will always return 429. And since we have already deleted all nonViolatingCandidates on the same node before trying to delete violatingCandidates, this will results in all violatingCandidates can't be deleted and thus preemption will always fail when deleting all nonViolatingCandidates is not enough.

Besides, if we use /evictions endpoint for all pods in preemption. Then what's the point of this KEP? We'll deny any preempter if pdb is violated.
Also, use /evictions endpoint for all pods would also be a breaking change and is not backward compatible.
@alculquicondor @Huang-Wei

[atiratree]

Please take a look at the original issue to understand the drive behind this KEP.

Also take a look at this comment to understand the reason behind the eviction first policy.

[AxeZhan]

Thanks! I read the original issue and comment you left in #3280

The drive behind the KEP and my understanding while reading it seem to be different. I thought the KEP was introducing a new field to make it harder for specific pods to be preempted during scheduling. However, it appears that the original intention is for the scheduler to respect PDBs in every preemptive case.

In this case, I agree that we should use eviction api in the whole preemption.

[AxeZhan]

ping @alculquicondor @Huang-Wei

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AxeZhan
Once this PR has been reviewed and has the lgtm label, please ask for approval from wojtek-t and additionally assign huang-wei for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS
• keps/sig-scheduling/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[atiratree]

/hold

[atiratree]

Please take a look at the original issue to understand the drive behind this KEP.

Also take a look at this comment to understand the reason behind the eviction first policy.

[AxeZhan]

I still have a question to be clarified:

I think the design details in original KEP conflicts much with current implementation of preemption:
In current implementation, we dry run the preemption to get a list of candidates(each candidate is a struct with a nodeName and a group of victim pods belong to the node).

So how can we achieve this:

If it responses
429 Too Many Requests, the scheduler will output an error log and choose another victim among the candidate victims to preempt
until it succeeds or there are no more candidates.

Suppose dry run shows our best candidate is nodeA with Pod1,Pod2, and Pod3.

So we begin to process preemption:

1. We evict Pod1 with status 200.
2. We evict Pod2, but received a 429, what now? The original KEP says we should choose another victim, but in this case, even if we successfully evict Pod3, we can't make up enough space for preemptor on NodeA. Then what? Can we even revert eviction for Pod1? I don't think so.

I'm not clear of how we should handle this situation:

1. Priority of the preemptor is less than the value of AllowDisruptionByPriorityGreaterThanOrEqual in victims.
2. During dryRun, deleting pod2 will not violate its PDB.
3. Some thing happens, and now deleting pod2 will violate its PDB.
4. Preemption begins, and we received 429 when we trying to evict pod2, and since (1), we can not delete pod2 now also.
5. Then what????

[alculquicondor]

Did you make sure that the PRR is up to date?

[alculquicondor]

Suggested change

	- "AxeZhan"
	- "@AxeZhan"

[alculquicondor]

I can't remember why this was acceptable. Why not use DELETE directly, given that there is a high chance that the /evictions endpoint will just fail?

[atiratree]

Please take a look at #4665 (comment). I think we should properly describe the reasoning in the KEP to make it clear for anyone reading it.

[alculquicondor]

I'm going on vacation today. Perhaps @Huang-Wei can take over?

For the most part, I don't have a problem with the KEP. Last time, however, the implementation was different from what the KEP presented, intending to cover more use cases, so it was rejected.
Please make sure that the design present in the KEP matches your expectations and use cases.

[AxeZhan]

The api definition lgtm. However, I have a lot questions about the original implementations.

For example, this one:
#4665 (comment)

I don't think we can change the victim during preemption midway.

I think we still need to discuss about the implementations. And as the Enhancements Freeze is approaching. I don' think we can bring this in 1.31. We should have more discussion and try introduce it in 1.32 if possible.