fiaas / fiaas-deploy-daemon

fiaas-deploy-daemon is the core component of the FIAAS platform
https://fiaas.github.io/
Apache License 2.0
55 stars 31 forks source link

Add PodDisruptionBudget support #220

Closed tg90nor closed 7 months ago

tg90nor commented 9 months ago

Having PDB support in FIAAS should allow us to auto upgrade nodepools and turn on cluster autoscaling without causing unacceptable levels of disruption.

My current plan is to have a default of minAvailable=replicas.min, with an option to explicitly set minAvailable or maxUnavailable, but I'm open to input on this.

Depends on fiaas/k8s#125

herodes1991 commented 9 months ago

I do not know if this can create problems with Singleton Deployments with only 1 replica (min & max) 😅

Also, I do not know how it will behave if the min.replicas = max.replicas 🤔

miguelbernadi commented 9 months ago

My takes :

oyvindio commented 9 months ago

Good points in the above comments!

  • Singletons (or apps with a single replica) should not have a PDB by default, as that blocks regular operations in the cluster.

I definitely agree on this point; if replicas.min or replicas.max is 1 there should probably not be a PDB for the application.

  • minAvailable requires knowing the number of replicas, and when the application scales in can have the same issues as a single replica deployment.

I think I may have originally suggested setting minAvailable=replicas.min in our internal discussions earlier, with the reasoning that it would be intuitive to the user that replicas.min should be available. I see that there may be some issues with that configuration though: As you say, when the application is scaled down to replicas.min, minAvailable would be equal to the actual/desired replicas, and it might not be possible to evict any pods within the PodDisruptionBudget.

  • maxUnavailable=1 is the most conservative, ensured to be correct (but not optimal) for any workload with more than 1 replica. I'd go with this for a default.

I think maxUnavailable=1 could be a reasonable default (with a recommendation for users to set a higher value for applications that can tolerate it).

Another option I can think of might be to (for applications with >1 replicas) set minAvailable or maxUnavailable to some percentage, e.g. 50%. That would be less conservative, also not optimal, not ensured to be correct (in the sense that it generally avoids service disruptions), but may act as an incentive to set a more accurate value. I'm not saying this is my preferred option, just mentioning it here for completeness.

blockjesper commented 9 months ago

Yet another option could be to set maxUnavailable to 10% (rather than 1 or 50%). I value of 10% would allow for draining nodes even when some pods are unavailable (say for a large replicaset of a 100 pods, then it's ok if 10 pods would be unavailable). This might be a more balanced approach.

tg90nor commented 9 months ago

Thanks for all the input! Apps with single replica should indeed not have a PDB. I want to go with the conservative approach of maxUnavailable=1, and have it not be configurable for the first iteration. I want to see if this results in acceptable eviction rates, or if further tuning is needed.