Repository navigation
fix(plugins): list only the job's own pods - #8215
madiyar-wayve wants to merge 1 commit into
Conversation
popojk
left a comment
popojk
left a comment
There was a problem hiding this comment.
This looks good. Maybe we can move the in name space filter one level up to plugin manager so we don't need to do this check in every plugin.
| func isRayHeadReady(ctx context.Context, namespace, rayJobName string, pluginContext k8s.PluginContext) (bool, error) { | ||
| podList := &v1.PodList{} | ||
| err := pluginContext.K8sReader().List(ctx, podList) | ||
| err := pluginContext.K8sReader().List(ctx, podList, client.InNamespace(namespace)) |
There was a problem hiding this comment.
Could we add a ray.io/cluster=<RayClusterName> label filter?
There was a problem hiding this comment.
Done: head readiness and the log context now select ray.io/cluster=<RayClusterName>.
| statusDetails, _ := utils.MarshalObjToStruct(app.Status) //nolint: staticcheck | ||
| podList := &v1.PodList{} | ||
| err = pluginContext.K8sReader().List(ctx, podList) | ||
| err = pluginContext.K8sReader().List(ctx, podList, client.InNamespace(app.Namespace)) |
There was a problem hiding this comment.
Could we add a training.kubeflow.org/job-name label filter?
There was a problem hiding this comment.
Done, using kubeflowv1.JobNameLabel.
| func isDaskSchedulerReady(ctx context.Context, namespace, daskJobName string, pluginContext k8s.PluginContext) (bool, error) { | ||
| podList := &v1.PodList{} | ||
| err := pluginContext.K8sReader().List(ctx, podList) | ||
| err := pluginContext.K8sReader().List(ctx, podList, client.InNamespace(namespace)) |
There was a problem hiding this comment.
Could we add a dask.org/cluster-name label filter?
There was a problem hiding this comment.
Done for both scheduler lookups. The operator names a DaskJob’s cluster after the job, so the value is the job name.
|
cc @AdilFayyaz |
6226b3f to
0c6e5ed
Compare
madiyar-wayve
changed the title
0c6e5ed to
e853be2
Compare
Ray, Dask, PyTorch and clustered listed pods cluster-wide and matched them by name prefix, which breaks once TaskActions live in separate namespaces. The executor now limits the plugin reader to the job's namespace, and each plugin selects its pods by the label its operator sets (Ray cluster and submitter Job, Dask cluster, PyTorch job, JobSet name). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Madiyar Aitzhanov <madiyar.aitzhanov@wayve.ai>
e853be2 to
618ed4f
Compare
Thanks for the review @popojk! I made plugin manager to wrap the reader so every List is limited to the namespace of the resource it watches, and removed the per-plugin |
This branch has not been deployed
2 participants
Tracking issue
Related to #8051
Why are the changes needed?
The executor gives plugins a reader over every Flyte pod in the cluster. Ray, Dask, PyTorch and the clustered (JobSet) plugin list all of them and pick their own by name prefix, or not at all. Once TaskActions live in per-project-domain namespaces (#8051), the same
<run>-<action>names can exist in two namespaces:Even within one namespace, a name prefix can match another job whose name starts with this one's, and the PyTorch and clustered log contexts had no filter at all.
What changes were proposed in this pull request?
Ray's log context also keeps the RayJob's submitter pod, selected by its Kubernetes Job name, since in K8sJobMode it carries the driver's output.
How was this patch tested?
New unit tests, which fail without this change: a plugin-manager test with pods in two namespaces, and plugin tests that add another job's pod (for Ray and Dask, one whose name shares the prefix) and check it is ignored. A Ray test checks the job's own submitter pod stays in the log context.
go test ./flyteplugins/go/tasks/plugins/k8s/... ./executor/...Labels
Check all the applicable boxes
🤖 Generated with Claude Code