-
Notifications
You must be signed in to change notification settings - Fork 148
feat(chart): add priorityClassName for control plane pod #4356
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @starlightromero! Welcome to the project! 🎉 Thanks for opening this pull request! Please make sure to include the issue number in the PR description to automatically close the issue when the PR is merged. |
|
✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
|
I have hereby read the F5 CLA and agree to its terms |
sjberman
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.
Thanks for the contribution!
2d4745d to
4f9726c
Compare
|
Hey @starlightromero, looks like there are still some generated files that weren't updated. When you get the chance, can you run |
4f9726c to
4621258
Compare
|
@bjee19 I rebased and then ran |
|
Oops I'm sorry I gave you the wrong command, could you run |
4621258 to
0b05975
Compare
0b05975 to
5d2642c
Compare
| name: nginx-agent-tls | ||
| nodeSelector: | ||
| kubernetes.io/os: linux | ||
| priorityClassName: null |
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.
Will this deploy okay and be ignored by k8s?
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.
Unfortunately, this will not be okay. I can manually update it to an empty string (""). However, does the make command need to be updated as well?
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.
We could just make this field conditional in the template, so that it's only rendered if it's specified. That way by default it isn't rendered at all.
Proposed changes
Problem: The user is unable to set a priorityClassName for control plane pods. This could mean that the scheduler prioritizes other pods which rely on Nginx Gateway Fabric
Solution: Add a field to the Helm Chart to allow the user to set the priorityClassName for control plane pods.
Testing: Ran
make lint-helm,make generate-helm-schema, andmake generate-helm-docsChecklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.