Skip to content

PINF-816: default readinessProbe on git-sync-relay's git-sync container - #764

Open
danielhoherd wants to merge 3 commits into
masterfrom
danielhoherd/PINF-816-gitsync-readiness-probe
Open

PINF-816: default readinessProbe on git-sync-relay's git-sync container#764
danielhoherd wants to merge 3 commits into
masterfrom
danielhoherd/PINF-816-gitsync-readiness-probe

Conversation

@danielhoherd

Copy link
Copy Markdown
Member

Description

PINF-816: companion chart-side change to git-sync-relay's readiness-tracking fix (astronomer/git-sync-relay#76): that PR makes /readyz reflect the relay's most recent sync attempt instead of only its first one at startup, but nothing wired that endpoint to an actual k8s probe — the gitSyncRelay.gitSync container had no readinessProbe/livenessProbe default at all (both {} in values.yaml).

Without a probe, a Deployment's Gitsync URL edited to something invalid produced no k8s-visible signal: the relay pod stayed Ready forever once its first sync had succeeded, silently failing every later poll/webhook cycle in the background.

Fix: a hardcoded readinessProbe fallback (httpGet /readyz on gitSync.webhookPort), following the same {{- if <values> }}...{{- else }}<default>{{- end }} pattern this template already uses for the git-daemon container's own liveness/startup defaults. Still fully overridable via gitSyncRelay.gitSync.readinessProbe.

Deliberately readinessProbe only, not livenessProbe: restarting the pod cannot fix a bad repo URL or revoked credentials, so tying liveness to sync health would just crash-loop the pod pointlessly instead of surfacing the actual problem.

This closes the loop with commander's existing WaitForDeployment/WaitForReady mechanism with no commander-side changes needed: it already discovers Deployments by release=<name>,tier=airflow (this Deployment already carries that label) and blocks/times out (default 300s) on status.readyReplicas < desired, which depends on pod readinessProbe passing. So a bad URL now makes this readinessProbe fail → readyReplicas drops → WaitForDeployment times out → commander reports a real, named deploy failure instead of a silently-degraded pod.

Kubelet probe traffic bypasses this chart's restrictive git-sync-relay NetworkPolicy (probes go from node to pod directly, never through the CNI's policy-enforced path) — confirmed this is standard behavior, not something needing an explicit allow-rule here.

Related Issues

  • PINF-816
  • Companion PR: astronomer/git-sync-relay#76 (the actual /readyz logic this probe depends on)

Testing

  • tests/chart/test_git_sync_relay_deployment.py's default-values test updated to assert the new readinessProbe body instead of asserting its absence; the existing "no hardcoded fallback, customer-configurable only" comment/assertions for livenessProbe/startupProbe are unchanged (still true — only readinessProbe gets a default). The explicit-override test elsewhere in the same file (which sets readinessProbe/livenessProbe directly) is unaffected since it never hits the new {{- else }} branch.
  • uv run pytest tests/chart/test_git_sync_relay_deployment.py — 155 passed.
  • uv run pytest tests/chart/ (full suite) — 1090 passed, 46 skipped (pre-existing).

Companion chart-side change to git-sync-relay's readiness-tracking fix
(danielhoherd/PINF-816-gitsync-readiness-tracking): that PR makes /readyz
reflect the relay's most recent sync attempt instead of only its first one
at startup, but nothing wired that endpoint to an actual k8s probe -- the
gitSyncRelay.gitSync container had no readinessProbe/livenessProbe
default at all (both {} in values.yaml).

Without a probe, a Deployment's Gitsync URL edited to something invalid
produced no k8s-visible signal: the relay pod stayed Ready forever once
its first sync had succeeded, silently failing every later poll/webhook
cycle in the background.

Fix: a hardcoded readinessProbe fallback (httpGet /readyz on
gitSync.webhookPort), following the same
{{- if <values> }}...{{- else }}<default>{{- end }} pattern this template
already uses for the git-daemon container's own liveness/startup
defaults. Still fully overridable via
gitSyncRelay.gitSync.readinessProbe, same as before.

