capacity plugin support DRA - #5058
Conversation
volcano-sh-bot
added
the
kind/feature
volcano-sh-bot
added
the
size/XXL
Summary of ChangesHello @xu-wentao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly extends the Volcano scheduler's capacity plugin by integrating comprehensive support for Kubernetes Dynamic Resource Allocation (DRA). The primary goal is to provide robust queue-level quota management for specialized hardware resources, enabling cluster administrators to enforce fair and controlled distribution of these resources in multi-tenant environments. The changes encompass defining new API types for DRA quotas, implementing the logic for aggregating and checking DRA resource requests, and ensuring compatibility with existing hierarchical queue semantics, alongside comprehensive testing and documentation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Dynamic Resource Allocation (DRA) quota support into the Volcano scheduler's capacity plugin. The changes are extensive, touching API definitions, documentation, core scheduler cache logic, the capacity plugin, and tests. The implementation follows the design document well and adds a significant new capability for managing specialized resources like GPUs.
My review has identified one high-severity correctness issue related to a side effect in a resource calculation function, which could lead to incorrect scheduling behavior. I've also found a minor typo in the design document. Overall, this is a well-structured and comprehensive feature addition.
| func (ji *JobInfo) GetMinDRAResources() map[string]*DRAResource { | ||
| if len(ji.Tasks) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| result := make(map[string]*DRAResource) | ||
|
|
||
| // Since DRA requests can vary per task/pod, we aggregate them based on TaskMinAvailable | ||
| for _, task := range ji.Tasks { | ||
| if task.DRAResreq == nil { | ||
| continue | ||
| } | ||
|
|
||
| // Calculate how many times this task type needs to run | ||
| taskType := task.TaskRole | ||
| minNum, ok := ji.TaskMinAvailable[taskType] | ||
| if !ok || minNum <= 0 { | ||
| // If TaskMinAvailable is not set, default to 1 for this task if it is part of the job's minAvailable | ||
| // However, for precise minimum calculation, we only count the first occurrence for each TaskRole | ||
| // and multiply it by minNum | ||
| continue | ||
| } | ||
|
|
||
| // Only process one sample task per TaskRole to represent that type | ||
| // Set minNum to 0 so we don't process it again | ||
| ji.TaskMinAvailable[taskType] = 0 | ||
|
|
||
| for deviceClass, res := range task.DRAResreq { | ||
| if _, exists := result[deviceClass]; !exists { | ||
| result[deviceClass] = &DRAResource{ | ||
| Count: 0, | ||
| Capacity: make(map[string]resource.Quantity), | ||
| } | ||
| } | ||
|
|
||
| result[deviceClass].Count += res.Count * int64(minNum) | ||
| for dim, cap := range res.Capacity { | ||
| totalCap := cap.DeepCopy() | ||
| // resource.Quantity has no Multiply func, so we parse memory/cpu as MilliValues | ||
| // For exact values we can just use set | ||
| // Since Quantity can represent fractional, we will loop to add | ||
| for i := int32(0); i < minNum-1; i++ { | ||
| totalCap.Add(cap) | ||
| } | ||
|
|
||
| if existing, exists := result[deviceClass].Capacity[dim]; exists { | ||
| existing.Add(totalCap) | ||
| result[deviceClass].Capacity[dim] = existing | ||
| } else { | ||
| result[deviceClass].Capacity[dim] = totalCap | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(result) == 0 { | ||
| return nil | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
The function GetMinDRAResources modifies ji.TaskMinAvailable by setting values to 0. This is a side effect that makes the function non-idempotent. If this function is called multiple times on the same JobInfo object within a scheduling cycle, subsequent calls will produce incorrect results (likely nil), which could lead to incorrect scheduling decisions. Get... functions should generally be read-only and not modify their receiver's state.
To fix this, please use a local map to track which task roles have been processed, instead of modifying ji.TaskMinAvailable.
func (ji *JobInfo) GetMinDRAResources() map[string]*DRAResource {
if len(ji.Tasks) == 0 {
return nil
}
result := make(map[string]*DRAResource)
processedRoles := make(map[string]struct{})
// Since DRA requests can vary per task/pod, we aggregate them based on TaskMinAvailable
for _, task := range ji.Tasks {
if task.DRAResreq == nil {
continue
}
taskType := task.TaskRole
if _, ok := processedRoles[taskType]; ok {
continue
}
// Calculate how many times this task type needs to run
minNum, ok := ji.TaskMinAvailable[taskType]
if !ok || minNum <= 0 {
// If TaskMinAvailable is not set, default to 1 for this task if it is part of the job's minAvailable
// However, for precise minimum calculation, we only count the first occurrence for each TaskRole
// and multiply it by minNum
continue
}
// Only process one sample task per TaskRole to represent that type
processedRoles[taskType] = struct{}{}
for deviceClass, res := range task.DRAResreq {
if _, exists := result[deviceClass]; !exists {
result[deviceClass] = &DRAResource{
Count: 0,
Capacity: make(map[string]resource.Quantity),
}
}
result[deviceClass].Count += res.Count * int64(minNum)
for dim, cap := range res.Capacity {
totalCap := cap.DeepCopy()
// resource.Quantity has no Multiply func, so we parse memory/cpu as MilliValues
// For exact values we can just use set
// Since Quantity can represent fractional, we will loop to add
for i := int32(0); i < minNum-1; i++ {
totalCap.Add(cap)
}
if existing, exists := result[deviceClass].Capacity[dim]; exists {
existing.Add(totalCap)
result[deviceClass].Capacity[dim] = existing
} else {
result[deviceClass].Capacity[dim] = totalCap
}
}
}
}
if len(result) == 0 {
return nil
}
return result
}| | Field | Semantics | Enforcement | | ||
| |-------|-----------|-------------| | ||
| | `dra.capability` | Hard limit, cannot exceed | Allocation rejected if quota would be exceeded | | ||
| | `dra.deserved` | Future extensibility | Currenly NOT supported for elasticity/preemption in capacity plugin | |
There was a problem hiding this comment.
There is a typo here. "Currenly" should be "Currently".
| | `dra.deserved` | Future extensibility | Currenly NOT supported for elasticity/preemption in capacity plugin | | |
| | `dra.deserved` | Future extensibility | Currently NOT supported for elasticity/preemption in capacity plugin | |
There was a problem hiding this comment.
Pull request overview
Adds Dynamic Resource Allocation (DRA) quota modeling to Queue APIs and wires DRA request accounting/enforcement into the scheduler capacity plugin, with accompanying unit/e2e tests and documentation.
Changes:
- Extend QueueSpec (internal + v1beta1) with
spec.draquotas (count + optional consumable capacity) and add conversions. - Track per-task DRA requests (via ResourceClaims) in scheduler cache and enforce queue DRA capability in the capacity plugin (allocate + enqueue paths, incl. hierarchical propagation).
- Add DRA-focused tests and user/design docs.
Reviewed changes
Copilot reviewed 73 out of 73 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/scheduling_client.go | New generated scheduling typed client (appears misplaced; see comments). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/queue.go | New generated Queue typed client (appears misplaced; see comments). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/podgroup.go | New generated PodGroup typed client (appears misplaced; see comments). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/generated_expansion.go | New generated expansion interfaces (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/fake/fake_scheduling_client.go | New generated fake scheduling client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/fake/fake_queue.go | New generated fake Queue client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/fake/fake_podgroup.go | New generated fake PodGroup client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/fake/doc.go | New generated fake package doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/scheduling/v1beta1/doc.go | New generated package doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/bus/v1alpha1/generated_expansion.go | New generated expansion interfaces (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/bus/v1alpha1/fake/fake_command.go | New generated fake Command client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/bus/v1alpha1/fake/fake_bus_client.go | New generated fake bus client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/bus/v1alpha1/fake/doc.go | New generated fake package doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/bus/v1alpha1/doc.go | New generated package doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/bus/v1alpha1/command.go | New generated Command typed client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/bus/v1alpha1/bus_client.go | New generated bus typed client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/job.go | New generated Job typed client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/generated_expansion.go | New generated expansion interfaces (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/fake/fake_job.go | New generated fake Job client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/fake/fake_cronjob.go | New generated fake CronJob client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/fake/fake_batch_client.go | New generated fake batch client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/fake/doc.go | New generated fake package doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/doc.go | New generated package doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/cronjob.go | New generated CronJob typed client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/typed/batch/v1alpha1/batch_client.go | New generated batch typed client (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/scheme/register.go | New generated scheme registration (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/scheme/doc.go | New generated scheme package doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/fake/register.go | New generated fake scheme registration (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/fake/doc.go | New generated fake clientset doc (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/fake/clientset_generated.go | New generated fake clientset (appears misplaced). |
| volcano.sh/apis/pkg/client/versioned/clientset.go | New generated clientset (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/scheduling/v1beta1/queue.go | New generated lister (appears misplaced). |
| volcano.sh/apis/pkg/client/scheduling/v1beta1/podgroup.go | New generated lister (appears misplaced). |
| volcano.sh/apis/pkg/client/scheduling/v1beta1/expansion_generated.go | New generated lister expansions (appears misplaced). |
| volcano.sh/apis/pkg/client/externalversions/scheduling/v1beta1/queue.go | New generated informer (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/scheduling/v1beta1/podgroup.go | New generated informer (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/scheduling/v1beta1/interface.go | New generated informer interface (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/scheduling/interface.go | New generated informer group interface (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/internalinterfaces/factory_interfaces.go | New generated informer internal interfaces (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/generic.go | New generated generic informer (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/factory.go | New generated shared informer factory (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/bus/v1alpha1/interface.go | New generated bus informer interface (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/bus/v1alpha1/command.go | New generated Command informer (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/bus/interface.go | New generated bus group informer interface (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/batch/v1alpha1/job.go | New generated Job informer (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/batch/v1alpha1/interface.go | New generated batch informer interface (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/batch/v1alpha1/cronjob.go | New generated CronJob informer (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/externalversions/batch/interface.go | New generated batch group informer interface (appears misplaced; invalid imports). |
| volcano.sh/apis/pkg/client/bus/v1alpha1/expansion_generated.go | New generated bus lister expansions (appears misplaced). |
| volcano.sh/apis/pkg/client/bus/v1alpha1/command.go | New generated bus lister (appears misplaced). |
| volcano.sh/apis/pkg/client/batch/v1alpha1/job.go | New generated batch lister (appears misplaced). |
| volcano.sh/apis/pkg/client/batch/v1alpha1/expansion_generated.go | New generated batch lister expansions (appears misplaced). |
| volcano.sh/apis/pkg/client/batch/v1alpha1/cronjob.go | New generated batch lister (appears misplaced). |
| test/e2e/util/util.go | Extend e2e test context/options with per-queue DRA quota config. |
| test/e2e/util/queue.go | Allow creating queues with spec.dra in e2e utilities (signature change). |
| test/e2e/util/job.go | Allow setting pod.spec.resourceClaims via e2e job/task specs. |
| test/e2e/util/dra.go | Add helper to create ResourceClaims in e2e tests. |
| test/e2e/stress/queue.go | Update stress test to new CreateQueue signature (adds nil DRA arg). |
| test/e2e/dra/dra_quota_test.go | New e2e coverage for DRA quota enforcement scenarios. |
| staging/src/volcano.sh/apis/pkg/apis/scheduling/v1beta1/zz_generated.conversion.go | Add conversions for QueueSpec.DRA + DRAQuota/DRAResourceQuota. |
| staging/src/volcano.sh/apis/pkg/apis/scheduling/v1beta1/types.go | Add v1beta1 API types for spec.dra quotas. |
| staging/src/volcano.sh/apis/pkg/apis/scheduling/types.go | Add internal API types for spec.dra quotas. |
| pkg/scheduler/util/test_utils.go | Add QueueWrapper helper to set QueueSpec.DRA in unit tests. |
| pkg/scheduler/uthelper/helper.go | Wait for scheduler cache sync in unit-test harness. |
| pkg/scheduler/plugins/capacity/capacity_dra_test.go | New unit tests for capacity plugin DRA quota behavior. |
| pkg/scheduler/plugins/capacity/capacity.go | Implement DRA quota tracking + enforcement (allocate + enqueue paths, hierarchical propagation). |
| pkg/scheduler/cache/event_handlers.go | Populate TaskInfo.DRAResreq when building TaskInfo from Pod. |
| pkg/scheduler/cache/cache_mock.go | Ensure mock cache initializes ResourceClaim assume-cache for tests. |
| pkg/scheduler/cache/cache_dra_test.go | New unit test for building task DRA requests from ResourceClaims. |
| pkg/scheduler/cache/cache.go | Add ResourceClaim cache + buildTaskDRAResreq aggregation logic. |
| pkg/scheduler/api/job_info.go | Add DRAResource model, task cloning support, and job min-DRA computation. |
| docs/user-guide/how_to_use_dra_quota.md | New user guide for configuring/using queue DRA quotas. |
| docs/design/capacity-dra-support.md | New design doc describing DRA quota support in capacity plugin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // A simpler way often used in k8s tests: | ||
| featureGate := utilfeature.DefaultFeatureGate.(k8sfeature.MutableFeatureGate) | ||
| if err := featureGate.SetFromMap(map[string]bool{string(kubefeatures.DynamicResourceAllocation): true}); err != nil { | ||
| t.Logf("Failed to enable DynamicResourceAllocation feature gate: %v", err) |
There was a problem hiding this comment.
This test does an unchecked type assertion utilfeature.DefaultFeatureGate.(k8sfeature.MutableFeatureGate), which will panic if the default gate isn’t mutable in a given test environment. Use featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, kubefeatures.DynamicResourceAllocation, true) (or at least guard the assertion) to make the test robust.
| | Field | Semantics | Enforcement | | ||
| |-------|-----------|-------------| | ||
| | `dra.capability` | Hard limit, cannot exceed | Allocation rejected if quota would be exceeded | | ||
| | `dra.deserved` | Future extensibility | Currenly NOT supported for elasticity/preemption in capacity plugin | |
There was a problem hiding this comment.
Spelling: Currenly → Currently.
| | `dra.deserved` | Future extensibility | Currenly NOT supported for elasticity/preemption in capacity plugin | | |
| | `dra.deserved` | Future extensibility | Currently NOT supported for elasticity/preemption in capacity plugin | |
| import ( | ||
| batchv1alpha1 "clientset/versioned/typed/batch/v1alpha1" | ||
| busv1alpha1 "clientset/versioned/typed/bus/v1alpha1" | ||
| schedulingv1beta1 "clientset/versioned/typed/scheduling/v1beta1" | ||
| fmt "fmt" | ||
| http "net/http" |
There was a problem hiding this comment.
The generated clientset code under volcano.sh/apis/pkg/client/... uses import paths like clientset/versioned/typed/... and clientset/versioned/scheme, but there is no clientset/... package in this repo/module. As-is, these files won’t compile under the main volcano.sh/volcano module. These generated clients should either be removed (if the canonical client lives under staging/src/volcano.sh/apis/pkg/client/...) or regenerated with correct module import paths (e.g. volcano.sh/apis/pkg/client/clientset/versioned/...).
| import ( | ||
| batch "informers/externalversions/batch" | ||
| bus "informers/externalversions/bus" | ||
| internalinterfaces "informers/externalversions/internalinterfaces" | ||
| scheduling "informers/externalversions/scheduling" |
There was a problem hiding this comment.
This informer factory code imports informers/externalversions/..., but that package path does not exist in this repo/module (the existing generated informers are under volcano.sh/apis/pkg/client/informers/externalversions/... in staging). This will fail to compile; please remove these duplicate generated informers or regenerate them with the correct import paths/output directory.
| if !utilfeature.DefaultFeatureGate.Enabled(kubefeatures.DynamicResourceAllocation) || len(pod.Spec.ResourceClaims) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| result := make(map[string]*schedulingapi.DRAResource) | ||
| consumableCapacityEnabled := utilfeature.DefaultFeatureGate.Enabled(kubefeatures.DRAConsumableCapacity) | ||
|
|
||
| for _, podClaim := range pod.Spec.ResourceClaims { | ||
| if podClaim.ResourceClaimName == nil { | ||
| continue | ||
| } | ||
| claimName := *podClaim.ResourceClaimName | ||
| obj, err := sc.resourceClaimCache.Get(pod.Namespace + "/" + claimName) | ||
| if err != nil || obj == nil { | ||
| klog.V(4).Infof("Failed to get ResourceClaim %s/%s: %v", pod.Namespace, claimName, err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
buildTaskDRAResreq silently ignores missing/unsynced ResourceClaims (it logs and continues). That can permanently leave TaskInfo.DRAResreq empty for pods that do request DRA devices, which bypasses queue DRA quota enforcement. Consider failing task creation when any referenced claim can’t be retrieved (propagate an error via NewTaskInfo), or marking the task as unschedulable until the claim is present, rather than treating it as ‘no DRA request’. Also add a nil-guard for sc.resourceClaimCache to avoid panics when DRA is enabled but the cache isn’t initialized in some test/mocked cache setups.
| // Calculate how many times this task type needs to run | ||
| taskType := task.TaskRole | ||
| minNum, ok := ji.TaskMinAvailable[taskType] | ||
| if !ok || minNum <= 0 { | ||
| // If TaskMinAvailable is not set, default to 1 for this task if it is part of the job's minAvailable | ||
| // However, for precise minimum calculation, we only count the first occurrence for each TaskRole | ||
| // and multiply it by minNum | ||
| continue | ||
| } | ||
|
|
||
| // Only process one sample task per TaskRole to represent that type | ||
| // Set minNum to 0 so we don't process it again | ||
| ji.TaskMinAvailable[taskType] = 0 | ||
|
|
There was a problem hiding this comment.
GetMinDRAResources mutates ji.TaskMinAvailable (ji.TaskMinAvailable[taskType] = 0) while computing a derived value. This introduces side effects that can break later scheduling logic that relies on TaskMinAvailable remaining intact. Use a local seenTaskRoles set (or copy of TaskMinAvailable) to ensure each TaskRole is only processed once, without modifying the JobInfo state.
| minNum, ok := ji.TaskMinAvailable[taskType] | ||
| if !ok || minNum <= 0 { | ||
| // If TaskMinAvailable is not set, default to 1 for this task if it is part of the job's minAvailable | ||
| // However, for precise minimum calculation, we only count the first occurrence for each TaskRole | ||
| // and multiply it by minNum | ||
| continue | ||
| } |
There was a problem hiding this comment.
The comment says to default to 1 when TaskMinAvailable isn’t set, but the current behavior is to continue, which can undercount the job’s minimum DRA needs and allow oversize jobs to be enqueued. If the intent is ‘default 1’, implement that fallback (and still ensure you only process one representative task per TaskRole).
| func Test_capacityPlugin_DRA(t *testing.T) { | ||
| // Enable DRA feature gate | ||
| utilfeature.DefaultMutableFeatureGate.Set(fmt.Sprintf("%s=true", kubefeatures.DynamicResourceAllocation)) | ||
|
|
There was a problem hiding this comment.
This test enables the global DynamicResourceAllocation feature gate via utilfeature.DefaultMutableFeatureGate.Set(...) but doesn’t restore the original value, which can leak into other tests and cause order-dependent failures. Prefer featuregatetesting.SetFeatureGateDuringTest (as used elsewhere in the repo) or explicitly defer resetting the gate to its prior state.
97f85b7 to
f5c3a92
Compare
|
Would you be willing to share on tomorrow's Asia community meeting? The meeting time is 15:00 UTC+8 |
|
@xu-wentao Any update on the PR? :) |
457c170 to
49a4244
Compare
Yes, I have updated the implemention of the DRA metioned here, we can discuss thie design again. |
|
Please resolve the CI failure and sign off your commits, and there are also conflict codes that you need to resolve @xu-wentao |
There was a problem hiding this comment.
Why do add a volcano.sh ignore item?
There was a problem hiding this comment.
The folder volcano.sh is not in this project, but if we run make generate-code will create this folder automatilly, If we no need to upload it, maybe ignore it is a way i think?
a336583 to
4f7d588
Compare
| dynamicResourceAllocationEnable := utilfeature.DefaultFeatureGate.Enabled(kubefeatures.DynamicResourceAllocation) | ||
| draConsumableCapacityEnable := utilfeature.DefaultFeatureGate.Enabled(kubefeatures.DRAConsumableCapacity) | ||
|
|
||
| arguments.GetBool(&dynamicResourceAllocationEnable, DynamicResourceAllocationEnable) |
There was a problem hiding this comment.
Should sync to the user guide to allow users know there are such arguments for capacity plugins, default is controlled by feature gate values
| spec: | ||
| reclaimable: true | ||
| capability: | ||
| "cores.deviceclass/hami-core-gpu.project-hami.io": "800" |
There was a problem hiding this comment.
It would be best to further clarify the scenario for consumable capacity here. If consumable capacity is specified, then [dims].deviceclass/xxx should be configured as count * capacity, right? This could be explained to guide users.
There was a problem hiding this comment.
I think you should also update the inqueued attr when the pg can be enqueued (especially for simultaneously submit jobs it's useful): https://github.com/volcano-sh/volcano/blob/da719bb3b636ecce560798d9ec0f19c74e3b412c/pkg/scheduler/plugins/capacity/capacity. go#L271
|
|
||
| DRA quota is expressed directly in the queue `ResourceList` using reserved key formats. | ||
|
|
||
| ### Key Formats |
There was a problem hiding this comment.
In the scenario of consumable capacity, I may have a worry. For example, if a card has virtualization, then one ResourceSlice corresponds to multiple slices inside, right? So currently your design is flattened at the deviceclass level, regardless of how many slices a ResourceSlice has, we configure a total amount. For instance, if there are 2 cards (in the same ResourceSlice), each card has 8Gi of GPU memory, and the total I configure on the queue is 16Gi. If I configure 16Gi at the deviceclass level on the queue, and a podgroup requests 2 pods with 10Gi + 4Gi respectively, then one pod actually cannot be scheduled. However, it gets through the queue validation and occupies inqueue resources. Is this acceptable?
I think the current design is fine, but I believe there might be a potential issue, similar to how node had fragmented resources before. However, the queue check still passed, but I don't know if users would report such a problem. I think it could be stated as a constraint in the design doc or user guide doc
| // For exact values we can just use set | ||
| // Since Quantity can represent fractional, we will loop to add | ||
| for i := int32(0); i < minNum-1; i++ { | ||
| totalCap.Add(cap) |
There was a problem hiding this comment.
For Consumable Capacity, it should also be multiplied by count to get the total capacity resource, right? For example, a single pod requires 2 * 8Gi of GPU memory, and there are 2 pods in total, so the total requirement should be 32Gi, not 8Gi * 2 pods = 16Gi?
| @@ -0,0 +1,245 @@ | |||
| /* | |||
| Copyright 2024 The Volcano Authors. | |||
There was a problem hiding this comment.
All should change to -> 2026 The Volcano Authors
There was a problem hiding this comment.
Can we construct a Driver that can produce ResourceSlices containing multiple devices, each device with a capacity? I'd like to see such use case if it's possible
|
Please also resolve the conflicts and sign off the commits
|
6c0a9f5 to
e8506cb
Compare
|
There is a conflict @xu-wentao Please rebase to the latest codes and push again, thanks |
|
/ok-to-test |
volcano-sh-bot
added
ok-to-test
| func (cp *capacityPlugin) queueAllocatableWithReserved(attr *queueAttr, candidate *api.TaskInfo, queue *api.QueueInfo, draEnabled bool, consumableCapacityEnabled bool) bool { | ||
| if draEnabled && attr.dra != nil && candidate.DRAResreq != nil { | ||
| if !checkDRAAllocatable(attr.dra, candidate.DRAResreq, consumableCapacityEnabled) { | ||
| candidateDRA := incrementalTaskDRA(attr, candidate) |
There was a problem hiding this comment.
Put this line inside the draEnabled judgement is better
|
#5058 (comment) I think you should still open the feature gate for the volcano scheduler: Only open the feature gate in the kind config is opened for the kube-scheduler, not for the volcano scheduler @xu-wentao |
Signed-off-by: xuwentao <cutenear1993@yahoo.com>
ee18205 to
70e7984
Compare
70e7984 to
e08866b
Compare
Signed-off-by: xuwentao <cutenear1993@yahoo.com>
e08866b to
5ba79f3
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JesseStutler The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
the
approved
github-project-automation
Bot
moved this from In review
to Done
in Volcano Release Tracking
A Pod ADD event can reach the scheduler before its referenced PVC ADD
reaches the PVC informer — for example when a Pod is created together
with an OnDemand PVC in quick succession (Spark's k8s scheduler-backend
does exactly this for executor pods with `claimName=OnDemand`).
Since v1.15.0, the resulting `looking up PVC ... not found` error from
NewTaskInfo permanently strands the pod:
addPod → NewTaskInfo → addPodCSIVolumesToTask
→ pvcInformer.Lister().Get(pvcName) returns not-found
→ klog.Errorf(...)
→ sc.resyncTask(pi) // enqueues taskKey into errTasks
→ return err // task never enters sc.Jobs
processResyncTask → errTasks.Get() → parseErrTaskKey → getTaskByUID
→ "failed to find task <uid>"
→ errTasks.Forget(taskKey) // permanent
→ return // never reaches retryResyncTask
There is no PVC-informer AddFunc that re-queues stuck pods, and the
pod's resourceVersion never advances before scheduling, so
`UpdatePod → deletePod + addPod` never fires. Executor pods stay
Pending; the PVC stays Pending with `WaitForPodScheduled`.
Before volcano-sh#5058, `addPod`'s error branch fell through to `sc.addTask(pi)`
even after `NewTaskInfo` returned an error, so the incomplete task
lived in `sc.Jobs` and processResyncTask could find it by UID on the
next tick and call syncTask, which refetches the pod from the apiserver
— by which point the PVC informer has synced. volcano-sh#5058 added a DRA-specific
happy path using the correct `addTask + resyncTask + return nil`
pattern, but also added a generic `return err` that removed the
implicit recovery for PVC-not-found.
Extend the DRA branch to PVC-not-found errors, using the same pattern.
Change getPodCSIVolumes to wrap the lister error with %w so callers
can classify it with errors.Is / apierrors.IsNotFound instead of
matching on the message string.
Adds `TestAddPodWithUnresolvedPVCCachesTaskForResync` mirroring the
existing DRA counterpart.
Version scope: only v1.15.0 is affected. Verified with
`git merge-base --is-ancestor 5ba79f3 <tag>` — every v1.14.x tag
through v1.14.3 (released after v1.15.0) is clean; v1.15.0-alpha.0 is
also clean.
Signed-off-by: Gustavo Parcianello <gustavo.parcianello@sap.com>
A Pod ADD event can reach the scheduler before its referenced PVC ADD
reaches the PVC informer — for example when a Pod is created together
with an OnDemand PVC in quick succession (Spark's k8s scheduler-backend
does exactly this for executor pods with `claimName=OnDemand`).
Since v1.15.0, the resulting `looking up PVC ... not found` error from
NewTaskInfo permanently strands the pod:
addPod → NewTaskInfo → addPodCSIVolumesToTask
→ pvcInformer.Lister().Get(pvcName) returns not-found
→ klog.Errorf(...)
→ sc.resyncTask(pi) // enqueues taskKey into errTasks
→ return err // task never enters sc.Jobs
processResyncTask → errTasks.Get() → parseErrTaskKey → getTaskByUID
→ "failed to find task <uid>"
→ errTasks.Forget(taskKey) // permanent
→ return // never reaches retryResyncTask
There is no PVC-informer AddFunc that re-queues stuck pods, and the
pod's resourceVersion never advances before scheduling, so
`UpdatePod → deletePod + addPod` never fires. Executor pods stay
Pending; the PVC stays Pending with `WaitForPodScheduled`.
Before volcano-sh#5058, `addPod`'s error branch fell through to `sc.addTask(pi)`
even after `NewTaskInfo` returned an error, so the incomplete task
lived in `sc.Jobs` and processResyncTask could find it by UID on the
next tick and call syncTask, which refetches the pod from the apiserver
— by which point the PVC informer has synced. volcano-sh#5058 added a DRA-specific
happy path using the correct `addTask + resyncTask + return nil`
pattern, but also added a generic `return err` that removed the
implicit recovery for PVC-not-found.
Extend the DRA branch to PVC-not-found errors, using the same pattern.
Change getPodCSIVolumes to wrap the lister error with %w so callers
can classify it with errors.Is / apierrors.IsNotFound instead of
matching on the message string.
Adds `TestAddPodWithUnresolvedPVCCachesTaskForResync` mirroring the
existing DRA counterpart.
Version scope: only v1.15.0 is affected. Verified with
`git merge-base --is-ancestor 5ba79f3 <tag>` — every v1.14.x tag
through v1.14.3 (released after v1.15.0) is clean; v1.15.0-alpha.0 is
also clean.
Signed-off-by: Gustavo Parcianello <gustavo.parcianello@sap.com>
A Pod ADD event can reach the scheduler before its referenced PVC ADD
reaches the PVC informer — for example when a Pod is created together
with an OnDemand PVC in quick succession (Spark's k8s scheduler-backend
does exactly this for executor pods with `claimName=OnDemand`).
Since v1.15.0, the resulting `looking up PVC ... not found` error from
NewTaskInfo permanently strands the pod:
addPod → NewTaskInfo → addPodCSIVolumesToTask
→ pvcInformer.Lister().Get(pvcName) returns not-found
→ klog.Errorf(...)
→ sc.resyncTask(pi) // enqueues taskKey into errTasks
→ return err // task never enters sc.Jobs
processResyncTask → errTasks.Get() → parseErrTaskKey → getTaskByUID
→ "failed to find task <uid>"
→ errTasks.Forget(taskKey) // permanent
→ return // never reaches retryResyncTask
There is no PVC-informer AddFunc that re-queues stuck pods, and the
pod's resourceVersion never advances before scheduling, so
`UpdatePod → deletePod + addPod` never fires. Executor pods stay
Pending; the PVC stays Pending with `WaitForPodScheduled`.
Before volcano-sh#5058, `addPod`'s error branch fell through to `sc.addTask(pi)`
even after `NewTaskInfo` returned an error, so the incomplete task
lived in `sc.Jobs` and processResyncTask could find it by UID on the
next tick and call syncTask, which refetches the pod from the apiserver
— by which point the PVC informer has synced. volcano-sh#5058 added a DRA-specific
happy path using the correct `addTask + resyncTask + return nil`
pattern, but also added a generic `return err` that removed the
implicit recovery for PVC-not-found.
Extend the DRA branch to PVC-not-found errors, using the same pattern.
Change getPodCSIVolumes to wrap the lister error with %w so callers
can classify it with errors.Is / apierrors.IsNotFound instead of
matching on the message string.
Adds `TestAddPodWithUnresolvedPVCCachesTaskForResync` mirroring the
existing DRA counterpart.
Version scope: only v1.15.0 is affected. Verified with
`git merge-base --is-ancestor 5ba79f3 <tag>` — every v1.14.x tag
through v1.14.3 (released after v1.15.0) is clean; v1.15.0-alpha.0 is
also clean.
Signed-off-by: Gustavo Parcianello <gustavo.parcianello@sap.com>
A Pod ADD event can reach the scheduler before its referenced PVC ADD
reaches the PVC informer — for example when a Pod is created together
with an OnDemand PVC in quick succession (Spark's k8s scheduler-backend
does exactly this for executor pods with `claimName=OnDemand`).
Since v1.15.0, the resulting `looking up PVC ... not found` error from
NewTaskInfo permanently strands the pod:
addPod → NewTaskInfo → addPodCSIVolumesToTask
→ pvcInformer.Lister().Get(pvcName) returns not-found
→ klog.Errorf(...)
→ sc.resyncTask(pi) // enqueues taskKey into errTasks
→ return err // task never enters sc.Jobs
processResyncTask → errTasks.Get() → parseErrTaskKey → getTaskByUID
→ "failed to find task <uid>"
→ errTasks.Forget(taskKey) // permanent
→ return // never reaches retryResyncTask
There is no PVC-informer AddFunc that re-queues stuck pods, and the
pod's resourceVersion never advances before scheduling, so
`UpdatePod → deletePod + addPod` never fires. Executor pods stay
Pending; the PVC stays Pending with `WaitForPodScheduled`.
Before volcano-sh#5058, `addPod`'s error branch fell through to `sc.addTask(pi)`
even after `NewTaskInfo` returned an error, so the incomplete task
lived in `sc.Jobs` and processResyncTask could find it by UID on the
next tick and call syncTask, which refetches the pod from the apiserver
— by which point the PVC informer has synced. volcano-sh#5058 added a DRA-specific
happy path using the correct `addTask + resyncTask + return nil`
pattern, but also added a generic `return err` that removed the
implicit recovery for PVC-not-found.
Extend the DRA branch to PVC-not-found errors, using the same pattern.
Change getPodCSIVolumes to wrap the lister error with %w so callers
can classify it with errors.Is / apierrors.IsNotFound instead of
matching on the message string.
Adds `TestAddPodWithUnresolvedPVCCachesTaskForResync` mirroring the
existing DRA counterpart.
Version scope: only v1.15.0 is affected. Verified with
`git merge-base --is-ancestor 5ba79f3 <tag>` — every v1.14.x tag
through v1.14.3 (released after v1.15.0) is clean; v1.15.0-alpha.0 is
also clean.
Signed-off-by: Gustavo Parcianello <gustavo.parcianello@sap.com>
A Pod ADD event can reach the scheduler before its referenced PVC ADD
reaches the PVC informer — for example when a Pod is created together
with an OnDemand PVC in quick succession (Spark's k8s scheduler-backend
does exactly this for executor pods with `claimName=OnDemand`).
Since v1.15.0, the resulting `looking up PVC ... not found` error from
NewTaskInfo permanently strands the pod:
addPod → NewTaskInfo → addPodCSIVolumesToTask
→ pvcInformer.Lister().Get(pvcName) returns not-found
→ klog.Errorf(...)
→ sc.resyncTask(pi) // enqueues taskKey into errTasks
→ return err // task never enters sc.Jobs
processResyncTask → errTasks.Get() → parseErrTaskKey → getTaskByUID
→ "failed to find task <uid>"
→ errTasks.Forget(taskKey) // permanent
→ return // never reaches retryResyncTask
There is no PVC-informer AddFunc that re-queues stuck pods, and the
pod's resourceVersion never advances before scheduling, so
`UpdatePod → deletePod + addPod` never fires. Executor pods stay
Pending; the PVC stays Pending with `WaitForPodScheduled`.
Before volcano-sh#5058, `addPod`'s error branch fell through to `sc.addTask(pi)`
even after `NewTaskInfo` returned an error, so the incomplete task
lived in `sc.Jobs` and processResyncTask could find it by UID on the
next tick and call syncTask, which refetches the pod from the apiserver
— by which point the PVC informer has synced. volcano-sh#5058 added a DRA-specific
happy path using the correct `addTask + resyncTask + return nil`
pattern, but also added a generic `return err` that removed the
implicit recovery for PVC-not-found.
Extend the DRA branch to PVC-not-found errors, using the same pattern.
Change getPodCSIVolumes to wrap the lister error with %w so callers
can classify it with errors.Is / apierrors.IsNotFound instead of
matching on the message string.
Adds `TestAddPodWithUnresolvedPVCCachesTaskForResync` mirroring the
existing DRA counterpart.
Version scope: only v1.15.0 is affected. Verified with
`git merge-base --is-ancestor 5ba79f3 <tag>` — every v1.14.x tag
through v1.14.3 (released after v1.15.0) is clean; v1.15.0-alpha.0 is
also clean.
Signed-off-by: Gustavo Parcianello <gustavo.parcianello@sap.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces Dynamic Resource Allocation (DRA) quota support into the Volcano scheduler's [capacity] plugin.
Key features include:
DeviceClassesthat are not explicitly configured in a Queue's limits, the plugin operates in a "pass-through" mode, allowing tasks to use them without imposing quota restrictions.This enhancement ensures fair and controlled distribution of customized DRA resources across multitenant clusters.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
DynamicResourceAllocationEnableandDRAConsumableCapacityEnablefeature flags/plugin arguments.Does this PR introduce a user-facing change?