Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add precautionary logic for PDB #436

Merged
merged 2 commits into from
Jul 27, 2023
Merged

Conversation

krithika369
Copy link
Collaborator

What this PR does / why we need it:

PDB support was introduced in #420. In some scenarios, the PDB can create problems. As an example, if PDB is configured to have a minimum of 20% replicas available and there is only one replica in the deployment, it's not possible to drain that node. To prevent such issues, this PR introduces some precautionary checks, - ensuring that at least 1 replica of the deployment can be removed:

Let
===
# replicas: r
minAvailablePercent / 100: p

We want
=======
r - ceil(r * p) >= 1 // ceil because k8s rounds up to the nearest int
=> ceil(r * p) + 1 <= r
=> ceil(r * p) < r // We can say this because all terms in the above inequality are int

It's enough to check ceil(r * p) < r where r is the minReplicas because:

r - ceil(r * p) >= 1
=> r * (1 - ceil(p)) >= 1

We can see that, as r increases, LHS will increase. So, it's enough to ensure this inequality holds for the smallest possible value of r, which is the minReplicas configured for the deployment.

Does this PR introduce a user-facing change?:

Disable PDB when the min replicas is very low, so that it doesn't block node drain.

Checklist

  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduce API changes

@@ -232,7 +232,7 @@ func (c *controller) Deploy(ctx context.Context, modelService *models.Service) (
}

if c.deploymentConfig.PodDisruptionBudget.Enabled {
pdbs := c.createPodDisruptionBudgets(modelService, c.deploymentConfig.PodDisruptionBudget)
pdbs := createPodDisruptionBudgets(modelService, c.deploymentConfig.PodDisruptionBudget)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing createPodDisruptionBudgets from the controller so that it's easy to unit test it.

@krithika369 krithika369 added the bug Something isn't working label Jul 27, 2023
Copy link
Contributor

@ariefrahmansyah ariefrahmansyah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for making it better, Krithika!

@ariefrahmansyah ariefrahmansyah merged commit e133eb7 into main Jul 27, 2023
42 checks passed
@ariefrahmansyah ariefrahmansyah deleted the fix_pdb_condition branch July 27, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants