-
Notifications
You must be signed in to change notification settings - Fork 568
Add Pod Disruption Budget and configurable deployment name #1404
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
Conversation
Hi! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I also place myself in the waiting queue :)
Hey team! PDB is very important configuration to have exposed by the chart! Can we get this merged ? I think this PR just needs a rebase and should be good to be merged. Unfortunately I can't rebase it myself. @omerap12 Can you rebase and lets work on getting this merged ? 🙏🏻 |
I can help in merge, @omerap12 can you you rebase it? |
@d-nishi @mskanth972 Sorry to bother guys! Will appreciate if we can get this merged 🙏🏻 It is backward compatible and should be OK to merge IMO 🙏🏻 |
Sure. Ill merge it today |
@@ -0,0 +1,19 @@ | |||
{{- if .Values.controller.pdb }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@omerap12 - As most of community controllers use podDisruptionBudget
instead of pdb
, can we use the same standard for consistency?
Should we also set defaults for minAvailable
and maxUnavailable
similar to how its set in ebs controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Ill adjust.
@@ -101,6 +103,8 @@ controller: | |||
# eks.amazonaws.com/role-arn: arn:aws:iam::111122223333:role/efs-csi-role | |||
healthPort: 9909 | |||
regionalStsEndpoints: false | |||
# Pod Disruption Budget | |||
pdb: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@omerap12 - What about enabling this as default from the chart similar to ebs
controller, unless we like to keep it backward compatible and not surprisingly have it enabled.
https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/50f6b2e68446b221bde01fbd3aca190ea9a0e569/charts/aws-ebs-csi-driver/values.yaml#L250-L254
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this feature doesn't exist. In my opinion, it would be best to disable it by default and only enable it by default once it is well-received by the users.
I don't think you can merge it after rebase based on the @k8s-ci-robot comment above,
|
Yes, that what I meant :) |
@deepak-kosaraju PTAL |
Are you sure? The chart version is 3.1.1 at the moment.. |
Please remove that, we can add while making a release. |
Done |
Sorry, can you please squash all the commits to one. |
Done |
/lgtm |
Thanks! by the way I have this PR ( from long time ago ): #1351 |
Please check my comments before merging :) |
/hold |
commit 2dfd178 Author: Omer Aplatony <[email protected]> Date: Tue Nov 26 17:07:14 2024 +0200 revert Signed-off-by: Omer Aplatony <[email protected]> commit 3b7dbbd Author: Omer Aplatony <[email protected]> Date: Tue Nov 26 09:16:04 2024 +0200 Set default values Signed-off-by: Omer Aplatony <[email protected]> commit 508e32a Merge: a943df0 c963295 Author: Omer Aplatony <[email protected]> Date: Tue Nov 26 09:04:32 2024 +0200 Merged master Signed-off-by: Omer Aplatony <[email protected]> commit a943df0 Author: Omer Aplatony <[email protected]> Date: Sun Jul 7 10:06:27 2024 +0300 Add Pod Disruption Budget and configurable deployment name Signed-off-by: Omer Aplatony <[email protected]> Signed-off-by: Omer Aplatony <[email protected]> Apply suggestions from code review Co-authored-by: Sherif Abdel-Naby <[email protected]> Signed-off-by: Omer Aplatony <[email protected]>
PTAL @sherifabdlnaby |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mkilchhofer, mskanth972, omerap12, sherifabdlnaby The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ping @mskanth972 |
/unhold |
/lgtm |
@mskanth972 and @omerap12 I was about to submit review comment, and noticed its merged so added this comment. |
|
Is this a bug fix or adding new feature?
Re-raising this PR #901. set configurable deployment and pdb name.
What is this PR about? / Why do we need it?
Fixes #1397
What testing is done?
Helm testing: the following values file:
provides this output: