feat(sdm): support priorityClassName on the relay and proxy Pods - #98
satdeveloping wants to merge 4 commits into
Conversation
8460d2d to
1780e17
Compare
kellenanker7
left a comment
There was a problem hiding this comment.
this is great, thanks for the contribution!
at the risk of some minor scope creep, could you please make the same change to sdm-proxy as well?
reminder to bump the version in sdm-proxy/Chart.yaml like you did here (I often forget to myself).
|
Thanks! Done — mirrored the same change onto |
Adds an optional strongdm.deployment.priorityClassName value, rendered into the Deployment pod spec alongside the existing nodeSelector/tolerations. Lets operators protect the relay from preemption/eviction under node pressure (e.g. system-cluster-critical), which matters because losing the relay means losing brokered cluster access. Empty by default, so existing installs are unchanged. values.schema.json and values.test.yaml updated; chart bumped 2.6.4 -> 2.7.0 (minor).
Mirrors the sdm-relay change for the proxy chart. Adds an optional strongdm.deployment.priorityClassName value, rendered into the Deployment pod spec alongside the existing nodeSelector/tolerations. Lets operators protect the proxy from preemption/eviction under node pressure (e.g. system-cluster-critical). Empty by default, so existing installs are unchanged. values.schema.json and values.test.yaml updated; chart bumped 2.7.6 -> 2.8.0 (minor).
4d0422d to
c62ea8b
Compare
| serviceAccountName: {{ include "strongdm.serviceAccountName" . }} | ||
| {{- end }} | ||
| terminationGracePeriodSeconds: 10 | ||
| {{- with .Values.strongdm.deployment.priorityClassName }} |
There was a problem hiding this comment.
sorry for the nit, but one last request for consistency with the other fields.
could you make these simply:
priorityClassName: {{ .Values.strongdm.deployment.priorityClassName }}the default "" is valid if applied server-side 👍
| {{- end }} | ||
| terminationGracePeriodSeconds: 10 | ||
| {{- with .Values.strongdm.deployment.priorityClassName }} | ||
| priorityClassName: {{ . }} |
There was a problem hiding this comment.
same as the other comment!
|
@satdeveloping sorry for the delay. I resolved some merge conflicts and requested one last nitpick change. |
Summary
Adds an optional
strongdm.deployment.priorityClassNamevalue to the sdm-relay and sdm-proxy charts, rendered into the Deployment's pod spec next to the existingnodeSelector/tolerations.Motivation
There's currently no way to set a PriorityClass on the relay or proxy Pod via the chart. On a busy cluster these Pods can be preempted or left
Pendingunder node pressure — and because they broker access to in-cluster resources, losing one means losing that access exactly when you're most likely to need it (e.g. during a node roll or an incident). Being able to set e.g.system-cluster-critical(or a custom high-priority class) lets operators protect them.Changes
Applied identically to both
deployments/sdm-relayanddeployments/sdm-proxy:values.yaml— newstrongdm.deployment.priorityClassName(with@schemadescription).templates/deployment.yaml— renderspriorityClassNameonly when set ({{- with … }}).values.schema.json— added the matchingstringproperty underdeployment.values.test.yaml— exercises the new value.Chart.yaml— relay2.6.4→2.7.0, proxy2.7.6→2.8.0(minor: additive, opt-in feature).Backward compatibility
Fully backward-compatible. The value defaults to
"", and the template omits the field entirely when empty, so existing installs render byte-for-byte identically.Testing
Both charts, e.g. for
sdm-proxy:(Equivalent commands pass for
sdm-relaywithstrongdm.auth.relayToken.) Values validate against the updatedvalues.schema.json(helm enforces it during templating).