Skip to content

Implement DRA Device Binding Conditions (KEP-5007) #130160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

Implement DRA Device Binding Conditions (KEP-5007) #130160

wants to merge 48 commits into from
+3,933 −428

Conversation

KobayashiD27
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR implements the KEP-5007 DRA device binding conditions. This feature ensures that the scheduler waits in the PreBind phase until any DRA devices that need preparation are ready.

Which issue(s) this PR fixes:

Related to kubernetes/enhancements#5007

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add new optional APIs in ResouceSlice.Basic and ResourceClaim.Status.AllocatedDeviceStatus.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/5007

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 14, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @KobayashiD27. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added area/code-generation area/kubelet kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. wg/device-management Categorizes an issue or PR as relevant to WG Device Management. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 14, 2025
@KobayashiD27
Copy link
Contributor Author

We are pleased to share the initial implementation of the KEP-5007 DRA device binding conditions. While this PR aligns with the outlined in the KEP, we recognize that there may be areas for improvement. We invite the community to review the implementation and provide feedback and insights to help refine and enhance this feature.

@pohly @johnbelamaric
First, from the perspective of DRA, I would like to confirm that the overall approach aligns with the intended direction. Your insight would be invaluable in ensuring that we are on the right track.

pohly
pohly reviewed Feb 14, 2025
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Some quick comments. I have looked at the API and allocator, but not the scheduler plugin.

The API lacks validation, but that's of course okay when the main goal for now is to try out the functionality.

staging/src/k8s.io/api/resource/v1beta1/types.go Outdated
const (
// IsPrepared indicates the device ready state.
// If NeedToPreparing is True and IsPrepared is True, the scheduler proceeds to Bind.
IsPrepared = "dra.example.com/is-prepared"
Copy link
Contributor

Choose a reason for hiding this comment

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

This was just an example in the KEP. It doesn't belong into the upstream API. Same for PreparingFailed.

staging/src/k8s.io/dynamic-resource-allocation/structured/allocator.go Outdated
})
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the device status only when needed by a device.

You also have to add feature gate checking: I don't remember whether it was spelled out explicitly in the KEP (if not, please add in a follow-up), but what would make sense to me is to ignore devices which have binding conditions when the feature is turned off. In other words, don't select them because the code which waits during binding wouldn't be active.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add allocatedDeviceStatus only when BindingConditions exist.
and I pass the featureGate status of BindingConditions to the allocator and check it.

@KobayashiD27 KobayashiD27 mentioned this pull request Feb 18, 2025
Open
@KobayashiD27
Copy link
Contributor Author

@pohly
Thank you for the initial feedback. To ensure we are on the right track, I would like to highlight a few key areas where community consensus is crucial. It would be helpful if you could prioritize reviewing the following aspects:

  1. Transferring AllocatedDeviceStatus from the allocator to the scheduler plugin
  2. Clearing the allocation state in the UnReserve phase

Early feedback on these sections would be very helpful.

Additionally, regarding the comment about the lack of API validation, are you referring to pkg/apis/resources/validation/validation.go?

@pohly pohly mentioned this pull request Feb 18, 2025
Open
6 tasks
@pohly
Copy link
Contributor

pohly commented Feb 19, 2025

/milestone v1.33

@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Feb 19, 2025
pohly
pohly reviewed Feb 19, 2025
View reviewed changes
johnbelamaric
johnbelamaric reviewed Feb 21, 2025
View reviewed changes
@KobayashiD27 KobayashiD27 force-pushed the dra-device-binding-conditions branch from 1c36f41 to f07e273 Compare February 25, 2025 00:18
KobayashiD27 and others added 25 commits June 30, 2025 16:33
@KobayashiD27 @pohly
Update pkg/scheduler/framework/plugins/dynamicresources/dynamicresour…
7a998e3 
…ces.go

