Repository navigation
fix(job): scope --main-only logs to the main pod and its default container - #6422
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the GKE job orchestrator's log selector logic to narrow down the target pods when mainOnly is enabled. It introduces a selector suffix that restricts matches to the main replicated jobs (main-job and pathways-head), preventing a race condition where auxiliary worker pods with identical job and completion indices were incorrectly matched. The changes are accompanied by updated unit tests and a new test targeting this specific scenario. I have no feedback to provide.
529e64e to
ad6e281
Compare
ad6e281 to
37f896e
Compare
be776d0 to
e34207e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the GKE orchestrator's log retrieval logic to correctly target the main replicated job in a JobSet (such as pathways-head in Pathways) when using the --main-only flag. It replaces the resolveLogsSelector and getFirstContainerName methods with resolveLogsTarget and inspectJobSetMainJob, which leverage a new logsTarget struct to capture both the target selector and container. Additionally, comprehensive unit tests and regression tests have been added or updated to validate these changes. No review comments were provided, so there is no additional feedback to address.
5bdc723 to
128e073
Compare
128e073 to
bc7cbf5
Compare
bc7cbf5 to
659b408
Compare
…ainer
`gcluster job logs --main-only` selected pods with
`jobset-name=<job>,job-index=0,job-completion-index=0` and passed a guessed
`-c <container>`. Both indices are scoped per replicated job, so on a
Pathways JobSet the selector also matched `<job>-worker-0-0` and kubectl
failed with
container workload-container is not valid for pod <job>-worker-0-0-xxxxx
The container was guessed from `workload-container` / `pathways-` name
prefixes, which also picks the wrong container in custom templates.
Main-only mode now scopes the selector to rank-0 of the JobSet's first
replicated job (`replicatedjob-name=<spec.replicatedJobs[0].name>`, read
from the live JobSet) and passes neither `-c` nor `--all-containers`, so
kubectl (1.24+) reads each pod's default container: the
`kubectl.kubernetes.io/default-container` annotation if set, else
`spec.containers[0]`. Native sidecars (Pathways proxy/rm, the colocated
Python sidecar, GCS FUSE) are init containers and never the default, so
the built-in templates resolve to the same container as before. If the
JobSet can't be read, main-only falls back to rank-0 of every replicated
job, each with its default container. `--main-only=false` is unchanged.
`findWorkloadContainer`, the name prefixes and
`extractJobSetNameFromSelector` are removed (the JobSet name is passed
down instead of re-parsed from the selector), and `getFirstContainerName`
becomes `firstReplicatedJobName`. Tests cover standard, Pathways,
renamed-first-job and unreadable JobSets, the 5/6 and 10/11 pod
boundaries, empty logs, both follow modes, the retry loop (including the
timeout and Windows line endings) and the pod count. The `--main-only`
help text and the job guide describe the container choice and the
annotation for custom templates.
659b408 to
4e98820
Compare
Problem
gcluster job logs --main-only(also the implicit default for jobs with more than 5 pods) had two problems:It matched the wrong pods. The selector
uses indices that are scoped per replicated job, so on a Pathways JobSet it matches
<job>-pathways-head-0-0and<job>-worker-0-0, andkubectl logs -c workload-containerfails on the worker:It only starts working once the worker pods are garbage-collected after the JobSet completes. In
PR-test-gke-tpu-v5ethis shows up as a race with the test's retry window: build900172f3hit the error 6 times before passing on attempt 7, and builds5214db0fand5a2a7769ran out of retries even though the workload itself succeeded.It guessed the container.
-ccame from matchingworkload-container/pathways-name prefixes, falling back to the first container. Custom templates (--gke-custom-templates-path) could get the wrong container, or the same error on another replicated job.Fix
jobset.sigs.k8s.io/replicatedjob-name=<spec.replicatedJobs[0].name>, read from the live JobSet:main-job,pathways-head, or whatever a custom template lists first.-cor--all-containers, sokubectl(1.24+) reads the pod's default container: the one named by thekubectl.kubernetes.io/default-containerannotation, otherwisespec.containers[0]. Native sidecars (Pathways proxy/rm, the colocated Python sidecar, GCS FUSE) are init containers and never the default, so every built-in template resolves to the same container as before. Custom templates can steer it with the standard annotation; the job guide now says so.--all-containers, sidecars included).--main-only=false, the 5-pod auto-default, the--max-log-requestsguard and the retry loop's behaviour are unchanged.findWorkloadContainer, the name prefixes andextractJobSetNameFromSelectorare removed (the JobSet name is passed down instead of re-parsed from the selector); the JobSet query only reads{.spec.replicatedJobs[0].name}. The--main-onlyhelp text matches the docs.Verification
Unit tests
TestGetJobLogs: standard, Pathways, renamed-first-job and unreadable JobSets; the 5/6-pod default and 10/11-pod limit boundaries; a pod with no output yet;--main-only=false; and both follow modes (MockExecutornow records streamed commands).TestResolveLogsSelector_MainOnlyTargetsMainReplicatedJobparses the selector withk8s.io/apimachinery/pkg/labelsand checks it rejects workers, rank-1 pods, other replicated jobs and other JobSets, including in the unreadable-JobSet fallback.TestFetchLogsWithRetry: containers still starting (also with Windows line endings), no matching pods, empty output, fail-fast errors, and the 12-attempt timeout with JobSet warning events.TestFirstReplicatedJobNameandTestGetJobPodCount: trimming, Windows line endings and failures.go test ./...andpre-commitpass.Windows:
gcluster.exeand both test packages cross-compile forwindows/amd64andgo vetis clean withGOOS=windows.kubectlis run with an argument list (no shell), and all output parsing tolerates CRLF.JobSet v0.12.0: labels, the
v1alpha2API,spec.replicatedJobs[].nameand the Indexed default are unchanged from v0.8.1; the selector and container choice were also checked against a v0.12.0 controller.Live test on a GKE 1.36 cluster (JobSet v0.8.1) with
kubectl1.36, against JobSets with the same container layouts as the built-in templates:develop--main-onlycontainer workload-container is not valid for pod …-worker-0-0-…workload-container--main-onlycontainer pathways-proxy is not valid for pod …-worker-0-0-…pathways-proxydefault-containerannotation,--main-onlycontainer metrics-exporter is not valid for pod …-evaluator-0-0-…my-trainer--main-only, rank-0workload-container-0--main-only -fworkload-container--main-only=falseNot addressed here (same on
develop), planned for a follow-up PR:-f, while telling apart jobs that are queued, finished, or not found, and applying the main-only default once pods exist.-f --main-only=false,kubectlcounts every container stream, sidecars included, against--max-log-requests, so a JobSet with 10 or fewer pods can still exceed the limit.Submission Checklist
pre-commit install)make tests)