Deliberately readinessProbe only, not livenessProbe: restarting the pod
cannot fix a bad repo URL or revoked credentials, so tying liveness to
sync health would just crash-loop the pod pointlessly instead of
surfacing the actual problem.

This closes the loop with commander's EXISTING WaitForDeployment/
WaitForReady mechanism with no commander-side changes needed: it already
discovers Deployments by `release=<name>,tier=airflow` (this Deployment
already carries that label) and blocks/times out (default 300s) on
status.readyReplicas < desired, which depends on pod readinessProbe
passing. So a bad URL now makes this readinessProbe fail -> readyReplicas
drops -> WaitForDeployment times out -> commander reports a real, named
deploy failure instead of a silently-degraded pod.

Kubelet probe traffic bypasses this chart's restrictive git-sync-relay
NetworkPolicy (probes go from node to pod directly, never through the
CNI's policy-enforced path) -- confirmed this is standard behavior, not
something needing an explicit allow-rule here.

Tests: tests/chart/test_git_sync_relay_deployment.py's default-values
test updated to assert the new readinessProbe body instead of asserting
its absence; the existing "no hardcoded fallback, customer-configurable
only" comment/assertions for livenessProbe/startupProbe are unchanged
(still true -- only readinessProbe gets a default). The explicit-override
test elsewhere in the same file (which sets readinessProbe/livenessProbe
directly) is unaffected since it never hits the new {{- else }} branch.

Verified: uv run pytest tests/chart/test_git_sync_relay_deployment.py --
155 passed. Full uv run pytest tests/chart/ suite also run for
regressions.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

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 a default Kubernetes readinessProbe to the umbrella chart’s git-sync-relay git-sync container so that the relay’s /readyz health signal (now reflecting the most recent sync attempt) is actually wired into Deployment readiness, allowing failed syncs (e.g., bad repo URL) to surface as NotReady rather than silently remaining Ready.

Changes:

  • Add a hardcoded readinessProbe fallback (httpGet /readyz on gitSyncRelay.gitSync.webhookPort) when gitSyncRelay.gitSync.readinessProbe is unset/empty.
  • Update the chart render test to assert the new default readinessProbe and keep liveness/startup behavior customer-configurable-only.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
templates/git-sync-relay/git-sync-relay-deployment.yaml Adds a default readinessProbe fallback for the git-sync container pointing at /readyz.
tests/chart/test_git_sync_relay_deployment.py Updates default-values assertions to expect the new git-sync readinessProbe.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Caught in review: failureThreshold: 3 with periodSeconds: 15 meant up to
45s of delay after a sync actually failed before kubelet would mark the
pod NotReady -- a second, redundant smoothing layer stacked on top of
sync_logic()'s own already-debounced failure signal (last_sync_success
only flips after _fetch_with_retry exhausts its own retries/backoff).
That works against the 'fail fast, clearly' point of this fix.

failureThreshold: 1 matches the design this PR (and git-sync-relay#76)
was actually built around: a single failed sync cycle should be enough,
since it's not a fleeting blip by the time it's observed. A bare
probe-request failure (as opposed to a real 503) should be rare -- /readyz
is a lightweight in-memory read that doesn't block on the sync itself,
which runs in a separate executor thread.

Verified: uv run pytest tests/chart/test_git_sync_relay_deployment.py --
155 passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment thread templates/git-sync-relay/git-sync-relay-deployment.yaml Outdated
No behavior change -- readability only. The readinessProbe default's
comment block had grown to 13 lines repeating the PR description's full
rationale inline; cut to the few lines a reader needs to know it's
deliberate (not a livenessProbe, failureThreshold: 1 is intentional).
Same for the matching test comment.

Verified: uv run pytest tests/chart/test_git_sync_relay_deployment.py --
155 passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants