fix(helm): override HINDSIGHT_API_PORT in worker StatefulSet to survive K8s service discovery#2904
Open
Fyko wants to merge 2 commits into
Open
fix(helm): override HINDSIGHT_API_PORT in worker StatefulSet to survive K8s service discovery#2904Fyko wants to merge 2 commits into
Fyko wants to merge 2 commits into
Conversation
Fyko
marked this pull request as ready for review
July 22, 2026 17:26
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The dedicated worker
StatefulSetcrashes on boot inside Kubernetes because it parsesHINDSIGHT_API_PORTas an int, but thehindsight-apiServicemakes Kubernetes inject the legacy service-discovery env varHINDSIGHT_API_PORT=tcp://<clusterIP>:8888into every pod in the namespace.The api Deployment template already guards against this — it explicitly sets
HINDSIGHT_API_PORTto a plain int before ranging.Values.api.env:The worker StatefulSet template had no equivalent, so it inherited the injected
tcp://…string and crashed. This PR adds the same guard to the worker.Root cause
get_config()runsConfig.from_env()at worker startup (hindsight_api/worker/main.py).from_env()unconditionally doesport=int(os.getenv("HINDSIGHT_API_PORT", DEFAULT_PORT))(config.py:1912). When thehindsight-apiService exists, Kubernetes injectsHINDSIGHT_API_PORT=tcp://<clusterIP>:8888into sibling pods, and theint()call raisesValueError.Value choice:
worker.service.targetPort(8889)The worker does not bind
config.port(HINDSIGHT_API_PORT). Its uvicorn health/metrics server bindsargs.http_port, which defaults toconfig.worker_http_port=HINDSIGHT_API_WORKER_HTTP_PORT(default8889, =worker.service.targetPort); the probes hit/healthon8889.config.portis parsed byfrom_env()but never listened on by the worker.So the override only needs to be a parseable int. Mirroring the api template with
{{ .Values.worker.service.targetPort | quote }}keeps the pattern consistent and self-documenting. There is no double-bind risk againstHINDSIGHT_API_WORKER_HTTP_PORTeven though both resolve to8889, because the worker never opens a socket onconfig.port.Validation
helm template ... --set worker.enabled=truenow renders exactly oneHINDSIGHT_API_PORTwith a plain int:No
tcp://, no duplicate.helm lint --set worker.enabled=truepasses (only the pre-existingicon is recommendedinfo).Secondary fix (second commit): de-duplicate worker env keys
The worker template ranged
.Values.api.envthen.Values.worker.envwith no de-dupe, so a key present in both (e.g.HINDSIGHT_API_WORKER_POLL_INTERVAL_MS) emitted a duplicate env entry, which Kubernetes server-side apply rejects. Reproduced before the fix:The two ranges are now merged with
merge (deepCopy worker.env) api.env, so worker-specific values still win on conflict (preserving the prior intended precedence) but only one entry is emitted. Default renders are unchanged. This is a separate, droppable commit if you'd prefer to keep the PR to just the crash fix.Note: no
Chart.yamlversion bump — this repo bumps chart versions viascripts/release.shat release time, not per PR.