-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Chart: Add resize policy. #13906
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
Chart: Add resize policy. #13906
Conversation
✅ Deploy Preview for kubernetes-ingress-nginx canceled.
|
|
|
|
Welcome @younsl! |
|
Hi @younsl. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
This PR is similar to oauth2-proxy/manifests#346 |
Gacko
left a comment
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.
Since this feature is supported from v1.30 upwards only, I'd ask you to wrap it into a condition to check if the target cluster supports it or not.
Gacko
left a comment
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.
Please do not re-request my review without having changed anything. Thank you! 😄
|
@Gacko Thanks for the feedback! I've updated the implementation to include Kubernetes version checks for the |
|
@Gacko PTAL |
Gacko
left a comment
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.
I went through your PR once again. In general I feel like you didn't dig into this project a lot and just want this field in the chart. While I appreciate your effort, I'd still ask you to get to know this project a bit better, because ultimately it's on the maintainers to further support your change in the future.
This feature is being introduced in v1.33 as a beta feature, which is enabled by default. Please update the capability checks accordingly.
Last but not least the admission webhook patch jobs are short-living, normally no longer than 5 seconds. I can barely imagine a resize to happen or make sense in this short time. So please either remove the property for the admission webhook patch job or elaborate on why you think we need this.
charts/ingress-nginx/templates/admission-webhooks/job-patch/job-createSecret.yaml
Outdated
Show resolved
Hide resolved
charts/ingress-nginx/templates/admission-webhooks/job-patch/job-patchWebhook.yaml
Outdated
Show resolved
Hide resolved
charts/ingress-nginx/tests/admission-webhooks/job-patch/job-createSecret_test.yaml
Outdated
Show resolved
Hide resolved
|
/retitle Chart: Add resize policy. |
|
@Gacko Thank you for the thorough review. I've addressed all your feedback:
|
Gacko
left a comment
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.
I left another round of things to change.
charts/ingress-nginx/tests/admission-webhooks/job-patch/job-createSecret_test.yaml
Outdated
Show resolved
Hide resolved
charts/ingress-nginx/tests/admission-webhooks/job-patch/job-patchWebhook_test.yaml
Outdated
Show resolved
Hide resolved
7f5e6d6 to
7362cca
Compare
|
@Gacko PTAL. Apply your all suggestion |
|
@Gacko Thank you so much for the thorough and constructive feedback. I really appreciate your patience and detailed guidance. I apologize for not being more familiar with the codebase initially - your review is helping me understand the project much better. I'll carefully address all your comments and learn from this experience. |
d30dd75 to
5f489ec
Compare
Gacko
left a comment
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.
/triage accepted
/kind feature
/priority backlog
/lgtm
I rebased your branch on top of main, just in case you wonder about the force push. Also I squashed your commits. Thank you very much for your endurance and efforts! 🙂
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Gacko, younsl 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 |
|
/unhold |
What this PR does / why we need it:
This PR adds support for Kubernetes resizePolicy configuration to the ingress-nginx Helm chart. The resizePolicy feature allows containers to be resized without pod restarts, enabling dynamic resource adjustments for better resource utilization and cost efficiency.
The implementation adds resizePolicy support for:
Related Kubernetes Documentation
Types of changes
Which issue/s this PR fixes
How Has This Been Tested?
Checklist: