DRA API: implement ResourceClaim strategy for DRADeviceTaints #132927
Conversation
k8s-ci-robot
added
release-note
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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
added
needs-triage
k8s-ci-robot
added
the
wg/device-management
k8s-ci-robot
removed
the
do-not-merge/needs-sig
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -212,6 +212,7 @@ func toSelectableFields(claim *resource.ResourceClaim) fields.Set { | |||
| // dropDisabledFields removes fields which are covered by a feature gate. | |||
| func dropDisabledFields(newClaim, oldClaim *resource.ResourceClaim) { | |||
| dropDisabledDRAPrioritizedListFields(newClaim, oldClaim) | |||
| dropDisabledDRADeviceTaintsFields(newClaim, oldClaim) // Intentionally after dropDisabledDRAPrioritizedListFields to avoid iterating over FirstAvailable slice which needs to be dropped. | |||
There was a problem hiding this comment.
Don't we need the same functionality in ResourceClaimTemplate?
There was a problem hiding this comment.
Yes. Will add it there, too. Good catch!
There was a problem hiding this comment.
For some reason, ResourceClaimTemplate update testing was less complete than the update testing of ResourceClaim. Fixed by copying the entire TestStrategyUpdate over and switching it to testing ResourceClaimTemplates.
I kept the existing test, to ensure that I am not removing coverage.
| @@ -372,6 +419,26 @@ func TestStrategyCreate(t *testing.T) { | |||
| } | |||
| }, | |||
| }, | |||
| "drop-fields-device-taints": { | |||
There was a problem hiding this comment.
Should we have tests here that covers interaction with the PrioritizedList feature?
| @@ -587,6 +656,54 @@ func TestStrategyUpdate(t *testing.T) { | |||
| } | |||
| }, | |||
| }, | |||
| "drop-fields-device-taints-in-prioritized-list": { | |||
There was a problem hiding this comment.
Should we have some tests that don't include PrioritizedList?
There was a problem hiding this comment.
Yes. Now I am questioning my own sanity. I could have sworn that I had added them.
…or DRADeviceTaints This wasn't possible at the time of implementing the Device Taints API, at least not completely, because it depended on prioritized list being merged first, to cover the "FirstAvailable" field introduced together with that feature. That the device taints PR got merged despite this gap was an oversight. The confusing TODO probably didn't help: the entire implementation was missing (or got lost due to a bad merge conflict resolution, not sure anymore) and it referenced the wrong other feature (partitionable devices doesn't affect ResourceClaim). For some reason, ResourceClaimTemplate update testing was less complete than the update testing of ResourceClaim. Fixed by copying the entire TestStrategyUpdate over and switching it to testing ResourceClaimTemplates.
pohly
force-pushed
the
dra-api-strategy-todo
branch
from
edeec84 to
fd7c1c2
Compare
k8s-ci-robot
added
size/XL
What type of PR is this?
/kind bug
What this PR does / why we need it:
Dropping the disabled "Tolerations" field in the ResourceClaim API was missing.
This wasn't possible at the time of implementing the Device Taints API, at least not completely, because it depended on prioritized list being merged first, to cover the "FirstAvailable" field introduced together with that feature.
That the device taints PR got merged despite this gap was an oversight. The confusing TODO probably didn't help: the entire implementation was missing (or got lost due to a bad merge conflict resolution, not sure anymore) and it referenced the wrong other feature (partitionable devices doesn't affect ResourceClaim).
Which issue(s) this PR is related to:
KEP: kubernetes/enhancements#5055
Special notes for your reviewer:
This allowed clients to set the field when it should have been dropped. It simply had no effect. Clients can keep updating such objects because of the "feature in use" check, as long as the spec remains immutable.
Does this PR introduce a user-facing change?