feat: Eviction controller

[Arvindthiru]

Description of your changes

Fixes #

I have:

• Run make reviewable to ensure this PR is ready for review.

How has this code been tested

UTs and ITs

Special notes for your reviewer

Changes to rollout controller needs to be present to add E2E tests

[michaelawyu]

Added some comments, PTAL 🙏

[michaelawyu]

Arvind and I had a discussion about some of the finer points; just to scribble down some of the thoughts/points we went through here:

• On the issue about whether eviction on a binding that has been marked for deletion should be considered as invalid, since this is an issue with no substantial effect (i.e., the binding is already gone and there's nothing anyone can do with it), our discussion was mostly on the logical/UX front. Both ways should be fine; the only minor concern is that, if we mark such evictions as invalid, people would need to look at the reason/message part to realize that this is a situation without solutions; it appears like an error, but it has no fix. Besides, K8s does not block repeated delete calls either. For these reasons, I am wondering if it would be OK for us to mark it as valid but with a reason/message that suggests the already deleted observation; this would seem to be easier for users to comprehend.

• For the second concern about whether we need to account for target numbers (e.g., the N number of PickN CRPs) for DB's maxUnavailable/minAvailable calculations, I have to admit that this seems to be an issue where we could never have a perfect solution 😣; specifically:

  • For downscaling scenarios, accounting for target numbers would yield a lower bound for maxUnavailable and higher bound for minAvailable, which does offer better protection; however,
  • For upscaling and insufficient number of picked cluster (5 clusters wanted, 4 scheduled) scenarios, accounting for target numbers would yield higher bound for maxUnavailable and lower bound for minAvailable, which weakens the protection.
  • Of course we could choose to honor the most strict bound applicable, but this would be a system that's fairly difficult for users to comprehend. Waiting for the up/down-scaling to complete could also help, but the scaling itself might also get stuck -> without eviction it might be tricky to do it right and unblock the impasse.

  With these being said, I am wondering if it would make sense for us to start with the simple model, like K8s' PDB does, which concerns only the observed count of bindings; this has its own disadvantages (e.g., limited protection when downscaling), but it excels at its ease to understand (this might be esp. true considering that our APIs has been quite complicated). We could always build upon that (like K8s does with 1.31's unhealthyEvictionPolicy) if the users demand so. Another reason why I am a bit inclined towards this is that, if the user would like to have best protection, they could always switch to minAvailable w/ absolute count in the DB -> the dichotomy between maxUnavailable and minAvailable already offers users a choose between flexibility and protection, so it might be OK if we don't add another layer of consideration.

[michaelawyu]

Just a few minor comments; LGTM ;)

[ryanzhang-oss]

what happens if one is deleting while the other is bound/scheduled?

[Arvindthiru]

Marking eviction as invalid for now, the user can retry once the deleting eviction is removed

[michaelawyu]

I've taken a closer look at the implementation for the K8s disruption controller, and given it a try in a real environment:

• Currently there's no support for pods under the management of a DaemonSet (no support for the scale sub-resource); PDB will not function (unless integer based minAvailable setting is applied), and any eviction attempt would fail (complaining that PDB is not yet ready).

(I was always under the impression that K8s PDB is a selector based mechanism -> I even have some very vivid recollection about this gained during prev. on-call experiences; that wasn't right at all -> though the API certainly suggests so, under the hood it basically assumes single ownership per pod with the support for multiple owners protected by one DB -> not sure how useful that case would be though. Well, learned something new every day 😑)

To summarize the model we could have if we would like to keep things analogous to the K8s model:

• PickAll CRPs can only work with integer-based minAvailable DBs; other types are invalid
• PickN and PickFixed CRPs can work with either (maxUnavailable/minAvailable, pct or integer-based) DB configs