DRA: migrate DeviceTaint and Conditions to declarative validation#135460
DRA: migrate DeviceTaint and Conditions to declarative validation#135460dumko2001 wants to merge 12 commits into
Conversation
|
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 DetailsInstructions 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. |
|
Welcome @dumko2001! |
|
Hi @dumko2001. Thanks for your PR. I'm waiting for a github.com 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. DetailsInstructions 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. |
a6760d0 to
46eccab
Compare
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
lalitc375
left a comment
There was a problem hiding this comment.
Thank you for the PR @dumko2001 . Totally appreciate it.
Please follow this pattern for adding tags.
The ideal PR structure for a PR contributing a DV tag migration is:
Commit # 1: IF NOT ALREADY WIRED UP FOR DV - Wire up the apigroup, add empty test and empty generated file. I don't need to review this, you know the pattern by now.
Commit # 2: Use a single DV tag on a single field. E.g. Add +k8s:required to required field. Includes generated code and robust test cases. Emphasis on test-cases -- we want enough to prove that a field is correct, but we don't want to duplicate all the per-tag tests (e.g. a k8s-short-name field has dozens of corner cases, but we really only need a few). We want each case to be as short and specific as possible, which is why the "tweak" pattern and the use of Origin for Invalid() errors is so important. Specify as little as possible to be sufficiently precise. Newcomers need guidance on this.
Commits # 3...N: Use DV tags on the same field, one per commit; repeat until that field is done or all that is left is not handled by DV yet. Includes generated code and tests for each field. (not the case for the specific examples below as they are for single tag migrations)
Commits N+1...M: Repeat the above for more fields. (not the case for the specific examples below as they are for single field migrations)
The emphasis is on lots of small commits. Maintaining commit discipline is hard but worthwhile.
If DV exposes bugs in the existing testing or the hand-written validation itself (!!), those should be fixed as a PR of its own or as commits at the front of the PR.
| // +optional | ||
| // +listType=atomic | ||
| // +featureGate=DRADeviceTaints | ||
| // +k8s:maxItems=16 |
There was a problem hiding this comment.
Add tests for these new validation tags in pkg/registry/resource/resourceslice/declarative_validation_test.go
|
/ok-to-test |
- Remove invalid +k8s:validation:MinLength=1 tag from DeviceTaint.Key - Remove redundant +k8s:validation:Enum tag from DeviceTaint.Effect - Add missing +k8s:optional tag to AllocatedDeviceStatus.Conditions - Applied to v1, v1beta1, and v1beta2 API versions
- Add 'valid: at limit taints' test (16 taints) for ResourceSlice - Add 'valid: at limit conditions' test (8 conditions) for ResourceClaim - Add 'invalid: duplicate condition type' test for +k8s:listMapKey=type - Move ResourceVersion assignment to test loop - Add normalization rule for conditions path matching - Mark duplicate condition errors as covered by declarative
2aa2ff8 to
e3d841f
Compare
e3d841f to
5ac5f9b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dumko2001 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@lalitc375 rebased PR is now green on The small follow-up in Could you please take another look when you have a moment? |
|
PR needs rebase. DetailsInstructions 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 just realized this has overlap with #139101 . Can you remove DV from conditions in this PR? Please also rebase |
What type of PR is this?
/kind cleanup
/kind feature
What this PR does / why we need it:
This PR migrates the validation for Device.Taints and AllocatedDeviceStatus.Conditions from manual Go code to declarative markers, addressing items from the tracking issue #135073.
Changes:
staging/src/k8s.io/api/resource/v1/types.go:
Added // +k8s:maxItems=16 to Device.Taints.
Added // +k8s:maxItems=8, // +k8s:listType=map, and // +k8s:listMapKey=type to AllocatedDeviceStatus.Conditions.
pkg/apis/resource/validation/validation.go:
Updated validateDevice to mark Taints size as covered by declarative validation (sizeCovered).
Removed the manual length check for Conditions in validateDeviceStatus.
Ran make update to synchronize generated files (zz_generated.* and generated.proto).
Which issue(s) this PR is related to:
Fixes #135073
Special notes for your reviewer:
The tracking issue suggested adding // +k8s:unique=map for AllocatedDeviceStatus.Conditions.
I have omitted this specific tag because it causes a conflict in update-codegen:
tag "k8s:unique": unique tag may not be used with listType=set or listType=map
Since listType=map already enforces uniqueness on the key (type), the explicit unique tag is redundant.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: