-
Notifications
You must be signed in to change notification settings - Fork 630
feat: add PriorityClassName to pod kube_types #12949
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
feat: add PriorityClassName to pod kube_types #12949
Conversation
6301746 to
7dc3005
Compare
7dc3005 to
89b5dcf
Compare
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.
Pull request overview
This PR adds support for the PriorityClassName field to gateway proxy pods to prevent pod preemption in environments with volatile workloads. The implementation follows the established pattern for adding Pod template configuration fields.
Key changes:
- Added
PriorityClassName *stringfield to the Pod struct with getter method - Updated merge logic, deployer values, and Helm templates to propagate the field
- Added CRD schema definition and test coverage
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/kube_types.go | Adds PriorityClassName field and GetPriorityClassName() method to Pod struct |
| pkg/deployer/values.go | Adds PriorityClassName field to HelmGateway struct |
| pkg/deployer/merge.go | Adds merge logic for PriorityClassName using MergePointers helper |
| internal/kgateway/deployer/gateway_parameters.go | Propagates PriorityClassName value from PodTemplate to gateway configuration |
| internal/kgateway/helm/envoy/templates/deployment.yaml | Adds conditional rendering of priorityClassName in deployment spec |
| install/helm/kgateway-crds/templates/gateway.kgateway.dev_gatewayparameters.yaml | Adds priorityClassName to CRD schema |
| pkg/deployer/deployer_test.go | Adds test data and assertion for PriorityClassName field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3378092 to
8ad63fc
Compare
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| {{- if $gateway.priorityClassName }} | ||
| priorityClassName: {{ $gateway.priorityClassName }} |
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.
Let's protect against a user who names a PriorityClass a YAML reserved word with priorityClassName: {{ $gateway.priorityClassName | quote }}
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.
Added in : 9d38407
pkg/deployer/deployer_test.go
Outdated
| }, | ||
| }, | ||
| PodTemplate: &gw2_v1alpha1.Pod{ | ||
| PriorityClassName: ptr.To("default"), |
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.
The only default PCs are system-cluster-critical and system-node-critical. The default PC is really to omit the PC, so why add this? (If someone uses globalDefault in a PC, then presumably they would not use the setting you're adding in this PR.)
Instead, chandler-solo@112764e tests this as end-to-end as is necessary. Please cherry pick it.
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.
Cherry-picked 👍 Thank you for that 🙇♂️
| dst.LivenessProbe = deepMergeProbe(dst.GetLivenessProbe(), src.GetLivenessProbe()) | ||
| dst.TopologySpreadConstraints = DeepMergeSlices(dst.GetTopologySpreadConstraints(), src.GetTopologySpreadConstraints()) | ||
| dst.ExtraVolumes = DeepMergeSlices(dst.GetExtraVolumes(), src.GetExtraVolumes()) | ||
| dst.PriorityClassName = MergePointers(dst.PriorityClassName, src.PriorityClassName) |
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.
Elsewhere we use MergeComparable for some (all?) *string, but that's semantically nonsensical and MergePointers is correct. (I put up #12978 to clean this up.)
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 leave it as it is then 👍
9d38407 to
1592b47
Compare
chandler-solo
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.
We'll need a maintainer to get CI checks running and approve.
Signed-off-by: Pierre-Jean CROUZET <[email protected]>
Signed-off-by: Pierre-Jean CROUZET <[email protected]>
Signed-off-by: Pierre-Jean CROUZET <[email protected]>
Signed-off-by: Pierre-Jean CROUZET <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: Pierre-Jean CROUZET <[email protected]>
Signed-off-by: Pierre-Jean CROUZET <[email protected]>
Signed-off-by: Pierre-Jean CROUZET <[email protected]>
1592b47 to
8b9d561
Compare
Sorry I disappeared. Tests should succeed now |
|
Let’s goooooo |
|
Thanks for the help, the review and the merge ! Cheers. |
Signed-off-by: Pierre-Jean CROUZET <[email protected]> Signed-off-by: David L. Chandler <[email protected]> Co-authored-by: David L. Chandler <[email protected]>
Description
This PR adds support for the
PriorityClassNameattribute for the gateway proxy pods.Right now pods are created with the
defaultPriotityClass. It can be an issue in environments with very volatile workloads (like AWS spots instances) where teams rely on the PriorityClassName to ensure critical pieces of the cluster remain active.With the default PriorityClass, gateway proxy pods are under threat of preemption.
Change Type
Changelog
Additional Notes
closes: #11917