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

chore: Added new policy to PodDisruptionBudget #7037

Closed
wants to merge 2 commits into from

Conversation

peterabarr
Copy link

@peterabarr peterabarr commented Sep 18, 2024

Fixes #7025

Description

Added the unhealthyPodEvictionPolicy to the Karpenter PDB.

How was this change tested?

I generated a helm template file locally and tried to set unhealthyPodEvictionPolicy in the chart values to AlwaysAllow:
Screenshot 2024-09-18 at 13 07 13

I also tried not setting anything in the chart values and expected that there would be no mention of unhealthyPodEvictionPolicy in the spec:
Screenshot 2024-09-18 at 13 09 35

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

netlify bot commented Sep 18, 2024

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit 892b5a1
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/67372e4cab337800081e9145
😎 Deploy Preview https://deploy-preview-7037--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

So this is only available in beta starting in 1.27. What happens with users who try to define this on 1.25/1.26? Is there some way we can just not include this based on the kubernetes version?

@peterabarr
Copy link
Author

@njtran Good point, I've added a function that ensures it only adds the field if the Kubernetes version is >= 1.27.

@peterabarr peterabarr force-pushed the feat-pdb-policy branch 5 times, most recently from e2d8e73 to 464f2c8 Compare October 7, 2024 12:55
@peterabarr peterabarr force-pushed the feat-pdb-policy branch 2 times, most recently from fd0de9a to 644339e Compare October 14, 2024 08:37
@@ -49,6 +49,7 @@ podAnnotations: {}
podDisruptionBudget:
name: karpenter
maxUnavailable: 1
unhealthyPodEvictionPolicy:
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that we should set a default policy in our values.yaml if we are going to support this configuration parameter

@@ -70,6 +70,7 @@ cosign verify public.ecr.aws/karpenter/karpenter:1.0.0 \
| podAnnotations | object | `{}` | Additional annotations for the pod. |
| podDisruptionBudget.maxUnavailable | int | `1` | |
| podDisruptionBudget.name | string | `"karpenter"` | |
| podDisruptionBudget.unhealthyPodEvictionPolicy | string | `nil` | Unhealthy pod eviction policy for the PDB. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I question if we need this? What's the need for us to allow folks to configure this? Do you have a use-case for wanting to change this from the default eviction policy?

Copy link
Contributor

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for user PDB configuration so new fields can be added
3 participants