fix: prevent composed clustered tasks from getting stuck in RUNNING due to long JobSet pod names - #7683
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents composed/nested clustered tasks (JobSet-backed) from hanging in RUNNING when long generated names cause JobSet admission to deterministically reject derived Job/Pod names.
Changes:
- Add deterministic
buildJobSetNameto sanitize/truncate generated names (with a hash) so JobSet-derived names stay within DNS label limits. - Ensure create and lookup/abort paths derive the same JobSet name (BuildResource + BuildIdentityResource).
- Update the executor’s K8s plugin manager to (a) not overwrite plugin-chosen object names and (b) fast-fail on deterministic
Invalidadmission errors instead of retry-looping.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| flyteplugins/go/tasks/plugins/k8s/clustered/util.go | Introduces bounded, deterministic JobSet naming to prevent webhook rejections from long composed names. |
| flyteplugins/go/tasks/plugins/k8s/clustered/build.go | Uses the bounded JobSet name for resource creation (and subdomain/service naming). |
| flyteplugins/go/tasks/plugins/k8s/clustered/plugin.go | Aligns identity (lookup/abort) naming with the create path for consistent reconciliation. |
| flyteplugins/go/tasks/plugins/k8s/clustered/clustered_test.go | Adds unit tests validating name sanitization, uniqueness, and derived-name length constraints. |
| executor/pkg/plugin/k8s/plugin_manager.go | Preserves plugin-set names and fails permanently on deterministic Invalid create errors. |
| executor/pkg/plugin/k8s/plugin_manager_test.go | Adds tests covering name preservation and Invalid fast-fail behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f52348c to
daf1869
Compare
| "version": "2.13.0", | ||
| "resolved": "https://registry.npmjs.org/@bufbuild/protobuf/-/protobuf-2.13.0.tgz", | ||
| "integrity": "sha512-acq7c49vxfm1ggJ95P70TX7ABDM0vxr1SYD3BB0o0jnBLB4OAqeHyKuN+cD3w80gXEDQ2zxHpR6CUeA+O/aU9g==", |
daf1869 to
56291bc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
executor/pkg/plugin/k8s/plugin_manager.go:128
- The PR description says
addObjectMetadatanow preserves a plugin-supplied object name (only defaulting to the generated name when the plugin didn’t set one). In the current implementation, the name is still unconditionally overwritten (o.SetName(name)), so a plugin that explicitly setsmetadata.name(e.g. to a bounded/derived value) would still get clobbered. If name-preservation is required for this fix,addObjectMetadataneeds to be updated accordingly; otherwise the PR description/test plan should be corrected to match the actual behavior.
// Admission/validation rejections (e.g. a generated object or derived child name
// exceeding k8s length limits) are deterministic: retrying re-submits the identical
// object and fails the same way, leaving the execution stuck RUNNING. Fast-fail
// instead of looping via UnknownTransition.
if k8serrors.IsInvalid(err) {
return pluginsCore.DoTransition(pluginsCore.PhaseInfoFailure("InvalidResource", err.Error(), nil)), nil
}
Signed-off-by: M. Adil Fayyaz <62440954+AdilFayyaz@users.noreply.github.com>
…chanism The plugin manager now stamps object names via GetGeneratedNameWith(0, GeneratedNameMaxLength) on both the create and lookup paths, so the clustered plugin no longer needs to name the JobSet itself: - Drop the name from BuildIdentityResource; the manager owns naming. - BuildResource derives the same name only for the pod subdomain, which must equal the JobSet name for pod DNS to resolve. - Delete buildJobSetName/toDNS1035Label; the DNS-1035 sanitization moves to a shared ConvertToDNS1035LabelCompatibleString util applied inside the executor's GetGeneratedNameWith, since FixedLengthUniqueID passes short names through verbatim and action names aren't constrained to label-safe form (dots / leading digits would be rejected by the JobSet webhook). - Keep the replicas cap and the GeneratedNameMaxLength property bound (63 minus the worst-case JobSet pod-name suffix). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: M. Adil Fayyaz <62440954+AdilFayyaz@users.noreply.github.com>
56291bc to
b81ec4f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
flyteplugins/go/tasks/plugins/k8s/clustered/util.go:44
- PR description mentions adding/using a new buildJobSetName helper to sanitize/truncate JobSet names, but the implementation in this PR appears to rely entirely on PluginProperties.GeneratedNameMaxLength + TaskExecutionID.GetGeneratedNameWith(...) instead (and there is no buildJobSetName symbol in the repo). Please update the PR description to match the current approach, or add the helper if it is still required for other call paths.
// generatedNameMaxLength bounds the task's generated name via the plugin's
// PluginProperties. The plugin manager stamps the object name with
// GetGeneratedNameWith(0, GeneratedNameMaxLength) — DNS-1035-sanitized and bounded —
// on both the create and lookup paths. JobSet's admission webhook computes the
// longest child pod name as "<jobSetName>-<replicatedJob>-<jobIdx>-<podIdx>-<5-char
executor/pkg/plugin/k8s/plugin_manager.go:126
- PR description says addObjectMetadata was changed to avoid clobbering a plugin-provided object name, but addObjectMetadata still unconditionally sets o.SetName(name) based on the generated name (executor/pkg/plugin/k8s/plugin_manager.go:79-103). If name preservation is still intended (e.g. for plugins that must set a bounded name), please either update the description or include that code change here.
// Admission/validation rejections (e.g. a generated object or derived child name
// exceeding k8s length limits) are deterministic: retrying re-submits the identical
// object and fails the same way, leaving the execution stuck RUNNING. Fast-fail
// instead of looping via UnknownTransition.
if k8serrors.IsInvalid(err) {
| // (leading letter, no dots). FixedLengthUniqueID passes names that already fit through | ||
| // verbatim — without sanitizing first, a generated name with a dot or leading digit |
There was a problem hiding this comment.
Do we really need ConvertToDNS1035LabelCompatibleString? dot and leading digit aren't not allow in the run name now in cloud. I think we need to add the same check in oss run service as well
There was a problem hiding this comment.
we can add that in a separate PR
There was a problem hiding this comment.
OSS doesnt have any validation for run name rn
Makes sense to add it but that change should land before this PR gets merged. I'll create a new one for this.
Signed-off-by: M. Adil Fayyaz <62440954+AdilFayyaz@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
executor/pkg/plugin/k8s/plugin_manager.go:128
- PR description says addObjectMetadata “only default[s] the object name … when the plugin didn't set one”, but addObjectMetadata still unconditionally calls o.SetName(name) (plugin_manager.go:83–90), which will clobber any plugin-supplied name. Either update addObjectMetadata to respect an existing name or adjust the PR description so reviewers/users aren’t relying on behavior that isn’t implemented.
// Admission/validation rejections (e.g. a generated object or derived child name
// exceeding k8s length limits) are deterministic: retrying re-submits the identical
// object and fails the same way, leaving the execution stuck RUNNING. Fast-fail
// instead of looping via UnknownTransition.
if k8serrors.IsInvalid(err) {
return pluginsCore.DoTransition(pluginsCore.PhaseInfoFailure("InvalidResource", err.Error(), nil)), nil
}
flyteplugins/go/tasks/plugins/k8s/clustered/util.go:43
- The comment says GetGeneratedNameWith(...) is “DNS-1035-sanitized”, but GetGeneratedNameWith only truncates/hashes (see executor/pkg/plugin/task_exec_metadata.go:35–37) and the plugin manager only enforces/sanitizes DNS-1123 subdomain in addObjectMetadata. Please reword this to avoid implying DNS-1035 sanitization happens here.
// generatedNameMaxLength bounds the task's generated name via the plugin's
// PluginProperties. The plugin manager stamps the object name with
// GetGeneratedNameWith(0, GeneratedNameMaxLength) — DNS-1035-sanitized and bounded —
// on both the create and lookup paths. JobSet's admission webhook computes the
Why are the changes needed?
Composed/nested clustered tasks generate long action names. The JobSet plugin derived its resource name directly from that generated name, so JobSet's admission webhook rejected the child pods (
<jobSetName>-workers-0-<podIdx>-<suffix>exceeded the 63-char DNS-1035 label limit). The plugin retried the identical, always-invalid object forever, leaving the execution stuck in RUNNING.What changes were proposed in this pull request?
How was this patch tested?
validation.IsDNS1035Label).
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Stack
If you do use
git townto manage PR Stacks, the stack relevant to this PRwill show below. Otherwise, you can ignore this section.
Docs link