Implement DRA Device Binding Conditions (KEP-5007) #130160
Conversation
|
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 Once the patch is verified, the new status will be reflected by the 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. |
|
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 |
| 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" |
There was a problem hiding this comment.
This was just an example in the KEP. It doesn't belong into the upstream API. Same for PreparingFailed.
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I add allocatedDeviceStatus only when BindingConditions exist.
and I pass the featureGate status of BindingConditions to the allocator and check it.
|
@pohly
Early feedback on these sections would be very helpful. Additionally, regarding the comment about the lack of API validation, are you referring to |
|
/milestone v1.33 |
pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go
Outdated
Show resolved
Hide resolved
pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go
Outdated
Show resolved
Hide resolved
1c36f41 to
f07e273
Compare
Signed-off-by: Zhou Hao <zhouhao@fujitsu.com>
…ces.go Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
57f494a to
8e56894
Compare
|
@pohly @johnbelamaric (for DRA) Could you please review the full implementation from each of your respective perspectives? |
| // feature gate. | ||
| // | ||
| // +optional | ||
| // +featureGate=DRADeviceBindingConditions,DRAResourceClaimDeviceStatus |
There was a problem hiding this comment.
I could imagine use cases for this besides DRADeviceBindingConditions, so making it depend on that feature gate might be too strict.
But introducing a new field in this KEP with just DRAResourceClaimDeviceStatus as feature gate (not really the right one) or no feature gate at all (core DRA = structured parameters) would be odd. Let's keep it like this.
| Device: goodName, | ||
| AdminAccess: ptr.To(false), | ||
| BindingConditions: []string{"condition1", "condition2"}, | ||
| BindingFailureConditions: []string{"condition3", "condition4"}, |
There was a problem hiding this comment.
Let's extend this to four conditions in both slices (current maximum) and include a complex label:
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Condition
There was a problem hiding this comment.
I've added those test cases.
pkg/features/kube_features.go
Outdated
| @@ -1173,6 +1181,10 @@ var defaultVersionedKubernetesFeatureGates = map[featuregate.Feature]featuregate | ||
| {Version: version.MustParse("1.32"), Default: false, PreRelease: featuregate.Beta}, | ||
| }, | ||
|
|
||
| DRADeviceBindingConditions: { | ||
| {Version: version.MustParse("1.33"), Default: false, PreRelease: featuregate.Alpha}, | ||
| }, |
There was a problem hiding this comment.
Please move it before DRADeviceTaints and change it to 1.34.
There was a problem hiding this comment.
Understood. I will move it over to arrange it in alphabetical order.
My understanding is that the client hasn't been updated to v1.34 yet. If that's the case, there might be issues with testing, etc. Therefore I'll wait to change it to v1.34 until we can confirm the client is updated.
| }(), | ||
| bindingConditions: true, | ||
| deviceStatus: true, | ||
| }, |
There was a problem hiding this comment.
For the sake of completeness: let's add bindingConditions: true, deviceStatus: false and bindingConditions: false, deviceStatus: true.
There was a problem hiding this comment.
I've added those test cases.
There was a problem hiding this comment.
We need the same "drop disabled fields" in resourceclaim/strategy.go.
There was a problem hiding this comment.
Add dropDisabledFields for bindingConditions in resourceclaim/strategy.go
Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
Co-authored-by: Patrick Ohly <patrick.ohly@intel.com>
There was a problem hiding this comment.
scheduler_perf review: would be easier to understand with a bit more documentation.
These new test cases need a lot of new plumbing (= operations). I'm undecided whether this is the right approach for them. Let me experiment a bit with doing the same under test/integration/dra.
| @@ -235,6 +238,10 @@ func resourceSlice(driverName, nodeName string, capacity int) *resourceapi.Resou | ||
| Capacity: map[resourceapi.QualifiedName]resourceapi.DeviceCapacity{ | ||
| "memory": {Value: resource.MustParse("1Gi")}, | ||
| }, | ||
| UsageRestrictedToNode: ptr.To(true), |
There was a problem hiding this comment.
I was wondering how this works in other tests until I realized that these fields get stripped when the feature gate is off.
Please add a comment explaining this.
There was a problem hiding this comment.
OK I will add a comment here.
| @@ -363,3 +371,278 @@ claims: | ||
| tCtx.Fatalf("Could not allocate claim %d out of %d", i, len(claims)) | ||
| } | ||
| } | ||
|
|
||
| type updateDeviceConditionsOp struct { |
There was a problem hiding this comment.
Please add documentation for the new operations:
- what do they do?
- what parameters do they have?
There was a problem hiding this comment.
I have added documentation for the new op and values.
|
@KobayashiD27: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: