pal-e-docs

Review: Bug: platform-validation OOMKilled at 64Mi + stale alert rule (v2)

review-388-2026-03-26 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Second review pass (post scope-correction comment on issue #171). First review identified 3 issues; scope correction resolved 2 of 3 but introduced a new PromQL semantics issue.

Template Completeness

Checked against template-issue. Original issue body + scope correction comment combined:
  • [x] Type (bug)
  • [x] Lineage (standalone — discovered during AlertManager triage 2026-03-26)
  • [x] Repo (corrected in comment: pal-e-deployments + pal-e-platform)
  • [ ] User Story — MISSING
  • [x] What Broke (serves as Context for a bug)
  • [x] File Targets (added in scope correction, verified — see below)
  • [x] Acceptance Criteria (updated in scope correction)
  • [ ] Test Expectations — MISSING (no test commands beyond manual kubectl; should specify tofu plan and ArgoCD sync verification commands)
  • [ ] Constraints — MISSING (should note: tofu plan -lock=false; ArgoCD auto-syncs pal-e-deployments; two separate PRs needed)
  • [ ] Checklist — MISSING
  • [x] Related

File Targets

  • [x] pal-e-deployments/overlays/platform-validation/prod/deployment-patch.yaml lines 22-27 — VERIFIED: resources block present, limits.memory: 64Mi confirmed at line 27. Scope correction says lines 22-27, actual is lines 22-27. Exact match.
  • [x] pal-e-platform/terraform/main.tf lines 230-242 — VERIFIED: OOMKilled alert rule at lines 230-241, expression kube_pod_container_status_last_terminated_reason{reason="OOMKilled"} > 0 with for = "0m" confirmed at lines 232-233. Scope correction says lines 230-242, actual block ends at line 241 (closing brace). Close enough.

Repo Placement

The scope correction correctly identifies two repos. Improvement over v1 review — repos and file paths are now explicit:
  • Memory limit: forgejo_admin/pal-e-deployments (kustomize overlay, ArgoCD-synced)
  • Alert rule: forgejo_admin/pal-e-platform (terraform PrometheusRule, applied via tofu apply)
However, the issue body on Forgejo has NOT been updated — the corrections only exist in a comment. The Forgejo issue is filed only on pal-e-platform. An agent reading just the issue body would not know about the pal-e-deployments change. The cross-repo scope needs to be either in the issue body or a second issue must be created on pal-e-deployments.

Dependencies

No blockers found on board-pal-e-platform. Item #388 is in todo column. No items currently in in_progress. Three sibling alert-triage bugs also in todo (#385, #386, #387) — independent, no blocking relationship. The pal-e-deployments memory bump and the pal-e-platform alert rule fix are independent of each other and can be merged in either order.

Acceptance Criteria

Assessment of the updated acceptance criteria from the scope correction comment:
  • [x] "Memory limit bumped to 128Mi in pal-e-deployments" — testable via kubectl get pod -n platform-validation -o jsonpath='{.items[0].spec.containers[0].resources.limits.memory}'
  • [x] "OOMKilled alert rule updated with for: 15m in pal-e-platform" — testable via tofu plan -lock=false
  • [x] "ArgoCD syncs platform-validation successfully" — testable via kubectl get app platform-validation -n argocd
  • [x] "tofu plan shows only the for: duration change" — testable
  • [ ] "Current OOMKilled alert clears within 15 minutes" — ISSUE: Misleading criterion. The metric kube_pod_container_status_last_terminated_reason{reason="OOMKilled"} persists in kube-state-metrics as long as the pod's last termination was OOMKilled. It does NOT clear when the pod recovers and runs successfully. Adding for: 15m means the alert transitions from pending to firing after 15 minutes of the condition being true — but since the condition is permanently true after an OOMKill (until pod deletion or restart for a different reason), the alert will still fire 15 minutes after the OOMKill and remain firing indefinitely. The alert clears in this specific case only because bumping the memory limit triggers a deployment rollout, which creates a new pod with no OOM termination history. The acceptance criterion should state: "After deployment rollout from the memory bump, the OOMKilled alert clears because the new pod has no OOM termination history." This distinction matters for future OOMKills — for: 15m alone does not prevent staleness.

Blast Radius

  • Other 64Mi services: playground (main + asset-upload sidecar) and svelte-playground also have 64Mi memory limits in pal-e-deployments. These are nginx static file servers where 64Mi is appropriate. No action needed.
  • Other for: 0m rules: PodRestartStorm (line 220-221) also uses for: 0m, but its expression uses increase(...[15m]) which naturally resets when restarts stop. Not stale in the same way. No blast radius issue.
  • Cluster-wide alert scope: The OOMKilled alert has no namespace filter — changing for: to 15m affects all OOMKill alerts cluster-wide. This is correct behavior (brief OOMKills that self-resolve should not be critical alerts), but agents should be aware.

PromQL Correction Assessment

The scope correction correctly identifies that the originally proposed increase(kube_pod_container_status_restarts_total{reason="OOMKilled"}[15m]) > 0 is invalid — kube_pod_container_status_restarts_total does NOT carry a reason label in kube-state-metrics. This was confirmed by examining the existing PodRestartStorm rule at line 220 which uses the same metric without a reason label.
The corrected approach (keep existing expression, add for: 15m) is a pragmatic improvement but does not fully solve staleness. See Acceptance Criteria section above for details.

Recommendation

Three items must be addressed before this ticket is READY:
  • Cross-repo scope must be explicit and agent-readable — The scope correction lives only in a comment. Either update the issue body to include both repos with file paths and note "two PRs required," or create a companion issue on pal-e-deployments for the memory bump. An agent reading just the issue body would miss the pal-e-deployments change entirely.
  • Correct the "alert clears within 15 minutes" acceptance criterion — Replace with: "After deployment rollout (triggered by memory limit bump), the OOMKilled alert clears because the new pod has no OOM termination history." The current wording implies for: 15m alone resolves staleness, which is incorrect.
  • Add missing template sections — Test Expectations (verification commands for both repos), Constraints (tofu plan -lock=false, ArgoCD auto-sync, two separate PRs), and Checklist.