Skip to content

fix(orchestrator): losing pod logs must not fail an execution - #325

Merged
morgan-wowk merged 1 commit into
masterfrom
08-12-fix_orchestrator_losing_pod_logs_must_not_fail_an_execution
Aug 13, 2026
Merged

fix(orchestrator): losing pod logs must not fail an execution#325
morgan-wowk merged 1 commit into
masterfrom
08-12-fix_orchestrator_losing_pod_logs_must_not_fail_an_execution

Conversation

@morgan-wowk

@morgan-wowk morgan-wowk commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

What breaks today

When the GKE cluster-autoscaler evicts and deletes a Job's pod mid-run, the Job object still reports Failed, so the orchestrator has everything it needs to record a clean FAILED. Today it doesn't:

  1. LaunchedKubernetesJob._get_log_by_pod_key re-raises on a 404 — only a 400 ("Bad Request", pod still initializing) was mapped to "no logs".
  2. The orchestrator's FAILED branch calls upload_log() unguarded, so that raise propagates. upload_log reads logs by remembered pod name → the pod is gone → 404_retry burns all 5 attempts and re-raises → the outer handler overwrites FAILED with SYSTEM_ERROR.

Observed twice on one production run: Exception calling <bound method LaunchedKubernetesJob.upload_log ...> ×5, then Error processing running container execution.

New rules

  • A log-upload failure never changes an execution's terminal status. The FAILED branch now guards upload_log exactly as the SUCCEEDED branch already does — a failed upload is logged and the execution stays FAILED with normal downstream skipping.
  • A vanished pod means "no logs", not an error. _get_log_by_pod_key returns None on a 404 (pod deleted), just as it already does on a 400 (pod initializing), with a warning naming the pod. _debug_pods deliberately retains pods that no longer exist, so a deleted pod would otherwise re-404 on every later read. See issue #139 for the original 400 case.

Why both fixes ship as one unit

Each fix alone leaves the failure reachable: the orchestrator guard stops the FAILED → SYSTEM_ERROR flip for any upload failure, while the 404 handling stops the vanished-pod read from raising in the first place — so logs from other still-present pods in the same Job still upload. Together they give one guarantee: losing logs never changes a terminal status.

Scope

  • LaunchedKubernetesContainer.get_log / stream_log_lines (the single-pod launcher) are left unchanged. They currently raise on any ApiException, and the orchestrator guard already protects the FAILED path for both launcher types; extending them would change pod-launcher semantics, so it's deliberately out of scope.
  • podFailurePolicy / backoffLimitPerIndex / maxFailedIndexes are untouched — a separate change.

Tests

  • tests/test_kubernetes_launcher_error_classification.py: _get_log_by_pod_key returns None on a 404 and still raises on 403 and 500 (mocked CoreV1Api).
  • tests/test_orchestrator_failed_log_upload.py: an execution whose launcher raises from upload_log ends FAILED (not SYSTEM_ERROR), with downstream marked SKIPPED.
  • Full suite green (uv run pytest).

Downstream

Shopify/oasis-backend pins this repo as the backend submodule; it needs a submodule pointer bump to pick this fix up.

morgan-wowk commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Signed-off-by: Morgan Wowk <morgan.wowk@shopify.com>
@morgan-wowk
morgan-wowk force-pushed the 08-12-fix_orchestrator_losing_pod_logs_must_not_fail_an_execution branch from 9802c98 to b2bdd00 Compare August 12, 2026 20:29
@morgan-wowk
morgan-wowk marked this pull request as ready for review August 12, 2026 20:49
@morgan-wowk
morgan-wowk requested a review from a team August 12, 2026 20:50
@morgan-wowk
morgan-wowk requested a review from Ark-kun as a code owner August 12, 2026 20:50
@morgan-wowk
morgan-wowk requested a review from a team August 12, 2026 20:50
@morgan-wowk
morgan-wowk merged commit 1b3e614 into master Aug 13, 2026
7 checks passed
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.

2 participants