ci: collect cluster diagnostics for failing component tests - #3560
Conversation
|
Welcome to the Kubeflow Community Distribution Repository Thanks for opening your first PR. Your contribution means a lot to the Kubeflow community. Before making more PRs: Community Resources:
Thanks again for helping to improve Kubeflow. |
The workflow diagnostics added here are useful independently of Pod Security Standards and were holding up review of both concerns at once. They now live in their own pull request against master. Remove the Collect Logs on Failure and Upload Diagnostic Logs steps from the seven component workflows and from the training operator workflow, so this change carries only the restricted Pod Security work. The workflow trigger paths added for Pod Security coverage stay, because they belong to this change. The diagnostics blocks that master already owns for the KServe, Ray, Trainer, and full integration workflows are untouched. Refs: kubeflow#3560 Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
There was a problem hiding this comment.
Pull request overview
Centralizes failure diagnostics for seven component workflows, capturing cluster, node, storage, host, and container evidence.
Changes:
- Adds a shared diagnostic collection script.
- Uploads diagnostic artifacts for seven workflows.
- Adds component-specific resource snapshots and pressure sampling.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/collect_cluster_diagnostics.sh |
Implements shared cluster diagnostics. |
.github/workflows/dashboard_test.yaml |
Adds dashboard failure diagnostics. |
.github/workflows/dex_oauth2-proxy_test.yaml |
Adds identity-service failure diagnostics. |
.github/workflows/katib_test.yaml |
Adds Katib diagnostics and resource snapshots. |
.github/workflows/notebooks_api_authorization_test.yaml |
Adds notebook authorization diagnostics. |
.github/workflows/pipeline_run_from_notebook.yaml |
Adds pipeline and notebook diagnostics. |
.github/workflows/volumes_web_application_test.yaml |
Adds volume application diagnostics. |
.github/workflows/workspaces_pipeline_run_test.yaml |
Adds workspace diagnostics and pressure sampling. |
Comments suppressed due to low confidence (1)
tests/collect_cluster_diagnostics.sh:45
- This pipeline also converts a failed node query into an empty value, which the output formatter reports as
none. That makes an unreachable API server indistinguishable from a healthy node with no pressure conditions; retain thekubectlstatus and reservenonefor a successful query with no matching conditions.
node_pressure=$(kubectl get nodes \
-o jsonpath='{range .items[*]}{range .status.conditions[?(@.status=="True")]}{.type} {end}{end}' \
2>/dev/null | tr ' ' '\n' | grep -E 'Pressure' | sort -u | tr '\n' ',' | sed 's/,$//')
| kubectl logs -n "$namespace" "$pod_name" --all-containers --previous \ | ||
| --tail=100 \ | ||
| >"$OUTPUT_DIRECTORY/$namespace-$pod_name-previous.txt" 2>&1 || | ||
| rm -f "$OUTPUT_DIRECTORY/$namespace-$pod_name-previous.txt" |
There was a problem hiding this comment.
Fixed: previous logs are now collected per container, selected by restartCount>0, so a stable sidecar no longer causes the restarted application container's prior log to be discarded.
| - name: Start diagnostics sampler | ||
| run: | | ||
| nohup ./tests/collect_cluster_diagnostics.sh sample 10 >/dev/null 2>&1 & |
There was a problem hiding this comment.
Removed, since the sampler was temporary instrumentation for diagnosing #3563 and its cost during installation is not justified now that the cause is known.
| - name: Collect Logs | ||
| if: always() |
There was a problem hiding this comment.
Fixed: this workflow now uses failure() || cancelled() like the other six, so no green run uploads an artifact.
| # Every command is best effort: a failure here must never mask the original | ||
| # test failure, so the script never exits non-zero and never stops early. | ||
| # | ||
| # Usage: tests/collect_cluster_diagnostics.sh [namespace ...] |
There was a problem hiding this comment.
Not adding these paths, because the diagnostic step runs only on failure, so a run triggered by editing this script would pass without ever executing it and would give no coverage while consuming seven end to end jobs. shellcheck already validates the script through pre-commit, and a smoke test asserting it exits zero without a cluster would be the appropriate coverage if more is wanted.
There was a problem hiding this comment.
Resolved by removing the script, so there is no shared implementation left to reference from the trigger paths.
| - name: Start diagnostics sampler | ||
| run: | | ||
| nohup ./tests/collect_cluster_diagnostics.sh sample 10 >/dev/null 2>&1 & | ||
| echo "Sampling runner and cluster pressure every 10 seconds." | ||
|
|
There was a problem hiding this comment.
Removed, since this sampler was temporary instrumentation used to find the cause of the pipelines readiness flake, which is now recorded as API server degradation in #3563.
| # taken once the job has finished shows a settled cluster, so it cannot show how | ||
| # close to its limit the runner came while the workloads were starting. Start | ||
| # this in the background before the step under investigation. | ||
| if [ "${1:-}" = "sample" ]; then |
There was a problem hiding this comment.
do we really need this ?
There was a problem hiding this comment.
No, removed together with the sampler step: it existed only to capture the pressure time series while diagnosing #3563, and the failure only diagnostics that remain are what produced that evidence.
There was a problem hiding this comment.
The whole script is now removed. Reviewing what the evidence actually required, every finding that identified the cause in #3563 came from cluster events, pod descriptions and pod logs, which the block already used by the full integration, KServe, Ray and Trainer workflows collects. These seven workflows now use that same block, so the pull request only adds the coverage that was missing and introduces no new pattern.
Seven component workflows ended without capturing anything when a job failed, so a red run left only the console output of the step that timed out. Diagnosing a failure then required reproducing it locally or guessing from the pod name in the timeout message. Add a Collect Logs on Failure step and a Diagnostic Logs artifact to the dashboard, Dex with OAuth2 Proxy, Katib, notebooks authorization, pipeline run from notebook, volumes web application, and workspaces pipeline workflows, matching the pattern already used by the KServe, Ray, Trainer, Istio validation, and full integration workflows. Collect the shared part through tests/collect_cluster_diagnostics.sh rather than repeating it in every workflow. Five workflows already carry their own copy of this block, and inlining it seven more times would leave thirteen copies to keep in step. Record what a per-namespace pod description alone does not show. Node descriptions report the memory, disk, and process identifier pressure conditions that distinguish a stalled workload from a runner that ran out of capacity and evicted healthy pods. Runner memory, disk, and load report the same limit from outside the cluster. Persistent volume claims explain readiness timeouts that cascade from unbound storage, wide pod output carries restart counts, and previous container logs explain a container that already died. The kube-system and local-path-storage namespaces are always collected because a stalled control plane or storage provisioner surfaces as an unrelated component timing out. Every command is best effort and the script always exits zero, so collecting diagnostics cannot mask the failure that triggered it. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Temporary marker to re-trigger the workspaces pipeline workflow while collecting diagnostics for the readiness stall. Removed before review. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Temporary marker to re-trigger the workspaces pipeline workflow while collecting diagnostics for the readiness stall. Removed before review. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
… test The workspaces pipeline test stalls intermittently: twelve of fifty pods stop becoming ready and the six hundred second wait in tests/pipelines_install.sh expires. A passing run completes the same wait in about thirty five seconds, so the failure is a stall rather than slowness, and the pods that stall include the storage provisioner and the authentication proxy, which were both ready earlier in the same run. Collecting only on failure cannot explain this, because the failure is rare and a snapshot taken after the job has finished shows a settled cluster. Sample instead: record the load average, available runner memory, the count of pods that are not ready, the count of unbound claims, and any node pressure condition every ten seconds, starting before the pipelines installation. Collect on every run rather than only on failure, so a passing run also reports how close to its limit the runner came. This shows whether a healthy run already sits near exhaustion, which a red run alone cannot establish. Sampling is confined to this workflow while the stall is under investigation. The other workflows keep collecting only on failure. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Temporary marker to re-trigger the pipelines installation workflows while confirming the ml-pipeline startup probe failure. Removed before review. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Temporary marker to re-trigger the pipelines installation workflows while confirming the ml-pipeline startup probe failure. Removed before review. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Temporary marker to re-trigger the pipelines installation workflows while confirming the ml-pipeline startup probe failure. Removed before review. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Remove the pressure sampler and its workflow step. The sampler was temporary instrumentation used to identify the cause of the pipelines readiness failure, which is now recorded as API server degradation in issue 3563. Keeping it would leave a background process issuing Kubernetes queries every ten seconds during the installation, which is the exact condition the failure is sensitive to. Restore the failure-or-cancellation condition on the workspaces workflow so that all seven workflows collect on the same condition and no green run uploads an artifact. Collect previous container logs per restarted container. A single call covering every container fails as soon as one container has no prior instance, and the cleanup then discarded the logs that had been obtained successfully. That is the ordinary case when an application container restarts while its sidecar stays up, which is precisely the evidence needed to explain a restart. Removing the sampler also removes the sampled counters, which reported zero unready pods and zero unbound claims when a query failed rather than reporting that the data was unavailable. Refs: kubeflow#3563 Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
005596a to
e94cc5b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
tests/collect_cluster_diagnostics.sh:78
- This query examines only
status.containerStatuses, so restarted initialization containers are omitted. Pipelines installed by the consuming workflows containwait-for-*initialization containers, for exampleapplications/pipeline/upstream/base/pipeline/ml-pipeline-apiserver-deployment.yaml:24, and the prior instance can explain why a pod never becomes ready. Includestatus.initContainerStatusesin the selection.
-o jsonpath='{.status.containerStatuses[?(@.restartCount>0)].name}' \
tests/collect_cluster_diagnostics.sh:29
- This comment is inaccurate: a pod can lose readiness without restarting, and
kubectl describealready reports restart counts and prior container state. Describe this output as a cluster-wide summary instead of asserting that a readiness transition appears as a restart.
# Pod restart counts and placement. A pod that was ready earlier and is not
# ready now shows up here as a restart, which a describe of the current state
# alone does not reveal.
tests/collect_cluster_diagnostics.sh:81
previous_logstores a file path rather than log content, so the name is ambiguous and conflicts with the expressive naming requirement inAGENTS.md:7. Rename it to identify the value as the previous container log file path.
previous_log="$OUTPUT_DIRECTORY/$namespace-$pod_name-$container_name-previous.txt"
| kubectl get all --all-namespaces >"$OUTPUT_DIRECTORY/resources.txt" 2>&1 || true | ||
| kubectl get events --all-namespaces --sort-by=.metadata.creationTimestamp \ | ||
| >"$OUTPUT_DIRECTORY/events.txt" 2>&1 || true |
There was a problem hiding this comment.
Fixed: every call now goes through a wrapper that sets --request-timeout and an outer per-call deadline, and collection stops once an overall budget is spent, so a degraded API server can no longer hang the step and cost us the artifact upload. Verified with a stubbed kubectl that never returns, where collection now ends in seconds instead of running to the job limit.
There was a problem hiding this comment.
Resolved by removing the script. These workflows now use the same inline block as the four existing consumers on master. The unbounded request concern applies equally to that existing block, so it is better handled once for all workflows in a separate change than introduced here.
There was a problem hiding this comment.
Reopening this: the concern is correct and now fixed here rather than deferred. kubectl options documents --request-timeout='0' as not timing out requests at all, and six of these seven workflows set no job level timeout, so the default of three hundred and sixty minutes would apply to a hang. The collection step now carries timeout-minutes: 5. A step that exceeds its own timeout fails rather than cancelling the job, so the upload still runs and partial evidence is preserved. The four workflows that already carry this block on master have the same gap, and are left for a follow up because two of them are also modified by #3515.
There was a problem hiding this comment.
Extended to every workflow rather than deferred. All eleven diagnostic collection steps in the repository now carry timeout-minutes: 5, including the four that already had this block before this pull request. The overlap with #3515 on kserve_test.yaml and full_kubeflow_integration_test.yaml is a three line addition in each and is straightforward to resolve whichever merges first.
| with: | ||
| name: workspaces-pipeline-run-test-logs | ||
| path: logs/ | ||
| # reproduction attempt 3 |
There was a problem hiding this comment.
Removed, that marker was left over from reproducing the failure and should not have been committed.
kubectl defaults --request-timeout to zero, which means no deadline. A degraded API server that accepts a request and then stops responding would hang the collection step until the job limit, and the artifact upload that follows would never run. The diagnostics would therefore be lost in exactly the situation they exist to explain, since API server degradation is the failure recorded in issue 3563. Route every call through a wrapper that sets a request timeout and an outer per-call deadline, and stop collecting once an overall budget is spent so a slow cluster cannot consume the remainder of the job. Verified with a stubbed kubectl that never returns: collection now ends in seconds and still writes what it managed to gather. Also remove the temporary reproduction marker left in the workspaces workflow while the failure was being reproduced. Refs: kubeflow#3563 Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Replace the shared collection script with the block that master already uses in the full integration, KServe, Ray and Trainer workflows. The script was not carrying its weight. Every piece of evidence that identified the cause of the pipelines readiness failure in issue 3563 came from three collectors that the existing block already runs: cluster events, pod descriptions and pod logs. The additional collectors only produced evidence that ruled possibilities out, and the previous container log collection never produced a single file across any run. The abstraction had also grown larger than the duplication it was meant to remove, and review found three defects in it that the existing nineteen line block does not have. The remaining change is therefore what was actually missing: seven workflows that collected nothing on failure now collect the same evidence as the four that already did. Component specific resources are kept for Katib, notebooks and workspaces. Refs: kubeflow#3563 Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
kubectl does not time out requests by default. Its --request-timeout defaults to zero, which the built in documentation describes as not timing out requests at all. An API server that accepts a request and then stops responding would therefore hold the collection step open, and since the upload step runs after it, the artifact would be lost in exactly the situation the collection exists to explain. Issue 3563 records an API server that degraded during installation, so this is not hypothetical. Six of these seven workflows set no job level timeout, so the default of three hundred and sixty minutes applies and a single hang would occupy a runner for six hours. Set a five minute timeout on the collection step. Collection normally finishes in seconds. A step that exceeds its own timeout fails rather than cancelling the job, so the upload step still runs and whatever was gathered before the hang is preserved. The four workflows that already carry this block on master have the same gap. They are left alone here because two of them are also modified by pull request 3515, and changing them in both places would conflict. Refs: kubeflow#3563 Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
Apply the same five minute bound to the four workflows that already carried this block, so every diagnostic collection step in the repository is bounded rather than only the seven added here. These four have the same gap for the same reason: kubectl does not time out requests by default, so an API server that accepts a request and then stops responding holds the step open and the upload never runs. Shorten the accompanying comment, since it now appears in eleven workflows. Signed-off-by: danish9039 <danishsiddiqui040@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (7)
.github/workflows/workspaces_pipeline_run_test.yaml:89
- These workloads use Istio sidecars, but
kubectl logswithout a container selector returns only the default container. The sidecar connection failures highlighted by issue 3563 are therefore omitted from the artifact. Collect every container and prefix each line so interleaved output remains attributable.
kubectl logs -n $namespace $pod --tail=100 > logs/$namespace-$pod.txt 2>&1 || true
.github/workflows/volumes_web_application_test.yaml:88
- These workloads use Istio sidecars, but
kubectl logswithout a container selector returns only the default container. The sidecar connection failures highlighted by issue 3563 are therefore omitted from the artifact. Collect every container and prefix each line so interleaved output remains attributable.
kubectl logs -n $namespace $pod --tail=100 > logs/$namespace-$pod.txt 2>&1 || true
.github/workflows/pipeline_run_from_notebook.yaml:97
- These workloads use Istio sidecars, but
kubectl logswithout a container selector returns only the default container. The sidecar connection failures highlighted by issue 3563 are therefore omitted from the artifact. Collect every container and prefix each line so interleaved output remains attributable.
kubectl logs -n $namespace $pod --tail=100 > logs/$namespace-$pod.txt 2>&1 || true
.github/workflows/notebooks_api_authorization_test.yaml:108
- These workloads use Istio sidecars, but
kubectl logswithout a container selector returns only the default container. The sidecar connection failures highlighted by issue 3563 are therefore omitted from the artifact. Collect every container and prefix each line so interleaved output remains attributable.
kubectl logs -n $namespace $pod --tail=100 > logs/$namespace-$pod.txt 2>&1 || true
.github/workflows/katib_test.yaml:94
- These workloads use Istio sidecars, but
kubectl logswithout a container selector returns only the default container. The sidecar connection failures highlighted by issue 3563 are therefore omitted from the artifact. Collect every container and prefix each line so interleaved output remains attributable.
kubectl logs -n $namespace $pod --tail=100 > logs/$namespace-$pod.txt 2>&1 || true
.github/workflows/dex_oauth2-proxy_test.yaml:87
- These workloads use Istio sidecars, but
kubectl logswithout a container selector returns only the default container. The sidecar connection failures highlighted by issue 3563 are therefore omitted from the artifact. Collect every container and prefix each line so interleaved output remains attributable.
kubectl logs -n $namespace $pod --tail=100 > logs/$namespace-$pod.txt 2>&1 || true
.github/workflows/dashboard_test.yaml:52
- These workloads use Istio sidecars, but
kubectl logswithout a container selector returns only the default container. The sidecar connection failures highlighted by issue 3563 are therefore omitted from the artifact. Collect every container and prefix each line so interleaved output remains attributable.
kubectl logs -n $namespace $pod --tail=100 > logs/$namespace-$pod.txt 2>&1 || true
| # let the upload below still run if the API server stops responding. | ||
| timeout-minutes: 5 | ||
| run: | | ||
| mkdir -p logs |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juliusvonkohout The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of Changes
Seven component workflows ended without capturing anything when a job failed, so
a red run left only the console output of the step that timed out.
Collect Logs on Failurestep and a diagnostic-log artifact to thedashboard, Dex with OAuth2 Proxy, Katib, notebooks authorization, pipeline run
from notebook, volumes web application, and workspaces pipeline workflows.
and full integration workflows.
tests/collect_cluster_diagnostics.shinsteadof repeating it in every workflow.
Why a shared script
Five workflows already carry their own copy of this block. Inlining it seven
more times would leave thirteen copies to keep in step. The shared script makes
this change 89 lines across the workflows rather than roughly 168, while
collecting strictly more than the existing copies do.
What is collected beyond the existing pattern
The current blocks record cluster resources, events, pod descriptions and pod
logs. They cannot answer why a pod that was ready earlier stopped being ready.
distinguishes a stalled workload from a runner that ran out of capacity and
evicted healthy pods.
storage.
kube-systemandlocal-path-storagenamespaces are always collected,because a stalled control plane or storage provisioner surfaces as an
unrelated component timing out.
Safety
Every command is best effort and the script always exits zero, so collecting
diagnostics can never mask the failure that triggered it. This was verified by
running the script with no cluster reachable.
There is no change to any installation path, so no test behaviour changes.
Verification
pre-commiton every changed file: yamllint, shellcheck and hygiene hooks pass.upload-artifactis pinned to the same commit already used elsewhere in therepository.
Contributor Checklist
Related Issues
Tracking issue for the failure these diagnostics were built to capture:
#3563