Add validation for unsupported DRA features #7226

harche · 2025-10-10T16:54:51Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR adds validation to reject DRA (Dynamic Resource Allocation) v1 API features that are enabled
by default in Kubernetes 1.34 but explicitly excluded from Kueue's DRA support scope per KEP-2941.

Without validation, Kueue silently ignores unsupported DRA features, leading to:

Silent admission failures - FirstAvailable workloads are admitted with 0 DRA resources, causing
runtime failures
Misleading errors - 5 out of 6 unsupported features produce confusing "DeviceClass not mapped"
errors instead of clear feature-not-supported messages
Poor UX - Users waste time debugging DeviceClass mappings when the actual issue is using
unsupported features

Solution is to add early validation in pkg/dra/claims.go to explicitly reject all 6 enabled-by-default DRA features
that Kueue doesn't support:

GA Features:

AllocationMode 'All'
CEL Selectors
Device Constraints
Device Config

Beta Features (enabled by default in K8s 1.34):

FirstAvailable (DRAPrioritizedList)
AdminAccess (DRAAdminAccess)

Each feature now returns a clear, actionable error message like "FirstAvailable device selection is
not supported" instead of misleading DeviceClass mapping errors.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Kueue now properly validates and rejects unsupported DRA (Dynamic Resource Allocation) features with clear error messages instead of silently failing or producing misleading "DeviceClass not mapped" errors. Unsupported features include: AllocationMode 'All', CEL Selectors, Device Constraints, Device Config, FirstAvailable device selection, and AdminAccess.

netlify · 2025-10-10T16:55:17Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`8de8f8c`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/68f659fe9cf72200082b1be9

k8s-ci-robot · 2025-10-10T16:55:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: harche
Once this PR has been reviewed and has the lgtm label, please assign gabesaba for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-10-10T16:55:20Z

Hi @harche. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

mimowo · 2025-10-10T16:58:27Z

/ok-to-test
@alaypatel07 would you like to give it a pass?

kannon92 · 2025-10-10T17:00:45Z

/ok-to-test

/cc @alaypatel07

alaypatel07 · 2025-10-10T17:11:56Z

Yes I'll take a look.

alaypatel07 · 2025-10-10T17:42:48Z

pkg/dra/claims.go

 		var dcName string
 		var q int64
 		if req.Exactly != nil {
+			// Check for CEL selectors


can you please convert the entire if condition into a switch case handling all the unsupported features first before considering the valid supported case?

Addressed, thanks.

@harche why does this have to be nested ifs and switch cases? Can we work out a logic with one single switch case?

For example:

switch { case req.FirstAvailable != nil: case req.Exactly != nil && len(req.Exactly.Selectors) > 0: // error case req.Exactly != nil && req.Exactly.AdminAccess != nil && *req.Exactly.AdminAccess: // error case req.Exactly != nil && req.Exactly.AllocationMode == DeviceAllocationModeAll: // error }

@alaypatel07 I didn't want to repeat req.Exactly != nil, would something like following do?

switch { case req.FirstAvailable != nil: return nil, fmt.Errorf("%w: FirstAvailable device selection is not supported", ErrUnsupportedDRAFeature) case req.Exactly == nil: continue case len(req.Exactly.Selectors) > 0: return nil, fmt.Errorf("%w: CEL selectors are not supported", ErrUnsupportedDRAFeature) case req.Exactly.AdminAccess != nil && *req.Exactly.AdminAccess: return nil, fmt.Errorf("%w: AdminAccess is not supported", ErrUnsupportedDRAFeature) case req.Exactly.AllocationMode == resourcev1.DeviceAllocationModeAll: return nil, fmt.Errorf("%w: AllocationMode 'All' is not supported", ErrUnsupportedDRAFeature) case req.Exactly.AllocationMode == resourcev1.DeviceAllocationModeExactCount: dcName = req.Exactly.DeviceClassName q = req.Exactly.Count case req.Exactly.AllocationMode == "": dcName = req.Exactly.DeviceClassName default: return nil, fmt.Errorf("%w: unsupported allocation mode: %s", ErrUnsupportedDRAFeature, req.Exactly.AllocationMode) }

Updated, let me know if that works, but I am open to changing it to repeated req.Exactly != nil.

alaypatel07 · 2025-10-10T17:45:55Z

pkg/dra/claims.go

-	ErrDeviceClassNotMapped = errors.New("DeviceClass is not mapped in DRA configuration")
-	ErrResourceClaimInUse   = errors.New("ResourceClaim is in use")
-	ErrClaimSpecNotFound    = errors.New("failed to get claim spec")
+	ErrDeviceClassNotMapped          = errors.New("DeviceClass is not mapped in DRA configuration")


Nit: can we use a common error "UnsupportedDRAfeature" and then wrap a human readable error message to be returned from the function later.

I think we only need to have the following concrete error types:

ErrResourceClaimInUse and other

ErrClaimSpecNotFound

ErrUnsupportedDRAfeature

Addressed, thanks.

alaypatel07 · 2025-10-10T17:47:38Z

pkg/dra/claims_test.go

 			},
 			want: map[kueuev1beta1.PodSetReference]corev1.ResourceList{"main": {"res-1": resource.MustParse("2")}},
 		},
