refactor: Change minMember for subgroups from int32 to pointer to int32#1340
Merged
refactor: Change minMember for subgroups from int32 to pointer to int32#1340
Conversation
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Merging this branch will increase overall coverage
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
|
📊 Performance Benchmark ResultsComparing PR (
|
SiorMeir
previously approved these changes
Mar 30, 2026
itsomri
reviewed
Mar 30, 2026
itsomri
reviewed
Mar 30, 2026
itsomri
reviewed
Mar 30, 2026
itsomri
reviewed
Mar 30, 2026
davidLif
force-pushed
the
DavidL/minSubgroups/minMemberToPointer
branch
from
3b35f1e to
3752775
Compare
davidLif
force-pushed
the
DavidL/minSubgroups/minMemberToPointer
branch
from
eceebd8 to
73399ca
Compare
Merging this branch changes the coverage (1 decrease, 3 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
|
itsomri
reviewed
Mar 31, 2026
davidLif
force-pushed
the
DavidL/minSubgroups/minMemberToPointer
branch
from
2f0c392 to
6ac5043
Compare
Merging this branch changes the coverage (2 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
|
itsomri
reviewed
Apr 3, 2026
![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: 1
🧹 Nitpick comments (1)
deployments/kai-scheduler/crds/scheduling.run.ai_podgroups.yaml (1)
98-99: Consider documenting the effective minimum enforcement.The CRD schema allows
subGroups[].minMemberto be0, but the factory code atpkg/scheduler/api/podgroup_info/subgroup_info/factory.go:85enforces a minimum of1viamax(*subGroup.MinMember, MinimumSubGroupMinAvailable).This is fine for backward compatibility, but consider adding a comment in the description (lines 94-96) to clarify that values below 1 are treated as 1, to avoid user confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/kai-scheduler/crds/scheduling.run.ai_podgroups.yaml` around lines 98 - 99, The CRD allows subGroups[].minMember to be 0 but the runtime enforces a minimum of 1 via Maximum/min logic in pkg/scheduler/api/podgroup_info/subgroup_info/factory.go (see the MinimumSubGroupMinAvailable constant and the max(*subGroup.MinMember, MinimumSubGroupMinAvailable) call), so update the description for subGroups[].minMember in the CRD (the current description around lines 94-96) to add a short note that values less than 1 will be treated as 1 at runtime to avoid confusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/apis/scheduling/v2alpha2/podgroup_webhook.go`:
- Around line 70-72: The webhook currently rejects any subgroup where
subGroup.MinMember == nil; change validation in the function that checks
subgroups (the code using subGroup.MinMember in podgroup_webhook.go) to only
require MinMember when the subgroup is a leaf (i.e., has no children/subgroups)
— check the subgroup's children collection (e.g., subGroup.SubGroups or
equivalent) and only return fmt.Errorf("subgroup %s: minMember is required",
subGroup.Name) when the children list is empty, matching the runtime factory
behavior that only leaf subgroups that create a PodSet need MinMember.
---
Nitpick comments:
In `@deployments/kai-scheduler/crds/scheduling.run.ai_podgroups.yaml`:
- Around line 98-99: The CRD allows subGroups[].minMember to be 0 but the
runtime enforces a minimum of 1 via Maximum/min logic in
pkg/scheduler/api/podgroup_info/subgroup_info/factory.go (see the
MinimumSubGroupMinAvailable constant and the max(*subGroup.MinMember,
MinimumSubGroupMinAvailable) call), so update the description for
subGroups[].minMember in the CRD (the current description around lines 94-96) to
add a short note that values less than 1 will be treated as 1 at runtime to
avoid confusion.
🪄 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: a8b64cb5-246d-4caf-b4c2-79213d38c5a0
📒 Files selected for processing (32)
deployments/kai-scheduler/crds/scheduling.run.ai_podgroups.yamlpkg/apis/scheduling/v2alpha2/podgroup_types.gopkg/apis/scheduling/v2alpha2/podgroup_webhook.gopkg/apis/scheduling/v2alpha2/podgroup_webhook_test.gopkg/apis/scheduling/v2alpha2/zz_generated.deepcopy.gopkg/binder/plugins/interface.gopkg/binder/plugins/mock/plugins_mock.gopkg/env-tests/utils/utils.gopkg/podgrouper/podgroup/handler.gopkg/podgrouper/podgroup/handler_test.gopkg/podgrouper/podgroup/updater_test.gopkg/podgrouper/podgrouper/plugins/ray/ray_grouper_test.gopkg/queuecontroller/controllers/suite_test.gopkg/scheduler/actions/common/solvers/pod_scenario_builder_test.gopkg/scheduler/api/podgroup_info/job_info.gopkg/scheduler/api/podgroup_info/subgroup_info/factory.gopkg/scheduler/api/podgroup_info/subgroup_info/factory_test.gopkg/scheduler/cache/cluster_info/cluster_info_test.gopkg/scheduler/cache/record_job_status_event_test.gopkg/scheduler/plugins/snapshot/snapshot_test.gotest/e2e/modules/resources/rd/pod_group/distributed_job.gotest/e2e/modules/resources/rd/pod_group/pod_group.gotest/e2e/scale/kwok_job_creation.gotest/e2e/suites/allocate/resources/dra_test.gotest/e2e/suites/allocate/subgroups/subgroups_test.gotest/e2e/suites/allocate/topology/topology_test.gotest/e2e/suites/api/events/events_specs.gotest/e2e/suites/integrations/third_party/jobset/jobset_test.gotest/e2e/suites/integrations/third_party/knative/knative_specs.gotest/e2e/suites/integrations/third_party/knative/knative_test.gotest/e2e/suites/integrations/third_party/leader_worker_set/leader_worker_set_test.gotest/e2e/suites/preempt/preempt_distributed_specs.go
itsomri
previously approved these changes
Apr 5, 2026
Signed-off-by: davidLif <davidshani12@gmail.com>
Signed-off-by: davidLif <davidshani12@gmail.com>
2- Make the podgroup minMember to pointer Signed-off-by: davidLif <davidshani12@gmail.com>
Signed-off-by: davidLif <davidshani12@gmail.com>
Signed-off-by: davidLif <davidshani12@gmail.com>
Signed-off-by: davidLif <davidshani12@gmail.com>
Signed-off-by: davidLif <davidshani12@gmail.com>
davidLif
force-pushed
the
DavidL/minSubgroups/minMemberToPointer
branch
from
f9c2816 to
5aa112a
Compare
itsomri
approved these changes
Apr 5, 2026
Merging this branch changes the coverage (1 decrease, 3 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
|
davidLif
added a commit
that referenced
this pull request
Apr 5, 2026
…32 (#1340) Signed-off-by: davidLif <davidshani12@gmail.com>
3 tasks
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
3 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
Change minMember for subgroups from int32 to pointer to int32
Related Issues
Fixes #
Checklist
Breaking Changes
Additional Notes
Summary by CodeRabbit
backoffLimit,completions, andparallelismfields from PodGroup specification.minMemberis now required for all subgroups; subgroups must explicitly specify this field.minMemberset to zero, enabling flexible group member requirements.