feat(deployment): gateway/dataservice feature parity#170
feat(deployment): gateway/dataservice feature parity#170camjay wants to merge 3 commits intoPortkey-AI:mainfrom
Conversation
camjay
force-pushed
the
feat/gateway-dataservice-parity
branch
from
c453f22 to
3ddcbc4
Compare
Brings gateway and dataservice to parity on deployment features that already exist on one component but not the other: Gateway (new — already exists on dataservice): - envFrom: bulk inject env vars from secrets/configmaps - extraEnv: additional env vars after commonEnv - startupProbe: configurable startup health check Dataservice (new — gateway already has these via Portkey-AI#156): - envFrom: same pattern as gateway - terminationGracePeriodSeconds: configurable grace period All default to empty/disabled. No behavioral change when unconfigured.
camjay
force-pushed
the
feat/gateway-dataservice-parity
branch
from
3ddcbc4 to
2808472
Compare
camjay
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the portkey-gateway Helm chart to align Gateway and Dataservice deployment configurability (env injection, probes, and termination behavior) so both components support the same operational features.
Changes:
- Add
envFromandextraEnvsupport for the Gateway container. - Add configurable
startupProbesupport for the Gateway container. - Add
envFromand optionalterminationGracePeriodSecondssupport for the Dataservice deployment.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| charts/portkey-gateway/values.yaml | Adds new values (envFrom, extraEnv, dataservice envFrom) and notes dataservice termination grace period configuration. |
| charts/portkey-gateway/templates/gateway/deployment.yaml | Renders envFrom, appends extraEnv, and conditionally renders startupProbe for Gateway. |
| charts/portkey-gateway/templates/dataservice/deployment.yaml | Adds optional terminationGracePeriodSeconds and envFrom rendering for Dataservice. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| deployment: | ||
| autoRestart: true | ||
| replicas: 1 | ||
| # terminationGracePeriodSeconds: 30 |
There was a problem hiding this comment.
dataservice.deployment.terminationGracePeriodSeconds is supported in the template, but values.yaml only contains a commented example and no actual key. Define terminationGracePeriodSeconds: under dataservice.deployment (empty by default) so it’s discoverable and can be set without relying on implicit/undocumented values.
| # terminationGracePeriodSeconds: 30 | |
| terminationGracePeriodSeconds: |
There was a problem hiding this comment.
Intentional — this mirrors how the gateway template already handles it (added in #156). The commented example documents the field without setting a default that would override Kubernetes's own 30s default.
| {{- with .Values.startupProbe }} | ||
| startupProbe: | ||
| {{- toYaml . | nindent 12 }} | ||
| {{- end }} |
There was a problem hiding this comment.
startupProbe is rendered from .Values.startupProbe, but values.yaml doesn’t define/document this value alongside the existing livenessProbe/readinessProbe settings. Add a startupProbe: section (or commented example) to values.yaml so users can discover/configure it consistently.
There was a problem hiding this comment.
Added a commented example in values.yaml in 3593ac8.
| # envFrom allows loading all keys from secrets/configmaps as environment variables. | ||
| # Useful for external secret managers (ESO, Vault CSI, AWS Secrets Manager). | ||
| # Variables in env (from environment.data) take precedence over envFrom. | ||
| envFrom: [] | ||
|
|
||
| # Additional environment variables appended after commonEnv. | ||
| extraEnv: [] |
There was a problem hiding this comment.
The chart now supports envFrom/extraEnv and (in the template) startupProbe, but startupProbe isn’t present in values.yaml near the other probe settings. Add a startupProbe: block (disabled by default) to keep values.yaml in sync with the template and improve discoverability.
There was a problem hiding this comment.
Same — added in 3593ac8 as a commented example, consistent with how livenessProbe and readinessProbe are already documented in this file.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
LGTM |
Brings gateway and dataservice to parity on deployment features that already exist on one component but not the other.
Gateway (new — already exists on dataservice):
envFrom: bulk inject env vars from secrets/configmaps (for ESO, Vault CSI, etc.)extraEnv: additional env vars after commonEnvstartupProbe: configurable startup health checkDataservice (new — gateway already has
terminationGracePeriodSecondsvia #156):envFrom: same pattern as gatewayterminationGracePeriodSeconds: configurable grace period for background job completionAll default to empty/disabled. No behavioral change when unconfigured.
We run Portkey Gateway in production on EKS and have been maintaining these patches in a fork. Upstreaming to reduce maintenance divergence.
See also #173 which adds CI to validate chart templates on PRs.