Skip to content

feat(operator): add finite PodClique phase handling - #728

Closed
shayasoolin wants to merge 20 commits into
ai-dynamo:mainfrom
shayasoolin:job-support-pclq-phase
Closed

shayasoolin wants to merge 20 commits into
ai-dynamo:mainfrom
shayasoolin:job-support-pclq-phase

Conversation

@shayasoolin

Copy link
Copy Markdown
Contributor

Summary

Related to #285.

This PR introduces the first implementation slice for finite/job-style PodCliques. A PodClique is treated as finite when its pod template uses restartPolicy: Never.

The branch is currently stacked on the GREP branch from #696, so until #696 is merged this PR also includes the proposal/documentation commits in its diff.

The implementation scope is intentionally limited to the PCLQ level:

  • add PodClique.status.phase with Completed / Failed
  • mark a finite PCLQ Completed when all pods succeed
  • mark a finite PCLQ Failed when any pod fails
  • stop pod/resource recreation once the PCLQ is terminal
  • delete non-terminal pods after terminal phase is persisted
  • retain terminal pods for logs/debugging

API / Validation

  • Adds JobPhase and optional status.phase to PodClique.
  • Allows restartPolicy: Never for finite PCLQs.
  • Keeps default/empty restart policy behavior as Always.
  • Rejects restartPolicy: OnFailure.
  • Rejects autoScalingConfig for finite PCLQs.
  • Rejects replica changes for finite PCLQs after creation.

Controller Behavior

  • Adds finite-PCLQ helpers behind generic naming so the finite/job-mode decision can later move away from restartPolicy if needed.
  • Updates PCLQ status bottom-up from pod phases.
  • Requeues immediately when status reconciliation transitions a PCLQ to terminal, so the next reconcile performs terminal cleanup with persisted phase.
  • Updates the pod watch predicate to react directly to pod.status.phase changes.

Tests

  • Unit coverage for finite detection, validation, phase calculation, terminal sync skipping, active pod cleanup, and terminal-transition requeue.
  • E2E coverage for:
    • all pods succeed -> PCLQ Completed, terminal pods retained
    • one pod fails -> PCLQ Failed, terminal pods retained, active pods deleted and not recreated

Verification

GOFLAGS=-mod=mod GOCACHE=/private/tmp/grove-go-build-cache GOTOOLCHAIN=auto go test ./internal/controller/podclique
GOFLAGS=-mod=mod GOCACHE=/private/tmp/grove-go-build-cache GOTOOLCHAIN=auto go test -tags=e2e ./e2e/tests -c -o /private/tmp/grove-e2e-tests.test

shayasoolin and others added 16 commits July 22, 2026 11:26
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>
@copy-pr-bot

copy-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@shayasoolin shayasoolin changed the title feat(pclq): add finite job-mode phase handling feat(operator): add finite PodClique phase handling Jul 22, 2026
| `updatedReplicas` _integer_ | UpdatedReplicas is the number of Pods that have been updated and are at the desired revision of the PodClique. | 0 | |
| `scheduleGatedReplicas` _integer_ | ScheduleGatedReplicas is the number of Pods that have been created with one or more scheduling gate(s) set.<br />Sum of ReadyReplicas and ScheduleGatedReplicas will always be <= Replicas. | 0 | |
| `scheduledReplicas` _integer_ | ScheduledReplicas is the number of Pods that have been scheduled by the backend scheduler. | 0 | |
| `phase` _[JobPhase](#jobphase)_ | Phase is the terminal phase of a finite PodClique. It is empty for non-finite PodCliques and finite PodCliques that are still running. | | Enum: [Completed Failed] <br /> |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not supporting it for not finite PCLQ ? "pending"/ "running"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We defined that we'll set phases only for "job-mode" resources. If we support pending/running we should put them on any resource even in regular non-job mode, and I'm not sure there's value in doing that.
Unless you suggest to have pending/running but only for finite resources?
Can we discuss this as part of the GREP PR?

}

// IsPCLQTerminal reports whether a PodClique has reached a finite terminal phase.
func IsPCLQTerminal(pclq *grovecorev1alpha1.PodClique) bool {

@danbar2 danbar2 Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to IsPCLQTerminated

return ctrlcommon.ContinueReconcile()
}

func isTerminalPod(pod *corev1.Pod) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to isPodTerminated

var warnings []string

if !utils.IsEmptyStringType(spec.RestartPolicy) {
warnings = append(warnings, "restartPolicy will be ignored, it will be set to Always")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually breaking, cause we supported any value before that and now it will return an error "OnFailure" for example

@shayasoolin
shayasoolin marked this pull request as draft July 28, 2026 08:02
@shayasoolin shayasoolin closed this Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants