Surface kubectl job failures instead of masking them#598
Conversation
Command() discarded any non-net.Error failure from CreateJobAndRunKubectlCommands, including the watch timeout returned by wait.WatchWait. It then proceeded to fetch the job pod's logs, and when no pod could be found GetPodLogs failed with a misleading "resource name may not be empty", hiding the real cause (e.g. an unpullable shell-image in an airgap cluster). Capture the job error while still attempting the best-effort log fetch, since the pod logs usually explain the failure. When the logs cannot be retrieved, include the job error in the returned error so the root cause is visible, and guard the empty-pod case with a clear message. Verified against an airgap-style failure (unpullable shell-image): the error now reports the job failure and "image can't be pulled" instead of "resource name may not be empty".
🔗 Related PRs — full fix chain for the airgap
|
There was a problem hiding this comment.
Pull request overview
This PR updates extensions/kubectl.Command() to preserve and surface underlying Kubernetes Job failures instead of masking them behind secondary log-streaming errors, while keeping the existing “best-effort fetch logs” behavior.
Changes:
- Stop discarding non-
net.ErrorJob failures by capturing the Job error (jobErr) and using it when log retrieval/pod discovery fails. - Add an explicit guard for the empty-pod-name case with a clearer, job-error-aware message.
- Improve function-level documentation to better describe the behavior and error-handling semantics.
Comments suppressed due to low confidence (1)
extensions/kubectl/command.go:110
- When ProxyDownstream/List pods fails and the job itself already errored (jobErr != nil), the returned error drops jobErr entirely. That can still mask the underlying job failure in exactly the scenarios this PR is trying to surface (e.g., watch timeout, create failures) because logs can't be retrieved if proxying/listing fails.
jobErr := CreateJobAndRunKubectlCommands(clusterID, jobName, jobTemplate, client)
steveClient, err := client.Steve.ProxyDownstream(clusterID)
if err != nil {
return "", err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| podLogs, err := kubeconfig.GetPodLogs(client, clusterID, podName, Namespace, logBufferSize) | ||
| if err != nil { | ||
| if jobErr != nil { | ||
| return "", fmt.Errorf("kubectl job %s failed (job error: %w); failed to stream logs for pod %s/%s: %v", jobName, jobErr, Namespace, podName, err) |
Problem
kubectl.Command()masked the real cause of helper-Job failures. It only re-surfaced non-timeoutnet.Errors fromCreateJobAndRunKubectlCommands; the watch timeout fromwait.WatchWait(a plain error) and other non-net.Errorfailures were silently discarded. It then proceeded to fetch the job pod's logs, and when no pod matched,GetPodLogs("")failed with a misleadingresource name may not be empty— hiding the actual root cause (e.g. an unpullableshell-imagein an airgap cluster).Fix
jobErrinstead of swallowing it, while preserving the best-effort log fetch (pod logs usually explain the failure).podNamecase with a clear error that includesjobErr.jobErrwhen the job also failed.(logs, nil).Verification
Reproduced the failure condition (unpullable
shell-image) against a live cluster viarancher/tests:error streaming pod logs for pod kube-system/: resource name may not be emptykubectl job kubectl-hrglgn failed (job error: error with watch connection…); … container … is waiting to start: image can't be pulledgofmtclean;go vet/go build ./extensions/kubectl/pass.