feat(podgrouper): support externally created pod groups#1552
Queued
enoodle wants to merge 6 commits into
Queued
Conversation
Signed-off-by: Erez Freiberger <[email protected]>
Signed-off-by: Erez Freiberger <[email protected]>
Signed-off-by: Erez Freiberger <[email protected]>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds support for externally-created PodGroups, enabling pods from multiple independent workloads to be grouped and scheduled atomically together. Users can opt out of podgrouper reconciliation via the ChangesExternal PodGroup Support
Sequence DiagramsequenceDiagram
participant User
participant PodController as Pod Controller
participant K8s as Kubernetes
participant Scheduler as Scheduler
participant StatusUpdater as Status Updater
User->>K8s: Create external PodGroup (with subgroups)
User->>K8s: Create Workload (Job/RayJob) with pod-group-name annotation
K8s->>PodController: Reconcile pod
alt Pod has skip-podgrouper annotation
PodController->>PodController: Check shouldSkipPodGrouper
PodController-->>K8s: Skip reconciliation
else Owner has skip-podgrouper annotation
PodController->>K8s: Fetch pod owners
PodController->>PodController: Check shouldSkipAnyOwner
PodController-->>K8s: Skip reconciliation
else Normal flow
PodController-->>K8s: Allow normal processing
K8s->>Scheduler: Discover pod via pod-group-name annotation
Scheduler->>Scheduler: AddTaskInfo: lookup subgroup-name
alt Subgroup exists
Scheduler->>Scheduler: Add pod to subgroup
else Subgroup missing
Scheduler->>Scheduler: Track as InvalidSubGroupTask
Scheduler->>Scheduler: Attach fit error
end
Scheduler->>StatusUpdater: Report pod status
StatusUpdater->>StatusUpdater: recordInvalidSubGroupPodsEvents
StatusUpdater->>K8s: Set pod Unschedulable condition with error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Performance Benchmark ResultsComparing PR (
|
Merging this branch changes the coverage (1 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Signed-off-by: Erez Freiberger <[email protected]>
Signed-off-by: Erez Freiberger <[email protected]>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/scheduler/api/podgroup_info/job_info.go (1)
482-531:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Clone()silently dropsInvalidSubGroupTasks.
addInvalidSubGroupTask(line 602) only writes topgi.InvalidSubGroupTasks— invalid tasks are never added to anyPodSetor toPodStatusIndex. BecauseGetAllPodsMapiteratespgi.PodSetsonly,Clone()'s callCloneWithTasks(maps.Values(pgi.GetAllPodsMap()))will not pass invalid-subgroup tasks to the clone, andCloneWithTasksinitializes the newInvalidSubGroupTasksempty. The result is that any code path that consumes a clonedPodGroupInfo(scenario simulations, fork/restore in actions) will see those tasks as completely absent rather than as invalid, which can desync the status updater path that relies onGetInvalidSubGroupTasks/IsInvalidSubGroupTask.Either include them in the task list, or carry the map across explicitly:
🔧 Suggested fix
func (pgi *PodGroupInfo) CloneWithTasks(tasks []*pod_info.PodInfo) *PodGroupInfo { info := &PodGroupInfo{ ... PodStatusIndex: map[pod_status.PodStatus]pod_info.PodsMap{}, InvalidSubGroupTasks: pod_info.PodsMap{}, activeAllocatedCount: ptr.To(0), } pgi.CreationTimestamp.DeepCopyInto(&info.CreationTimestamp) info.RootSubGroupSet = pgi.RootSubGroupSet.Clone() info.PodSets = info.RootSubGroupSet.GetDescendantPodSets() for _, task := range tasks { info.AddTaskInfo(task.Clone()) } + + // Carry over invalid-subgroup tasks so callers of Clone()/CloneWithTasks + // see the same view as the source. AddTaskInfo will route them back to + // InvalidSubGroupTasks because the subgroup is still missing on the clone. + for _, task := range pgi.InvalidSubGroupTasks { + info.AddTaskInfo(task.Clone()) + } return info }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/scheduler/api/podgroup_info/job_info.go` around lines 482 - 531, Clone() currently omits pgi.InvalidSubGroupTasks because it builds the task list from GetAllPodsMap; fix by carrying InvalidSubGroupTasks into the clone instead of dropping it: in Clone() either include maps.Values(pgi.InvalidSubGroupTasks) when calling CloneWithTasks or, better, after creating info in CloneWithTasks copy/clone pgi.InvalidSubGroupTasks into info.InvalidSubGroupTasks (preserving the same pod IDs and pod_info.PodInfo pointers) so that methods like GetInvalidSubGroupTasks/IsInvalidSubGroupTask continue to work; update the Clone and/or CloneWithTasks implementations to perform this explicit map copy (referencing PodGroupInfo.InvalidSubGroupTasks, Clone(), CloneWithTasks(), and addInvalidSubGroupTask).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/batch/external-podgroup-job.yaml`:
- Around line 8-18: Remove the misleading top-level minMember from the PodGroup
example (the declared subGroups: - name: workers minMember: 2 means the
top-level minMember is ignored by setSubGroups) and instead document that
per-subGroup members drive sizing; also add a one-line comment in the YAML next
to the PodGroup metadata (or directly above the resource) stating that this
external PodGroup has no ownerReferences and must be deleted manually or by the
external controller after the Job completes (to warn about GC footgun).
In `@pkg/scheduler/cache/status_updater/default_status_updater.go`:
- Around line 384-404: In defaultStatusUpdater.recordInvalidSubGroupPodsEvents,
make the MarkUnschedulable behavior consistent with
recordUnschedulablePodsEvents by using the PodGroup's MarkUnschedulable flag
instead of hardcoding true when calling markTaskUnschedulable (use
job.PodGroup.MarkUnschedulable or equivalent), and add the missing debug branch
that logs the full detailed fit error (log.InfraLogger.V(6).Infof("Full fit
error: %s", fitError.DetailedError())) when detailedFitErrors is false/when
fitError exists, mirroring the logging in recordUnschedulablePodsEvents; keep
addNodePoolPrefixIfNeeded and existing error aggregation unchanged.
---
Outside diff comments:
In `@pkg/scheduler/api/podgroup_info/job_info.go`:
- Around line 482-531: Clone() currently omits pgi.InvalidSubGroupTasks because
it builds the task list from GetAllPodsMap; fix by carrying InvalidSubGroupTasks
into the clone instead of dropping it: in Clone() either include
maps.Values(pgi.InvalidSubGroupTasks) when calling CloneWithTasks or, better,
after creating info in CloneWithTasks copy/clone pgi.InvalidSubGroupTasks into
info.InvalidSubGroupTasks (preserving the same pod IDs and pod_info.PodInfo
pointers) so that methods like GetInvalidSubGroupTasks/IsInvalidSubGroupTask
continue to work; update the Clone and/or CloneWithTasks implementations to
perform this explicit map copy (referencing PodGroupInfo.InvalidSubGroupTasks,
Clone(), CloneWithTasks(), and addInvalidSubGroupTask).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43eebb4f-1b5a-48ea-87eb-84b4df202c64
📒 Files selected for processing (14)
CHANGELOG.mddocs/batch/README.mddocs/developer/pod-grouper.mdexamples/batch/README.mdexamples/batch/external-podgroup-job.yamlpkg/common/constants/constants.gopkg/podgrouper/pod_controller.gopkg/podgrouper/pod_controller_test.gopkg/scheduler/api/podgroup_info/job_info.gopkg/scheduler/api/podgroup_info/job_info_test.gopkg/scheduler/cache/cluster_info/cluster_info_test.gopkg/scheduler/cache/record_job_status_event_test.gopkg/scheduler/cache/status_updater/default_status_updater.gotest/e2e/suites/integrations/k8s_native/k8s_native_specs.go
Merging this branch changes the coverage (1 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
SiorMeir
reviewed
May 7, 2026
SiorMeir
approved these changes
May 7, 2026
Merging this branch changes the coverage (1 decrease, 2 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implement #1493 - support externally created pod groups
Related Issues
Fixes #1420
Checklist
Breaking Changes
None
Additional Notes
Adding
kai.scheduler/skip-podgrouperannotation to tell the pod grouper to skip a pod / resource to support externally created PodGroups by not overwriting the annotation.Summary by CodeRabbit
New Features
Documentation
Tests