docs: GREP-0285 job support proposal - #696
shayasoolin wants to merge 30 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ase model sections Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion criteria, and alternatives Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mpletion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…starts:0, phase struct, cleanup wording, event table Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and KubeRay Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use spec.template.cliques[] with podSpec.restartPolicy for the all-ranks example, and spec.template.podCliqueScalingGroups[].cliqueNames for the leader-driven example. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the proposal. Could you help me understand the intended behavior when |
Good question. I think there are two coherent options here: Option A: allow non-required completion-aware children, but define precedence clearly. Option B: require children outside I prefer Option A. It is less strict, fits better with the rest of the current design, and keeps more scenarios valid. I tend to think it may be problematic to disable gang-terminations for such leader-worker combinations, requiring the workers to always restart individually. What do you think? What will better serve the applications running on top of this? |
Thanks for the thoughtful reply. I agree that Option A seems like the better fit, since it preserves gang-level failure handling while supporting leader-driven completion. |
|
|
||
| **New conditions.** Two terminal conditions are added across all three resource types: | ||
|
|
||
| - `Completed` — all required pods or completion-aware children have succeeded. |
There was a problem hiding this comment.
Can we call this condition as Succeeded?
There was a problem hiding this comment.
I checked the closest APIs. My recommendation is to rename the success condition from Completed to Complete, but not Succeeded:
- Kubernetes Job - the terminal condition types are
CompleteandFailed. - Kubernetes API conventions allow
Succeeded, but that is generic guidance, the concrete batch Job API usesComplete. - JobSet currently uses
Completed, but that looks like an inconsistency: it also usesCompletein related places, and there is an open discussion about renamingCompletedtoComplete. - Kubeflow is mixed: older Training Operator uses
Succeeded, while newer Trainer usesComplete. - KubeRay also distinguishes app-level job result (SUCCEEDED/FAILED) from Kubernetes deployment lifecycle (
Complete/Failed).
I’d keep spec.policy.completion and failure.maxRestarts. I’d consider renaming success.completedNames, since it sounds like observed status. success.requiredNames or success.targetNames will be clearer.
What do you think? cc @nvrohanv
|
|
||
| 1. *Replica state*: a replica is **Completed** when all required completion-aware child `PodClique`s are `Completed` — either all completion-aware children, or the named children in `policy.completion.success.completedNames` when it is set. A replica is permanently **Failed** when a required child `PodClique` fails and no restart budget remains (`maxRestarts` exhausted, or `maxRestarts: 0`). A failure of a completion-aware `PodClique` not listed in `completedNames` triggers a gang restart and consumes budget while budget remains; it does not mark the replica as `Failed` until no restart budget remains. | ||
|
|
||
| 2. *PCSG state*: the PCSG is **Completed** when all replicas are `Completed`. It is **Failed** when enough replicas have exhausted their budget that the remaining replicas cannot satisfy the completion criterion. |
There was a problem hiding this comment.
How does this work with MinAvailable? Or should the semantics be driven by MinAvailable, i.e. a PCSG is considered successfully completed when at least MinAvailable replicas have completed with success.
There was a problem hiding this comment.
What is the concern exactly with minAvailable? What kind of conflict or issue do you see?
For jobs, I expect minAvailable to be equal to replicas, but since replicas is the higher number, and we do not support partial completion at this first release (as agreed), I think the right thing to do is to follow replicas as the deciding value here. I don't see a semantic issue but maybe I'm missing something so please elaborate more.
|
|
||
| 1. *Replica state*: a replica is **Completed** when all required completion-aware direct children are `Completed` — either all completion-aware `PodClique`s and `PodCliqueScalingGroup`s, or the named children in `policy.completion.success.completedNames` when it is set. A replica is permanently **Failed** when a required direct child fails and no restart budget remains. A failure of a completion-aware direct child not listed in `completedNames` follows the same gang-restart behavior as PCSG: it consumes budget while budget remains, and fails the replica when no budget remains. | ||
|
|
||
| 2. *PCS state*: the PCS is **Completed** when all replicas are `Completed`. It is **Failed** when enough replicas have exhausted their budget that the remaining replicas cannot satisfy the completion criterion. |
There was a problem hiding this comment.
Again, there needs to be a consideration of MinAvailable number of replicas to complete with succeed for the PCS to be considered as succeeded.
unmarshall
left a comment
There was a problem hiding this comment.
1/n review - will post the comments in iterations
|
|
||
| Regular-mode `PodClique`s within a `PodCliqueScalingGroup` or `PodCliqueSet` are excluded from completion evaluation. A resource can be Completed even if some of its children remain running in regular mode. | ||
|
|
||
| **Gang termination for completion-aware resources.** Gang scheduling is unchanged: `minAvailable` continues to gate pod launch until the full gang can be placed simultaneously, on initial start and after each restart. `minAvailable` also continues to be passed to scheduler backends as the gang's minimum member count. For gang termination, completion-aware resources use the `Failed` condition as the termination signal instead of `MinAvailableBreached`, and fire immediately without `terminationDelay`. A pod failure or eviction in a completion-aware `PodClique` moves the pod to `pod phase=Failed`, causing the owning `PodClique` to set the `Failed` condition and triggering this gang-termination path. Crucially, the termination scope is identical to the existing gang-termination implementation: the same gang boundary that would have been torn down by a `MinAvailableBreached` trigger is torn down by a `Failed` trigger. Completion-aware behavior changes the runtime trigger, not the scheduling contract or termination scope. |
There was a problem hiding this comment.
A pod failure or eviction in a completion-aware
PodCliquemoves the pod topod phase=Failed, causing the owningPodCliqueto set theFailedcondition and triggering this gang-termination path.
A pod eviction causing PCLQ to transition to FAILED state is a bit too harsh. Also this will then cause a restart/recreate subtracting from the restart/recreate budget. Slurm distinguishes infrastructure loss or scheduler preemption from application failure (container exiting with non-zero exit code). JOB_NODE_FAIL is a separate state from JOB_FAILED. JobRequeue requeues batch jobs on node failure/preemption instead of failing them. Automatic requeues are capped by MaxBatchRequeue. Similarly if a job is preempted then its quite visible as PREEMPTED (never FAILED). As far as i know preemption-driven restarts/reschedules are effectively uncapped in Slurm.
In k8s changes of node failures and preemption very much exists and therefore categorizing all of these under FAILED is IMO not correct.
There was a problem hiding this comment.
It's intentional. As described in the GREP, the majority/all of the distributed training frameworks cannot tolerate a single worker's failure. They expect the whole group to be restarted, and re-sync with the leader. Please correct me if this is a wrong or inaccurate observation. Therefore, due to this application-level expectation, we prefer to treat the whole PCLQ as failed rather than restart a single failed or evicted pod.
This is also not the final word here. We will be able to add a more nuanced restart policy for pods within PCLQ in later releases.
If it's important for this first release, we can consider having a dedicated phase for preemption, which is different than failure, but still keep the consequences of it as a failed PCLQ. Otherwise it's a bigger change while we really prefer to keep this first release minimal.
| - Retain terminal pods (`Succeeded`, `Failed`) for log access via `kubectl logs`, unless the `PodClique` is deleted as part of a gang restart. | ||
|
|
||
| **On `PodCliqueScalingGroup` replica terminal state:** | ||
| - Delete active pods and non-terminal child `PodClique`s belonging to that replica. |
There was a problem hiding this comment.
This is perhaps in line with the user story 3 that mentions that if the leader completes while workers are still running, you will start the cleanup. Slurm provides WaitTime and the default wait time is 0 (which means wait indefinitely). These workers are termed as stragglers. For training this is important. A worker receiving a SIGTERM in the middle of a checkpoint flush or a final all_gather produces corrupt artifacts.
The only saving grace is terminationGracePeriodSeconds that are set on the Pod but this is always optional. So either make this explicit in the GREP or provide an explicit wait time.
| - Support gang restart for completion-aware parent scopes: when a required `PodClique` or `PodCliqueScalingGroup` fails, the parent deletes and recreates the affected scope as a unit, consuming from a per-replica restart budget. | ||
| - Guarantee that terminal states are persisted to status before any pod cleanup, and that terminal pods (`Succeeded`, `Failed`) from final terminal scopes are retained for log access until the workload is deleted. | ||
|
|
||
| ### Non-Goals |
There was a problem hiding this comment.
Will you support for rolling updates for running Jobs?
If yes then how will this impact restart budgets and how will you do the updates? If no, then what happens when a PCS is changed, thus changing its hash? Will you reject the change in the webhook itself? There is no mention of that in this GREP. If you do not wish to support that then please mention this as a non-goal and explicitly state how will this prevented.
There was a problem hiding this comment.
Can we assume in the job mode these components run to completion?
There was a problem hiding this comment.
Not supported.
Clarified in ea281e9:
- Added rolling updates as a non-goal.
- Stated that the validation webhook rejects edits that would update running job pods or change generated child-resource hashes.
| // PodCliqueScalingGroup or PodCliqueSet replica to be considered Completed. | ||
| // If omitted, all completion-aware direct children must complete. | ||
| // +optional | ||
| CompletedNames []string `json:"completedNames,omitempty"` |
There was a problem hiding this comment.
This is not an accurate name and IMO should be changed. completedNames is past tense and reads like its an observed state when its actually part of the desired-state (spec) and signals intent.
Alternatives: : targetNames, requiredNames
I would recommend:
success:
targetNames: [leader]This reads as "success targets all of these names"
So tomorrow when you support Any (that JobSet also supports) then it becomes:
success:
targetNames: [leader, backup-leader]
operator: AnyThis reads as "success targets any of these names"
There was a problem hiding this comment.
Agree and this indeed was the plan following another discussion. Renamed completedNames to targetNames, see 6e362e0.
unmarshall
left a comment
There was a problem hiding this comment.
2/n review. More will follow
| - Define `policy.completion.failure.maxRestarts` on `PodClique`, `PodCliqueScalingGroup`, and `PodCliqueSet`; `PodClique` supports only `0` in this release, while `PodCliqueScalingGroup` and `PodCliqueSet` use it as a per-replica gang restart budget. | ||
| - Define `policy.completion.success.completedNames` on `PodCliqueScalingGroup` and `PodCliqueSet` to support named-child completion criteria. | ||
| - Support gang restart for completion-aware parent scopes: when a required `PodClique` or `PodCliqueScalingGroup` fails, the parent deletes and recreates the affected scope as a unit, consuming from a per-replica restart budget. | ||
| - Guarantee that terminal states are persisted to status before any pod cleanup, and that terminal pods (`Succeeded`, `Failed`) from final terminal scopes are retained for log access until the workload is deleted. |
There was a problem hiding this comment.
Guarantee that terminal states are persisted to status before any pod cleanup
There are case like API initiated eviction (node-drain, eviction sub resource) or scheduler pre-emption or a node loss (force-delete) or a taint-based eviction. In these case kubelet will not update Pod phase to Failed. So in these cases failure must be inferred from absence of the Pod and not from a pod in failed phase. This GREP's whole failure story is written around observing phase=Failed. For such pods the retention promise will break. Since this GREP also promises that terminal pods are retained for log access - this should be corrected.
IMO, terminal pod retention is best-effort (not a guarantee) and must be documented clearly.
There was a problem hiding this comment.
Thanks, indeed we previously assumed that an evicted/failed node will lead to a failed pod, but this is not necessarily the case. cc @sanjaychatterjee
Fixed in 322106c.
| - **No pod-level retry.** A failed pod within a `PodClique` is not replaced in isolation. `PodClique` `policy.completion.failure.maxRestarts` may only be `0` in this release; values greater than `0` are future work. This is acceptable for the first release because current distributed training frameworks generally do not tolerate replacing a single failed worker independently. A single worker failure usually requires restarting the whole group, so gang restart covers the common recovery path. | ||
| - **No scaling for completion-aware workloads.** Completion-aware workloads use fixed replica counts. Grove rejects autoscaling configuration and manual replica changes for resources with `policy.completion` and for parent scopes that contain completion-aware direct children. | ||
| - `completions` and `completedIndexes` — configurable completion counts and index-based filtering at the `PodCliqueScalingGroup` and `PodCliqueSet` levels. In this release, all replicas must complete successfully for a resource to be considered Completed. | ||
| - Runtime deadline support (`maxRuntime`). Deadline semantics require a separate design for resource-level versus replica-level or attempt-level limits, and whether deadlines reset on gang restart. Since runtime deadlines are orthogonal to completion tracking and gang restart, they are deferred to keep the first release focused. |
There was a problem hiding this comment.
Since runtime deadlines are orthogonal to completion tracking and gang restart
Choice of words is not accurate. Runtime deadline is separable but not orthogonal to gang restart but it is orthogonal to completion tracking.
Why its not orthogonal to gang restart - upon gang restart do we reset the stopwatch? So the deadline could be per-attempt deadline or deadline across restarts. So you see it's not orthogonal.
Perhaps it can be reworded to below:
A runtime deadline does not affect how we detect completion. Completion is decided by pod exit codes, not by a clock. So we can add `maxRuntime` later without changing completion tracking.
A runtime deadline does interact with gang restart, but only in one way: when a gang restarts, we must decide whether the deadline clock resets or keeps running. This is a small, self-contained question. It does not change the core completion or restart logic. Because the core does not depend on it, and its only interaction with gang restart is limited and well-scoped, we defer maxRuntime to keep the first release focused.
|
|
||
| 1. *Replica state*: a replica is **Completed** when all required completion-aware child `PodClique`s are `Completed` — either all completion-aware children, or the named children in `policy.completion.success.completedNames` when it is set. A replica is permanently **Failed** when a required child `PodClique` fails and no restart budget remains (`maxRestarts` exhausted, or `maxRestarts: 0`). A failure of a completion-aware `PodClique` not listed in `completedNames` triggers a gang restart and consumes budget while budget remains; it does not mark the replica as `Failed` until no restart budget remains. | ||
|
|
||
| 2. *PCSG state*: the PCSG is **Completed** when all replicas are `Completed`. It is **Failed** when enough replicas have exhausted their budget that the remaining replicas cannot satisfy the completion criterion. |
There was a problem hiding this comment.
It is Failed when enough replicas have exhausted
what does enough replicas mean here? This is very vague. Either there is a configurable value or for this iteration it defaults to 1 (so even if 1 exhausts its restart budget, it's considered failed). Can you please be precise here?
There was a problem hiding this comment.
The answer is in the second half of the sentence you've quoted:
that the remaining replicas cannot satisfy the completion criterion
Will be a bit clearer if I use "such that" instead of "that".
| - **No scaling for completion-aware workloads.** Completion-aware workloads use fixed replica counts. Grove rejects autoscaling configuration and manual replica changes for resources with `policy.completion` and for parent scopes that contain completion-aware direct children. | ||
| - `completions` and `completedIndexes` — configurable completion counts and index-based filtering at the `PodCliqueScalingGroup` and `PodCliqueSet` levels. In this release, all replicas must complete successfully for a resource to be considered Completed. | ||
| - Runtime deadline support (`maxRuntime`). Deadline semantics require a separate design for resource-level versus replica-level or attempt-level limits, and whether deadlines reset on gang restart. Since runtime deadlines are orthogonal to completion tracking and gang restart, they are deferred to keep the first release focused. | ||
| - Pod cleanup policies other than the fixed default (retain terminal pods from final terminal scopes, delete active pods on terminal state). |
There was a problem hiding this comment.
This sentence is not easy for me to read. Would it make sense to rephrase it?
My attempt at rewording:
Pod cleanup uses a single fixed policy: when a resource reaches a terminal state, still-active pods (Running/Pending) in that scope are deleted, while already-finished pods (Succeeded/Failed) in a final terminal scope are retained for log access. Configurable cleanup policies and TTL-based deletion are out of scope
Also no where in the GREP I could find the definition of terminal scope. Also "final" in final terminal scopes is also undefined and is left open for user interpretation.
My recommendation:
Scope: A unit that can independently reach a terminal state. e.g. `PodClique`, a `PodCliqueScalingGroup` replica, `PodCliqueScalingGroup` resource, a `PodCliqueSet` replica or the `PodCliqueSet` resource.
Terminal Scope: A scope whose status is `Completed` or `Failed`.
Final Terminal Scope: A terminal scope that will not be retried (success or failure with no restart budget remaining). A scope that is failed but still has a restart budget is `terminal-but-not-final` - it is deleted and recreated by gang restart, so its pods (including finished ones) are not retained.
There was a problem hiding this comment.
Revised. I removed the "final terminal" wording everywhere, it doesn't sound right.
See a05504f.
unmarshall
left a comment
There was a problem hiding this comment.
3/n review. More will follow
| // +optional | ||
| Policy *WorkloadPolicy `json:"policy,omitempty"` | ||
|
|
||
| type WorkloadPolicy struct { |
There was a problem hiding this comment.
First issue: Workload is IMO not accurate.
Reasons:
- Collision with
WorkloadAPI (k8s scheduling API), Kueue'sWorkloadAPI object (that is the unit of admission/queueing). This creates confusion. Workload- binds to no specific level. The same type is embedded in PodCliqueSpec, PodCliqueScalingGroupSpec and PodCliqueSetSpec. So now workload is an undefined concept that is smuggled in via a type.
Alternative: RunPolicy - kubeflow uses this and its quite accurate.
Second observation: one shared type across levels.
success.completedNames/targetNames- invalid on PCLQ, valid on PCSG and PCS.- failure.maxRestarts — valid everywhere, but on PodClique only 0 is legal.
So additional validations needs to be introduced to prevent setting fields inside WorkloadSpec at different levels. Creates a cognitive overload.
There was a problem hiding this comment.
Updated GREP to use RunPolicy instead of WorkloadPolicy, with separate types:
PodCliqueRunPolicyforPodCliqueRunPolicyforPodCliqueScalingGroupandPodCliqueSet
See 916affe.
| // +optional | ||
| Policy *WorkloadPolicy `json:"policy,omitempty"` | ||
|
|
||
| type WorkloadPolicy struct { |
There was a problem hiding this comment.
My recommendation that you could consider:
type RunPolicy struct {
// Completion configures completion-aware behaviour.
// +optional
Completion *CompletionPolicy `json:"completion,omitempty"`
// (future: MaxRuntime, TTL, CleanupPolicy, ... live here)
}
type CompletionPolicy struct {
// +optional
Success *CompletionSuccessPolicy `json:"success,omitempty"`
// +optional
Failure *CompletionFailurePolicy `json:"failure,omitempty"
}
// +kubebuilder:validation:XValidation:rule="self.type == 'NamedChildren' ? has(self.targetNames) :
!has(self.targetNames)",message="targetNames must be set iff type is NamedChildren"
// +union
type CompletionSuccessPolicy struct {
// +unionDiscriminator
// +kubebuilder:default=AllChildren
Type SuccessType `json:"type"`
// TargetNames lists the direct children that must complete when type is NamedChildren.
// +optional
// +listType=atomic
// +kubebuilder:validation:MinItems=1
TargetNames []string `json:"targetNames,omitempty"`
}Benefits:
- Explicit
SuccessType: With the flat completedNames, "field absent" silently overloaded to "all children must complete," which is ambiguous: did the author mean all, or did they forget to set it? The discriminator forces the choice to be stated (type: AllChildren vs type: NamedChildren)
It now reads as:
success:
type: NamedChildren
targetNames: [leader] success:
type: AllChildrenThe job mode is currently decided by presence of leaf. First I could not really understand what is leaf as it's a term that is not even defined in this GREP. But then i finally understood that it only means that somewhere deep inside completion is set (at the PCLQ level).
That raises another API consumption question:
In the GREP the choice is made to force the user to specify:
policy:
completion:
failure:
maxRestarts: 0even though the default value of maxRestarts is 0 for PCLQ. This is not so nice.
You should simply allow definition of an empty completion and use it as a marker which signals 2 things:
- PCLQ is completion-aware
- Default values must apply
So this allows:
policy:
completion: {}to be a valid construct. I find this a LOT more intuitive and easy to reason than forcing the user to specify the entire defaults.
There was a problem hiding this comment.
Agree with this API suggestion.
There was a problem hiding this comment.
@sanjaychatterjee I'm surprised you agree, as we had a very specific meeting exactly about that, and agreed with @Ronkahn21 and @nvrohanv that an empty completion section {} is a poor signal for a "job-mode resource". You preferred (and I agreed) that the user will explicitly specify at least one member field e.g. maxRestarts, even that it has a default value.
Second point, I don't see the value in success.type. The default is an empty targetNames field, which means ALL names should complete. If a subset is set, then this is the subset. I think it's cleaner and easier. It also saves the case of specifying a list of children but using AllChildren as the type.
There was a problem hiding this comment.
@sanjaychatterjee I'm surprised you agree, as we had a very specific meeting exactly about that, and agreed with @Ronkahn21 and @nvrohanv that an empty completion section {} is a poor signal for a "job-mode resource".
I am not sure why is that considered a poor choice. I can quote some examples in k8s space:
- emptyDir: {} — VolumeSource.EmptyDir *EmptyDirVolumeSource. Every field in EmptyDirVolumeSource is optional, so emptyDir: {} is the idiomatic way to request the volume with defaults. Presence selects the type.
- rollingUpdate: {} — DeploymentStrategy.RollingUpdate *RollingUpdateDeployment (same for StatefulSet/DaemonSet). Both maxUnavailable and maxSurge are optional and default to 25%, so an empty block is valid and simply means "rolling update with defaults."
- securityContext: {} — PodSpec.SecurityContext *PodSecurityContext; all fields optional, presence-based.
- podFailurePolicy / successPolicy on batch/v1 Job — optional struct pointers whose presence turns on that evaluation path.
Maybe there are more such cases.
You preferred (and I agreed) that the user will explicitly specify at least one member field e.g. maxRestarts, even that it has a default value.
That is not a clean design IMHO. Completion is a pointer field.
If it is nil for a component then its not completion-aware.
if it is not-nil then it is completion aware.
Now given this - If the user does not really wish to configure any values and is happy with the defaults then he should not be asked to explicitly re-mention the defaults. An empty struct != nil and thus makes the component completion aware.
There was a problem hiding this comment.
Second point, I don't see the value in success.type. The default is an empty targetNames field, which means ALL names should complete. If a subset is set, then this is the subset. I think it's cleaner and easier. It also saves the case of specifying a list of children but using AllChildren as the type.
Sure we can go with this. If in future you have a need for specifying % of workers to succeed then you can add another field MinSucceeded. You can then model JobSets Any operator by using the int value of (1) and if there is a need can also specify that at least 50% of the workers should succeed. I think this might only be needed when there is elastic training workloads, right?
There was a problem hiding this comment.
Re completion: {} - okay, I agree with that. I wasn't familiar with the common usage of this pattern. That changes the picture for me.
Glad we agree to defer successType. It's relevant for elastic training but not only. We have a customer use-case of MoE training where they define a threshold of replicas that need to succeed (more precisely, an index range). It's not exactly a list of names but it goes along the same lines.
unmarshall
left a comment
There was a problem hiding this comment.
4/n review comments. More will follow
| **PCLQ failure handled by completion-aware PCSG:** | ||
|
|
||
| 1. The PCSG increments `replicaRestartCounts[replicaIndex]`. | ||
| 2. If the budget is not exhausted: the PCSG deletes the failed `PodClique` and recreates it from the template. The new `PodClique` is placed by the scheduler as a complete gang before any pods run. |
There was a problem hiding this comment.
A PCSG can comprise of one more PCLQs. All the PCLQs are gang scheduled together. Unit of gang scheduling for a PCSG is a single PCSG replica. So your statement that a PCLQ in a PCSG is recreated and it will be gang scheduled is incorrect.
So if you only delete a single PCLQ and then it gets scheduled not-as-a-gang but as pods wherever there is capacity then you lose:
- Atomicity: You can only get partial placement while the surviving leader idles at a barrier. No all-or-nothing guarantee is applicable here.
- Topology: the whole reason for gang + topology-aware placement is co-locating the ranks. New pods scheduled against fragmented leftover capacity may land topologically far from the survivors, degrading the collective — exactly the scenario a training job restart is supposed to fix, not create
So you must delete all PCLQs for this PCSG replica.
There was a problem hiding this comment.
I see that the phrasing can be better and I'll correct it.
As stated elsewhere in the GREP, a PCS/G is in "completion aware" if ANY of its children is.
A failed PCLQ within a PCSG will cause the whole PCSG replica to restart - just as you'd expect.
| - **`policy.completion.success.completedNames`** *(PodCliqueScalingGroup / PodCliqueSet only)*: Named direct children within a replica that must complete for that replica to count as Completed. If omitted, all completion-aware direct children must complete. At the `PodClique` level, all pods must complete for the `PodClique` to be considered Completed. | ||
| - **`policy.completion.failure.maxRestarts`** *(PodClique / PodCliqueScalingGroup / PodCliqueSet)*: Restart budget. On `PodCliqueScalingGroup` and `PodCliqueSet`, this is a per-replica gang restart budget. On `PodClique`, only `0` is supported in this release, meaning any pod failure makes the `PodClique` Failed. | ||
|
|
||
| Regular-mode `PodClique`s within a `PodCliqueScalingGroup` or `PodCliqueSet` are excluded from completion evaluation. A resource can be Completed even if some of its children remain running in regular mode. |
There was a problem hiding this comment.
Is it possible to have a regular-mode PCLQ and a completion-aware PCLQ within the same PCSG? I do not see the GREP mentioning that it's disallowed. So for now I will assume that this is indeed allowed. There are now repurcursions.
So for example lets take a PCSG that has 2 PCLQs:
pclq-a : this is completion-aware
pclq-b: this is regular-mode
- If pclq-a fails, then to ensure gang-scheduling we must recreate the PCSG replica. This now impacts the pclq-b which is now swept with pclq-b. Is this intended? If yes what is the use case?
- GREP's own cleanup says "once the replica is terminal, the PCSG controller deletes the non-terminal child PodCliques or active pods in that replica." → the regular PCLQ is deleted.
My view:
- If two PCLQs are tightly coupled enough to require co-scheduling and topo-packing → they share a fate, so both should be finite (or both long-running).
- If they genuinely have independent lifecycles (one finite, one long-running) → they shouldn't be in the same gang in the first place; they should be separate PodGangs (e.g. a separate standalone PCLQ / separate PCSG) so their scheduling and lifecycle aren't fused.
So I am not aware of any use case where a long-running service is co-scheduled/gang-scheduled and topo-packed with a finite training job in the same PCSG.
If you want to have - "a member that runs in the gang and shares its fate, but does not gate success" then the API already has provision for that via targetNames. Do not list it in there. That will express: in the gang but does not gate success coherently.
There was a problem hiding this comment.
Please see the previous reply:
As stated elsewhere in the GREP, a PCS/G is in "completion aware" if ANY of its children is.
A failed PCLQ within a PCSG will cause the whole PCSG replica to restart - just as you'd expect.
| ### Limitations/Risks & Mitigations | ||
|
|
||
| **Application-level hangs without pod failure.** | ||
| For completion-aware resources, `MinAvailableBreached`-based gang termination is disabled. Ordinary pod failures and evictions are still handled: the pod reaches `pod phase=Failed`, the owning `PodClique` sets the `Failed` condition, and Grove terminates or restarts the gang through the failure path. The remaining risk is narrower: if the cluster does not surface a failed pod and the application keeps running despite a lost peer or broken collective, Grove cannot infer the application-level deadlock from availability alone. Workloads should use framework-level failure detection, such as rendezvous timeouts, and exit non-zero when peer loss makes progress impossible. |
There was a problem hiding this comment.
MinAvailableBreached-based gang termination is disabled.
We need to be precise here. What does disabled mean here?
Does it mean that this condition is never set or does it mean that it is set but ignored?
If it is never set then it's fine. If its set but ignored, then it will have True during successful completion which is very misleading as its still set in the status and potentially risky if later code evolution opens a closed door which reacts on it.
There was a problem hiding this comment.
The condition will not be set. I'll clarify that in the doc.
|
|
||
| Regular-mode `PodClique`s within a `PodCliqueScalingGroup` or `PodCliqueSet` are excluded from completion evaluation. A resource can be Completed even if some of its children remain running in regular mode. | ||
|
|
||
| **Gang termination for completion-aware resources.** Gang scheduling is unchanged: `minAvailable` continues to gate pod launch until the full gang can be placed simultaneously, on initial start and after each restart. `minAvailable` also continues to be passed to scheduler backends as the gang's minimum member count. For gang termination, completion-aware resources use the `Failed` condition as the termination signal instead of `MinAvailableBreached`, and fire immediately without `terminationDelay`. A pod failure or eviction in a completion-aware `PodClique` moves the pod to `pod phase=Failed`, causing the owning `PodClique` to set the `Failed` condition and triggering this gang-termination path. Crucially, the termination scope is identical to the existing gang-termination implementation: the same gang boundary that would have been torn down by a `MinAvailableBreached` trigger is torn down by a `Failed` trigger. Completion-aware behavior changes the runtime trigger, not the scheduling contract or termination scope. |
There was a problem hiding this comment.
Crucially, the termination scope is identical to the existing gang-termination implementation: the same gang boundary that would have been torn down by a
MinAvailableBreachedtrigger is torn down by aFailedtrigger
Well, unfortunately that is really true. Today the gang-termination is only done at the PCS level. While this needs to be corrected your claim of same gang boundary is not really true today.
unmarshall
left a comment
There was a problem hiding this comment.
5/n review comments. More to come.
|
|
||
| ### Completion Policy | ||
|
|
||
| Completion-aware behavior is rooted at the `PodClique` level. A `PodClique` becomes completion-aware by setting at least one concrete supported leaf under `spec.policy.completion`. Omitting `policy.completion` keeps the `PodClique` in regular mode. |
There was a problem hiding this comment.
A
PodCliquebecomes completion-aware by setting at least one concrete supported leaf underspec.policy.completion
Please define what a leaf means here. You have this term on more than one occasion but not really defined it clearly.
There was a problem hiding this comment.
It should be field, sorry, I'll correct that.
| ### Non-Goals | ||
|
|
||
| - **No pod-level retry.** A failed pod within a `PodClique` is not replaced in isolation. `PodClique` `policy.completion.failure.maxRestarts` may only be `0` in this release; values greater than `0` are future work. This is acceptable for the first release because current distributed training frameworks generally do not tolerate replacing a single failed worker independently. A single worker failure usually requires restarting the whole group, so gang restart covers the common recovery path. | ||
| - **No scaling for completion-aware workloads.** Completion-aware workloads use fixed replica counts. Grove rejects autoscaling configuration and manual replica changes for resources with `policy.completion` and for parent scopes that contain completion-aware direct children. |
There was a problem hiding this comment.
Can you also clarify that as a consequence of this Grove at least in this iteration will not support elastic training. e.g. PyTorch elastic (torchelastic), Elastic horovod, Ray train. Kubeflow PytorchJob exposes elasticPolicy
|
|
||
| Regular-mode `PodClique`s within a `PodCliqueScalingGroup` or `PodCliqueSet` are excluded from completion evaluation. A resource can be Completed even if some of its children remain running in regular mode. | ||
|
|
||
| **Gang termination for completion-aware resources.** Gang scheduling is unchanged: `minAvailable` continues to gate pod launch until the full gang can be placed simultaneously, on initial start and after each restart. `minAvailable` also continues to be passed to scheduler backends as the gang's minimum member count. For gang termination, completion-aware resources use the `Failed` condition as the termination signal instead of `MinAvailableBreached`, and fire immediately without `terminationDelay`. A pod failure or eviction in a completion-aware `PodClique` moves the pod to `pod phase=Failed`, causing the owning `PodClique` to set the `Failed` condition and triggering this gang-termination path. Crucially, the termination scope is identical to the existing gang-termination implementation: the same gang boundary that would have been torn down by a `MinAvailableBreached` trigger is torn down by a `Failed` trigger. Completion-aware behavior changes the runtime trigger, not the scheduling contract or termination scope. |
There was a problem hiding this comment.
Let's assume a PCSG has a completion-aware PCLQ and a regular-mode PCLQ.
If MinAvailableBreached condition is disabled for this PCS - then what happens when the regular-mode PCLQ goes below minAvailable? This PCLQ also will never go to Failed state since its restartPolicy is set to Always.
- If MinAvailableBreached is disabled scope-wide → a regular PCLQ can lose all its pods and the gang silently ignores it. If the completion-aware siblings depend on that helper (and they're co-scheduled in one gang, so they're coupled), the job keeps running against a dead dependency with no signal. This is quite unexpected.
- If you keep MinAvailableBreached alive only for regular mode PCLQs then now you have 2 termination triggers that coexist together in one scope. Now if due to MinAvailableBreached the PCSG replica is recreated will this count towards maxRestarts?
There was a problem hiding this comment.
Thanks, good points, my POV here:
- We have to support those mixed regular/completion-aware PCS/PCSGs. The main use-cases have things like parameter-server or router etc. that do not run to completion but are part of the training/RL composite.
- It feels wrong to disable
MinAvailableBreachedon regular PCLQs, exactly for the reasons you mentioned. - It's hard to decide whether to consume
MaxRestartsupon gang-termination or not.
I tend NOT to consume. Why?
a. MaxRestarts refers to failure-triggered restarts. That's what this budget is about. When provided by the user, the user doesn't expect it to count for restarts due to PCLQs that are not part of the job itself.
b. If we consume MaxRestarts for gang-terminations, why not do that also for regular PCS that has no completion-aware children at all? Then this field turns to be a generic one, not just for completion-aware, and this takes us to a different journey, as now we assume that everything under completion.failure is related to completion-aware resources only.
Done in d8d01c8.
|
|
||
| For a completion-aware `PodCliqueSet`, evaluation follows the same two-level pattern as PCSG: | ||
|
|
||
| 1. *Replica state*: a replica is **Completed** when all required completion-aware direct children are `Completed` — either all completion-aware `PodClique`s and `PodCliqueScalingGroup`s, or the named children in `policy.completion.success.completedNames` when it is set. A replica is permanently **Failed** when a required direct child fails and no restart budget remains. A failure of a completion-aware direct child not listed in `completedNames` follows the same gang-restart behavior as PCSG: it consumes budget while budget remains, and fails the replica when no budget remains. |
There was a problem hiding this comment.
A failure of a completion-aware direct child not listed in
completedNamesfollows the same gang-restart behavior as PCSG: it consumes budget while budget remains, and fails the replica when no budget remains.
If the PCLQs listed in the targetNames/completedNames are satisfied but a completion-aware PCLQ that is not part of targetNames/completedNames fails then why should it fail the replica if the restart budget has expired? So even if the success gate is met it still fails the replica. This conflicts with "leader driven user story" - So if the leader completes successfully but one or more workers fails the will this now fail the replica?
There was a problem hiding this comment.
Please see my discussion with @yankay in this PR and check out the updated wording of this section.
|
|
||
| **PCLQ or PCSG failure handled by completion-aware PCS:** | ||
|
|
||
| When a constituent completion-aware `PodClique` or `PodCliqueScalingGroup` within a PCS replica fails, the PCS treats it as a gang-level failure for the whole replica: |
There was a problem hiding this comment.
the usage of the term gang-level failure for the whole PCS replica is incorrect. There is no concept of a Gang at the PCS replica level.
There was a problem hiding this comment.
Following this comment as well as this one, I'm thinking to refrain from using "gang-termination" at all, but replace it with "failure handling". I feel that "gang-termination" is already a loaded term, plus it's not fully implemented or working as planned. WDYT?
| 2. If the budget is not exhausted: the PCS deletes all constituents of that replica (all `PodClique`s and `PodCliqueScalingGroup`s) and recreates them together from the template. | ||
| 3. If the budget is exhausted: the PCS marks that replica as failed and re-evaluates its own terminal conditions. | ||
|
|
||
| **Ordering guarantee.** In all cases, terminal conditions and updated `replicaRestartCounts` are persisted to status before any deletion begins. If the controller restarts mid-cleanup, it can resume from the persisted state without double-counting restarts or re-creating resources that were already deleted. Cleanup of active (non-terminal) pods when a resource reaches a terminal state is described in [Cleanup Behavior](#cleanup-behavior). |
There was a problem hiding this comment.
terminal conditions and updated
replicaRestartCountsare persisted to status before any deletion begins
Persisting the updated replicaRestartCount will prevent double counting but it does nothing to tell Grove whether the delete + recreated work is pending or it's already done. If the status update is done with the increased replicaRestartCount and Grove crashes. When it comes back up Grove cannot know whether there is a pending delete + recreate.
So the count alone is insufficient.
There was a problem hiding this comment.
I agree that replicaRestartCounts alone is not enough.
I suggest to encode the restart attempt on recreated child resources.
For example, after replicaRestartCounts[replicaIndex] is incremented to 1, Grove creates the resources for that replica with an attempt label, e.g. grove.io/restart-attempt: "1".
On reconcile, Grove compares the desired attempt from the parent status with the attempt labels on existing child resources:
- children still have attempt
0→ the restart is still pending, so Grove should delete/recreate them - children have attempt
1→ the restart already happened, so Grove should not consume budget again
This keeps the parent status relatively small and fits the usual reconciliation model: desired attempt is stored on the parent, realized attempt is visible on the children.
An alternative is to add an explicit restart operation state to the parent status, for example replicaRestarts[] with replicaIndex, attempt, and phase: Deleting|Recreating. That is more explicit and easier to debug, but it adds more status/API surface and another state machine to maintain.
|
|
||
| **Ordering guarantee.** In all cases, terminal conditions and updated `replicaRestartCounts` are persisted to status before any deletion begins. If the controller restarts mid-cleanup, it can resume from the persisted state without double-counting restarts or re-creating resources that were already deleted. Cleanup of active (non-terminal) pods when a resource reaches a terminal state is described in [Cleanup Behavior](#cleanup-behavior). | ||
|
|
||
| **Gang scheduling on restart.** Recreated pods are placed by the scheduler as a complete gang, consistent with the initial launch behavior. |
There was a problem hiding this comment.
The same issue - if PCLQ pods are recreated in isolation then its pods will not be gang scheduled.
There was a problem hiding this comment.
Can you clarify? Pods are not recreated in isolation - any single pod failure takes the whole PCLQ down (together with the rest of the PCS/PCSG replica).
| **New conditions.** Two terminal conditions are added across all three resource types: | ||
|
|
||
| - `Completed` — all required pods or completion-aware children have succeeded. | ||
| - `Failed` — enough pods or children have failed that the completion criterion is permanently unreachable. Once set, this condition is irreversible. |
There was a problem hiding this comment.
enough pods - vague - what is enough?
There was a problem hiding this comment.
Duplicate comment? The definition of "enough" is in the second half of the sentence. Resource can no longer reach completion.
|
|
||
| These conditions are represented in the standard `conditions` list on each resource's status. Regular-mode resources never set these conditions. Grove does not introduce `Pending` or `Running` states for this feature. | ||
|
|
||
| Terminal conditions are not propagated top-down to children that did not independently complete or fail. For example, if a `PodCliqueScalingGroup` replica completes because the `PodClique`s listed in `completedNames` completed successfully, any other child `PodClique`s may not have a `Completed` or `Failed` condition. Instead, the parent terminal state makes them no longer desired, and top-down cleanup removes their active pods and non-terminal child resources. |
There was a problem hiding this comment.
if a
PodCliqueScalingGroupreplica completes because thePodCliques listed incompletedNamescompleted successfully, any other childPodCliques may not have aCompletedorFailedcondition. Instead, the parent terminal state makes them no longer desired,
I have raised a similar issue before where you have mentioned that if a PCLQ that is not part of targetNames/completedNames fails then it will PCSG replica will be considered fail. But here this is reversed. So when you make the correction there, perhaps this will then be harmonised.
Clarified in f2afa6b. Thanks |
Adds GREP-0285 — the Grove Enhancement Proposal for adding job support to Grove: finite, completion-oriented workloads with bottom-up completion and failure semantics, gang restart, and new API fields (
completedNames,maxRestarts).Related to #285
🤖 Generated with Claude Code