+		{


we should also update the integration tests for these errors at https://github.com/kubernetes-sigs/kueue/tree/main/test/integration/singlecluster/controller/dra

Added integration tests.

harche · 2025-10-10T19:10:44Z

/test pull-kueue-test-e2e-multikueue-main

harche · 2025-10-10T19:13:08Z

@alaypatel07 I have updated the PR. Can you take another look? Thanks.

alaypatel07 · 2025-10-10T20:42:25Z

test/integration/singlecluster/controller/dra/dra_test.go

+			wl := utiltesting.MakeWorkload("test-wl-all-mode", ns.Name).
+				Queue("test-lq").
+				Obj()
+			wl.Spec.PodSets[0].Template.Spec.ResourceClaims = []corev1.PodResourceClaim{


can we use this opportunity to start using, https://github.com/kubernetes-sigs/kueue/blob/main/pkg/util/testing/wrappers.go#L623 and https://github.com/kubernetes-sigs/kueue/blob/main/pkg/util/testing/wrappers.go#L637?

I think this change is needed across all the test cases.

Updated, thanks.

alaypatel07 · 2025-10-10T20:44:33Z

test/integration/singlecluster/controller/dra/dra_test.go

+
+		ginkgo.It("Should reject workload with AllocationMode 'All'", func() {
+			ginkgo.By("Creating a ResourceClaimTemplate with AllocationMode All")
+			rct := &resourcev1.ResourceClaimTemplate{


I wonder if we can create a ResourceClaimWrapper to produce these templates/claims in utiltesting:

kueue/pkg/util/testing/wrappers.go

Line 422 in 5a6b10a

type PodSetWrapper struct{ kueue.PodSet }

it will be helpful in unit testing as well as integration tests

Sounds like a great idea, but should we tackle this outside this PR? I can create a separate issue to look into it. I am afraid it might escalate the scope of this PR (which is just to harden the validation around unsupported DRA fields/features)

Added the wrapper, https://github.com/kubernetes-sigs/kueue/pull/7226/files#diff-8c600775ee9d35183196effef13d4df7be756e077046325d17b6214b080cba2bR1823

+1. IMHO, since this PR is adding all the permutations of creating resource slices with different features, this is the right moment to add the wrapper. I dont really think it expands the scope. Thanks for tackling that.

alaypatel07 · 2025-10-13T12:35:03Z

pkg/dra/claims.go

-				q = req.Exactly.Count
-			}
+			q = req.Exactly.Count
+		case req.Exactly.AllocationMode == "":


This case is not needed, Kueue should assume that the defaults are set appropriately during resource admission: https://github.com/kubernetes/kubernetes/blob/f306bb14b4a288cd04c6ddad65a5a91bc1f665f8/pkg/apis/resource/v1/defaults.go#L32-L37.

Addressed, thanks.

alaypatel07 · 2025-10-13T12:38:04Z

pkg/util/testing/wrappers.go

+
+// ResourceClaimTemplateWrapper wraps a resourcev1.ResourceClaimTemplate
+type ResourceClaimTemplateWrapper struct {
+	resourcev1.ResourceClaimTemplate


nit: Instead of having separate structs for RCT and RC, there should be a common struct for ResourceClaimSpec and the With* functions use that struct to generate the spec. I think will remove a lot of duplication.

Between ResourceClaimTemplateWrapper and ResourceClaimWrapper, there's now a shared ResourceClaimSpecBuilder that both wrappers use, let me know if that aligns with what you had in mind.

cc @alaypatel07

kannon92 · 2025-10-15T19:29:24Z

Please add a release note.

harche · 2025-10-16T01:50:04Z

Please add a release note.

Thanks, done.

alaypatel07 · 2025-10-16T01:58:57Z

@harche I'm falling behind on reviews due to the upcoming KEP freeze. I'll take a look first thing tomorrow morning.

harche · 2025-10-16T09:04:42Z

@harche I'm falling behind on reviews due to the upcoming KEP freeze. I'll take a look first thing tomorrow morning.

No worries @alaypatel07 , take your time. This can wait until the KEP freeze deadline passes. Thanks for your valuable feedback.

mimowo · 2025-10-16T14:12:30Z

pkg/controller/core/workload_controller_test.go

+			resourceClaims: []*resourcev1.ResourceClaim{
+				utiltesting.MakeResourceClaim("rc1", "ns").
+					DeviceRequest("device-request", "gpu.example.com", 1).
+					Obj(),
+			},


Actually could you please send a preaparatory non-functional PR just for the cleanup?

So that we don't mix cleanups with features or bugfixes.

It will make the diff here smaller for the ease of reviewing / focusing on the user-changing experience.

Thanks @mimowo, I have brought this up earlier, #7226 (comment)

@alaypatel07 are you okay with splitting the refactoring of tests utils from this PR?

@harche yes, I think what @mimowo is suggesting is little different from earlier conversation, instead of following up on the refactor, I think he wants the refactor to go in first as a separate PR, I am okay with that. Correct me if I am wrong.

Thanks @alaypatel07 and @mimowo, I have raised #7300 to add wrapper for existing tests cases only.

We will deal with this PR once that one is merged.

/hold

I merged the other PR already, please rebase, let me unhold in the meanwhile, it will not merge anyway while not rebased.
/unhold

This commit introduces test helper wrappers for DRA ResourceClaim and ResourceClaimTemplate objects to improve test readability and reduce boilerplate. Related to PR kubernetes-sigs#7226 review comment: kubernetes-sigs#7226 (comment)

mimowo · 2025-10-20T14:57:21Z

pkg/dra/claims_test.go

+				if dc == "test-deviceclass-1" {
+					return "res-1", true
+				}
+				return "", false


This looks very verbose and repeatable, can we replace it somehow with some default lookups maybe?

Added a common defaultLookup function

mimowo · 2025-10-20T14:58:50Z

pkg/dra/claims_test.go

+				}
+				return "", false
+			},
+			wantErr: true,


Instead of asserting true/false it is better to assert on specific validation error, please check https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/jobs/jobset/jobset_webhook_test.go#L49

I know it was like this before, but I would like to use it as an occasion to cleanup

Thanks, updated to assert exact error.

harche · 2025-10-20T15:44:46Z

/hold just like unit tests, the integration tests should also check for exact error.

Signed-off-by: Harshal Patil <[email protected]>

harche · 2025-10-20T15:52:03Z

/hold just like unit tests, the integration tests should also check for exact error.

updated.

/unhold

mimowo · 2025-10-20T15:56:54Z

pkg/dra/claims.go

+	ErrResourceClaimInUse    = errors.New("ResourceClaim is in use")
+	ErrClaimSpecNotFound     = errors.New("failed to get claim spec")
+	ErrUnsupportedDRAFeature = errors.New("unsupported DRA feature")
+
+	// Specific unsupported DRA feature errors
+	ErrUnsupportedDRADeviceConstraints = errors.New("device constraints (MatchAttribute) are not supported")
+	ErrUnsupportedDRADeviceConfig      = errors.New("device config is not supported")
+	ErrUnsupportedDRAFirstAvailable    = errors.New("FirstAvailable device selection is not supported")
+	ErrUnsupportedDRACELSelectors      = errors.New("CEL selectors are not supported")
+	ErrUnsupportedDRAAdminAccess       = errors.New("AdminAccess is not supported")
+	ErrUnsupportedDRAAllocationModeAll = errors.New("AllocationMode 'All' is not supported")


Do these need to be exposed? If not then unexport (lowercase)

They are used in integration tests, besides that, I am not sure having errors exported causes any harm, future callers might be able to verify if the errors belongs to any of the DRA related errors.

We can always export them later if needed. If they are unexported it is immediately obvious which are used or not outside the package for example during review in GH. Also, sometimes exported constants become unused but linters don't complain, so making them unused is easy to miss during review/ code changing.

mimowo · 2025-10-20T16:02:23Z

pkg/dra/claims_test.go

+			if tc.wantErr != nil {
+				if err == nil {
+					t.Fatalf("expected error but got none")
+				}
+				if !errors.Is(err, tc.wantErr) {
+					t.Fatalf("unexpected error: got=%v, want=%v", err, tc.wantErr)
+				}
 				return
 			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}


I would prefer to assert errors as we do here, using cmp.Diff:

kueue/pkg/controller/jobs/jobset/jobset_webhook_test.go

Lines 273 to 291 in 1fa1463

wantErr: field.ErrorList{

field.Invalid(field.NewPath("spec.replicatedJobs[0].template.spec.template.metadata.annotations"), field.OmitValueType{}, "must specify 'kueue.x-k8s.io/podset-required-topology' or 'kueue.x-k8s.io/podset-preferred-topology' topology consistent with 'spec.replicatedJobs[1].template.spec.template.metadata.annotations' in group 'groupname'"),

field.Invalid(field.NewPath("spec.replicatedJobs[1].template.spec.template.metadata.annotations"), field.OmitValueType{}, "must specify 'kueue.x-k8s.io/podset-required-topology' or 'kueue.x-k8s.io/podset-preferred-topology' topology consistent with 'spec.replicatedJobs[0].template.spec.template.metadata.annotations' in group 'groupname'"),

}.ToAggregate(),

topologyAwareScheduling: true,

},

}

for _, tc := range testcases {

t.Run(tc.name, func(t *testing.T) {

features.SetFeatureGateDuringTest(t, features.TopologyAwareScheduling, tc.topologyAwareScheduling)

jsw := &JobSetWebhook{}

ctx, _ := utiltesting.ContextWithLog(t)

_, gotErr := jsw.ValidateCreate(ctx, tc.job)

if diff := cmp.Diff(tc.wantErr, gotErr); diff != "" {

t.Errorf("validateCreate() mismatch (-want +got):\n%s", diff)

}

Would it work here? I'm not sure maybe it works better if the validation errors are using field.ErrorList type.

The current tests use errors.Is, this is exactly what we want. IMO, achieving this in any other way would not be more optimal than simply using errors.Is.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 10, 2025

k8s-ci-robot requested review from PBundyra and mimowo October 10, 2025 16:55

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 10, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 10, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 10, 2025

k8s-ci-robot requested a review from alaypatel07 October 10, 2025 17:00

harche force-pushed the dra_validation branch from c524414 to 4318946 Compare October 10, 2025 17:20

alaypatel07 reviewed Oct 10, 2025

View reviewed changes

harche force-pushed the dra_validation branch from 4318946 to 006e7b3 Compare October 10, 2025 18:52

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2025

harche requested a review from alaypatel07 October 10, 2025 18:54

alaypatel07 reviewed Oct 10, 2025

View reviewed changes

harche force-pushed the dra_validation branch 2 times, most recently from e6127d8 to 03ca256 Compare October 10, 2025 23:12

alaypatel07 reviewed Oct 13, 2025

View reviewed changes

harche force-pushed the dra_validation branch from 1f307fd to 33f5c56 Compare October 14, 2025 15:12

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 16, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2025

mimowo reviewed Oct 16, 2025

View reviewed changes

harche mentioned this pull request Oct 16, 2025

Add DRA ResourceClaim wrappers for test infrastructure #7300

Merged

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 16, 2025

harche force-pushed the dra_validation branch from 33f5c56 to 5f56b50 Compare October 20, 2025 14:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 20, 2025

mimowo reviewed Oct 20, 2025

View reviewed changes

harche force-pushed the dra_validation branch from 5f56b50 to 8d24bce Compare October 20, 2025 15:36

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2025

Add validation for unsupported DRA features

8de8f8c

Signed-off-by: Harshal Patil <[email protected]>

harche force-pushed the dra_validation branch from 8d24bce to 8de8f8c Compare October 20, 2025 15:49

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2025

mimowo reviewed Oct 20, 2025

View reviewed changes

	wantErr: field.ErrorList{
	field.Invalid(field.NewPath("spec.replicatedJobs[0].template.spec.template.metadata.annotations"), field.OmitValueType{}, "must specify 'kueue.x-k8s.io/podset-required-topology' or 'kueue.x-k8s.io/podset-preferred-topology' topology consistent with 'spec.replicatedJobs[1].template.spec.template.metadata.annotations' in group 'groupname'"),
	field.Invalid(field.NewPath("spec.replicatedJobs[1].template.spec.template.metadata.annotations"), field.OmitValueType{}, "must specify 'kueue.x-k8s.io/podset-required-topology' or 'kueue.x-k8s.io/podset-preferred-topology' topology consistent with 'spec.replicatedJobs[0].template.spec.template.metadata.annotations' in group 'groupname'"),
	}.ToAggregate(),
	topologyAwareScheduling: true,
	},
	}

	for _, tc := range testcases {
	t.Run(tc.name, func(t *testing.T) {
	features.SetFeatureGateDuringTest(t, features.TopologyAwareScheduling, tc.topologyAwareScheduling)

	jsw := &JobSetWebhook{}
	ctx, _ := utiltesting.ContextWithLog(t)
	_, gotErr := jsw.ValidateCreate(ctx, tc.job)

	if diff := cmp.Diff(tc.wantErr, gotErr); diff != "" {
	t.Errorf("validateCreate() mismatch (-want +got):\n%s", diff)
	}

Add validation for unsupported DRA features #7226

Are you sure you want to change the base?

Add validation for unsupported DRA features #7226

Conversation

harche commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

netlify bot commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

k8s-ci-robot commented Oct 10, 2025

Uh oh!

k8s-ci-robot commented Oct 10, 2025

Uh oh!

mimowo commented Oct 10, 2025

Uh oh!

kannon92 commented Oct 10, 2025

Uh oh!

alaypatel07 commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harche commented Oct 10, 2025

Uh oh!

harche commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kannon92 commented Oct 15, 2025

Uh oh!

harche commented Oct 16, 2025

Uh oh!

alaypatel07 commented Oct 16, 2025

Uh oh!

harche commented Oct 16, 2025

Uh oh!

mimowo Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

harche commented Oct 10, 2025 •

edited

Loading

netlify bot commented Oct 10, 2025 •

edited

Loading

mimowo Oct 16, 2025 •

edited

Loading

harche Oct 20, 2025 •

edited

Loading