Conversation
bad1539 to
ec4fd7d
Compare
rooftopcellist
left a comment
There was a problem hiding this comment.
Left inline comments on two issues found while cross-checking this against the probe research from #282 review (see gitlab.cee.redhat.com/-/snippets/12186).
| livenessProbe: | ||
| httpGet: | ||
| path: "/{{ pulp_combined_settings.galaxy_api_path_prefix }}/pulp/api/v3/status/" | ||
| path: "/{{ pulp_combined_settings.galaxy_api_path_prefix }}/status/healthz" |
There was a problem hiding this comment.
This resolves to /api/galaxy/status/healthz, which 404s. galaxy_ng/app/urls.py registers healthz as a top-level route (a sibling of the {API_PATH_PREFIX}/ include), not nested under it, and there's no status/ segment anywhere in the source. Confirmed independently via galaxy_ng's own Clowder deployment (openshift/clowder/clowd-app.yaml), which probes plain /healthz with no prefix.
Suggested fix:
path: "/healthz"As written, liveness will fail continuously once initialDelaySeconds elapses → restart-loop on rollout.
There was a problem hiding this comment.
@rooftopcellist we added /api/galaxy/status/healthz in https://github.com/ansible-automation-platform/galaxy_ng/pull/561 because the /healthz wasn't reachable in the 2.7 deployments, where it's through gateway only
| livenessProbe: | ||
| httpGet: | ||
| path: "/{{ pulp_combined_settings.galaxy_api_path_prefix }}/pulp/api/v3/status/" | ||
| path: "/{{ pulp_combined_settings.galaxy_api_path_prefix }}/status/healthz" |
There was a problem hiding this comment.
Same issue as galaxy-api's livenessProbe: /{{ pulp_combined_settings.galaxy_api_path_prefix }}/status/healthz doesn't exist. The nginx configmap (galaxy-web.configmap.yaml.j2) has no location block matching this path, so it falls through to the catch-all location / and hits Django's 404 the same way.
Suggested fix:
path: "/healthz"| readinessProbe: | ||
| httpGet: | ||
| path: "/{{ pulp_combined_settings.galaxy_api_path_prefix }}/pulp/api/v3/status/" | ||
| path: "/{{ pulp_combined_settings.galaxy_api_path_prefix }}/status/healthz" |
There was a problem hiding this comment.
This readinessProbe wasn't restored to the deep status check the way galaxy-api's was (line 221 there is now pulp/api/v3/status/). Per the PR description and Dylan's ask in Slack, readiness should stay on the DB-aware check for both pods so a pod that can't reach the DB doesn't get marked Ready. Right now this is inconsistent with galaxy-api, and (combined with the healthz path bug above) this pod would never become Ready at all.
Suggested fix:
path: "/{{ pulp_combined_settings.galaxy_api_path_prefix }}/pulp/api/v3/status/"| @@ -209,7 +209,7 @@ spec: | |||
| containerPort: 8000 | |||
| livenessProbe: | |||
There was a problem hiding this comment.
Suggestion, non-blocking: consider adding a startupProbe (httpGet: /healthz, generous failureThreshold) here so initialDelaySeconds: 90 on liveness doesn't have to keep guessing a value large enough to survive migrations/cold start. Raised in the #282 review thread, not required for this fix.
e41fc43 to
67934c4
Compare
SUMMARY
Issue: https://redhat.atlassian.net/browse/AAP-88905
Builds on #282 by splitting the liveness and readiness probes on the galaxy-api pod
to use different endpoints appropriate to each probe's purpose.
livenessProbe: keeps
/api/galaxy/status/healthz(lightweight, returns 200 OK,no DB/redis/worker checks). This prevents a slow database from triggering a pod
restart loop — a live-but-slow pod should not be killed.
readinessProbe: restores
/api/galaxy/pulp/api/v3/status/so that k8s does notroute traffic to a pod that cannot reach the database. The
timeoutSecondsisincreased from 5 to 10 to give more headroom on instances with large job history.
This probe only runs at pod startup so a slower check is acceptable.
ADDITIONAL INFORMATION
The split was discussed in review of #282 — the consensus from reviewers was: