potiuk commented on code in PR #73591:
URL: https://github.com/apache/airflow/pull/73591#discussion_r4308013045
##########
dev/breeze/src/airflow_breeze/commands/kubernetes_kustomize_commands.py:
##########
@@ -539,6 +595,8 @@ def _smoke_test_overlay_impl(
console_print("[error]verify block failed.")
return 1
console_print("\n[success]verify block passed.")
+ if _wait_for_airflow_pods(namespace, release_name, timeout, env) != 0:
Review Comment:
This gets a fresh full `timeout` after the verify block has already used its
own per resource, so a failing run can now take 2-3× the documented per-overlay
`timeout_seconds` and run into the CI job's own timeout (losing the pod
diagnostics, which only print when this function's deadline is reached).
Sharing one deadline across verify and the pod check, or documenting the extra
budget, would keep that predictable.
##########
dev/breeze/src/airflow_breeze/commands/kubernetes_kustomize_commands.py:
##########
@@ -279,6 +280,61 @@ def _wait_for_verify_resource(
return 1
+def _get_pod_health_problems(pods: list[dict[str, Any]]) -> list[str]:
+ problems = []
+ for pod in pods:
+ name = pod["metadata"]["name"]
+ status = pod.get("status", {})
+ phase = status.get("phase", "Unknown")
+ if pod["metadata"].get("deletionTimestamp"):
+ problems.append(f"{name}: terminating")
+ elif phase == "Succeeded":
+ continue
Review Comment:
Any `Failed` pod with the release label keeps this failing until the
timeout. That includes an evicted pod (kind under disk pressure) and a failed
attempt of a `restartPolicy: Never` Job such as the chart's cleanup CronJob,
which leaves a `Failed` pod next to the `Succeeded` retry. Neither says
anything about the live deployment. A bare failed pod (e.g. a
KubernetesExecutor task pod) is still reported.
```suggestion
if pod["metadata"].get("deletionTimestamp"):
problems.append(f"{name}: terminating")
elif phase == "Succeeded":
continue
elif phase == "Failed" and (
status.get("reason") == "Evicted"
or any(owner.get("kind") == "Job" for owner in
pod["metadata"].get("ownerReferences", []))
):
# The controller replaces an evicted pod, and a Job retries a
failed attempt in a new pod,
# so neither says anything about the live deployment.
continue
```
##########
dev/breeze/src/airflow_breeze/commands/kubernetes_kustomize_commands.py:
##########
@@ -279,6 +280,61 @@ def _wait_for_verify_resource(
return 1
+def _get_pod_health_problems(pods: list[dict[str, Any]]) -> list[str]:
+ problems = []
+ for pod in pods:
+ name = pod["metadata"]["name"]
+ status = pod.get("status", {})
+ phase = status.get("phase", "Unknown")
+ if pod["metadata"].get("deletionTimestamp"):
+ problems.append(f"{name}: terminating")
+ elif phase == "Succeeded":
+ continue
+ elif phase != "Running":
+ problems.append(f"{name}: {phase}")
+ elif not any(
+ condition["type"] == "Ready" and condition["status"] == "True"
+ for condition in status.get("conditions", [])
+ ):
+ problems.append(f"{name}: not Ready")
+ return problems
+
+
+def _wait_for_airflow_pods(
+ namespace: str, release_name: str, timeout_seconds: int, env: dict[str,
str]
+) -> int:
+ """Wait for the Helm release's pods to be Running/Ready or successfully
completed."""
+ kubectl = str(KUBECTL_BIN_PATH)
+ scope = ["-n", namespace, "-l", f"release={release_name}"]
+ console_print(f"[info]Checking Airflow pod health for release
{release_name} in {namespace}...")
+ deadline = time.monotonic() + timeout_seconds
+ problems = ["No healthy Airflow pods observed"]
+ while time.monotonic() < deadline:
+ result = run_command(
+ [kubectl, "get", "pods", *scope, "-o", "json",
"--request-timeout=10s"],
+ env=env,
+ check=False,
+ capture_output=True,
+ text=True,
+ )
+ if result.returncode != 0:
+ console_print(f"[error]Could not check Airflow pod health:
{result.stderr}")
+ return 1
+ pods = json.loads(result.stdout)["items"]
+ problems = _get_pod_health_problems(pods)
Review Comment:
Three things in one block:
- **retry** a failed `kubectl get pods` until the deadline, as
`_wait_for_verify_resource` does, instead of failing on the first API blip
right after the overlay is applied;
- **dry-run**: `run_command` returns empty output under `breeze --dry-run`,
and `json.loads("")` then raises;
- **fail fast** on `CrashLoopBackOff` / `ImagePullBackOff` and the other
`_TERMINAL_POD_WAITING_REASONS`, which the verify block already treats as
unrecoverable, instead of waiting out the full timeout.
```suggestion
if result.returncode != 0:
# Transient API errors right after applying an overlay are
common; retry like the verify loop.
problems = [f"Could not list pods: {result.stderr.strip()}"]
time.sleep(min(5, max(0, deadline - time.monotonic())))
continue
if not result.stdout.strip():
# `breeze --dry-run` runs nothing and returns empty output.
return 0
pods = json.loads(result.stdout)["items"]
problems = _get_pod_health_problems(pods)
unrecoverable = [
f"{pod['metadata']['name']}: {reason}"
for pod in pods
for container in [
*pod.get("status", {}).get("containerStatuses", []),
*pod.get("status", {}).get("initContainerStatuses", []),
]
if (reason := container.get("state", {}).get("waiting",
{}).get("reason"))
in _TERMINAL_POD_WAITING_REASONS
]
if unrecoverable:
console_print(f"[error]Airflow pods will not recover: {';
'.join(unrecoverable)}")
run_command([kubectl, "describe", "pods", *scope], env=env,
check=False)
return 1
```
##########
dev/breeze/src/airflow_breeze/commands/kubernetes_kustomize_commands.py:
##########
@@ -279,6 +280,61 @@ def _wait_for_verify_resource(
return 1
+def _get_pod_health_problems(pods: list[dict[str, Any]]) -> list[str]:
+ problems = []
+ for pod in pods:
+ name = pod["metadata"]["name"]
+ status = pod.get("status", {})
+ phase = status.get("phase", "Unknown")
+ if pod["metadata"].get("deletionTimestamp"):
+ problems.append(f"{name}: terminating")
+ elif phase == "Succeeded":
+ continue
+ elif phase != "Running":
+ problems.append(f"{name}: {phase}")
+ elif not any(
+ condition["type"] == "Ready" and condition["status"] == "True"
+ for condition in status.get("conditions", [])
+ ):
+ problems.append(f"{name}: not Ready")
+ return problems
+
+
+def _wait_for_airflow_pods(
+ namespace: str, release_name: str, timeout_seconds: int, env: dict[str,
str]
+) -> int:
+ """Wait for the Helm release's pods to be Running/Ready or successfully
completed."""
+ kubectl = str(KUBECTL_BIN_PATH)
+ scope = ["-n", namespace, "-l", f"release={release_name}"]
Review Comment:
Selecting by `release=<name>` checks the whole release rather than the
overlay's effect. On a long-lived local cluster, an unrelated unhealthy pod
(statsd, pgbouncer, something left by an earlier `--skip-cleanup` run) gets
blamed on the overlay, and with `--promote-status` the overlay can't be
promoted. Scoping to the workloads the overlay patches, or taking a baseline
before applying, would isolate it; at least worth a note in the docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