Co-authored-by: Patrick Ohly <[email protected]>
@KobayashiD27
Use the assume cache instead of doing a GET call in hasDeviceBindingS…
d46bc84 
…tatus()
@KobayashiD27
Changes the Event to be emitted only when the wait begins.
16946f7
@KobayashiD27
Add deadline context
09d38a5
@KobayashiD27
fix unit test for binding conditions
19e9cb4
@KobayashiD27
Add BindingConditions to v1beta2 resource API
f4e1f0f
@KobayashiD27
Add BindingCondition conversion process
dcf2815
@KobayashiD27
Add BindingStartTime to AllocationResult
2deab7e
@KobayashiD27
Add check for ResourceClaim timeout
5bb775e
@KobayashiD27
Change GatherPools() to prefer node-local devices
5f2ba84
@KobayashiD27
Change BindingStartTime to AllocationTimestamp
3fd92a0
@KobayashiD27
Fix linter check error
bd63ed7
@KobayashiD27 @pohly
Update pkg/apis/resource/types.go
b2c5b6f 
Co-authored-by: Patrick Ohly <[email protected]>
@KobayashiD27 @pohly
Update pkg/apis/resource/types.go
6a9ec56 
Co-authored-by: Patrick Ohly <[email protected]>
@KobayashiD27 @pohly
Update pkg/apis/resource/validation/validation.go
dc8fdb1 
Co-authored-by: Patrick Ohly <[email protected]>
@KobayashiD27 @pohly
Update pkg/apis/resource/validation/validation.go
d348ac1 
Co-authored-by: Patrick Ohly <[email protected]>
@KobayashiD27 @pohly
Update pkg/features/kube_features.go
0e89fbf 
Co-authored-by: Patrick Ohly <[email protected]>
@KobayashiD27
Fix missing dropDisableFields in ResouceClaim validation
fa8416d
@KobayashiD27
Fix order of featuregate definitions
51f1b34
@KobayashiD27
Fix unit test
3fd3adc
@KobayashiD27
Address comments on Scheduler_perf updates
76f6887
@KobayashiD27 @pohly
Update test/integration/scheduler_perf/dra.go
b41fb9d 
Co-authored-by: Patrick Ohly <[email protected]>
@KobayashiD27
fix allocator_test: apply wrapResourceSlice
b868727
@KobayashiD27
update kube_feature
ccdd60e
@KobayashiD27
Fix logic to favor devices without binding conditions
2f9ace7
@KobayashiD27 KobayashiD27 force-pushed the dra-device-binding-conditions branch from fd3ec1f to 607bb1a Compare June 30, 2025 07:40
@KobayashiD27 KobayashiD27 force-pushed the dra-device-binding-conditions branch from 8bd3cc2 to 12320f9 Compare July 1, 2025 06:26
@KobayashiD27
Copy link
Contributor Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@mrunalp mrunalp mrunalp left review comments

@haosdent haosdent haosdent left review comments

@hase1128 hase1128 hase1128 left review comments

@zhouhao3 zhouhao3 zhouhao3 left review comments

@bart0sh bart0sh Awaiting requested review from bart0sh

@damemi damemi Awaiting requested review from damemi

@sanposhiho sanposhiho Awaiting requested review from sanposhiho

@dom4ha dom4ha Awaiting requested review from dom4ha

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

@macsko macsko Awaiting requested review from macsko

@thockin thockin Awaiting requested review from thockin

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@thockin thockin

@johnbelamaric johnbelamaric

@macsko macsko

@dom4ha dom4ha

Labels
api-review Categorizes an issue or PR as actively needing an API review. area/code-generation area/kubelet area/release-eng Issues or PRs related to the Release Engineering subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/device-management Categorizes an issue or PR as relevant to WG Device Management.
Projects
API Reviews
Status: Assigned
Dynamic Resource Allocation
Status: 👀 In review
SIG Apps
Status: Needs Triage
SIG Auth
Status: !SIG Auth
SIG CLI
Status: Needs Triage
SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Status: Work in progress
[sig-release] Bug Triage
Status: Tracked
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.