feat: Support DRA Admin Access #8063

MenD32 · 2025-04-27T18:45:13Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is a part of Dynamic Resource Allocation (DRA) support in Cluster Autoscaler. ResourceClaims support the AdminAccess field which is used to allow cluster administrators to access devices already in use. This changes the CA's business logic by introducing the idea that "some resourceclaims don't reserve their allocated devices".

Which issue(s) this PR fixes:

Fixes #7685

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ResourceClaims with AdminAccess will now be ignored when calculating node utilization for scaledown

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

- [KEP]: https://github.com/kubernetes/enhancements/blob/a55eefc6051d6684d8cc7521e1f4de6319625e23/keps/sig-auth/5018-dra-adminaccess/README.md

k8s-ci-robot · 2025-04-27T18:45:24Z

Hi @MenD32. 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.

jackfrancis · 2025-04-28T23:26:43Z

cluster-autoscaler/simulator/dynamicresources/utils/utilization.go

 	result := map[string]map[string][]string{}
 	for _, claim := range claims {
-		alloc := claim.Status.Allocation
+		claimCopy := ClaimWithoutAdminAccessRequests(claim)


It might be cleaner to do this in CalculateDynamicResourceUtilization ? We'd have to wrap an enumerator around ClaimWithoutAdminAccessRequests, but at least we could prune the claims data at the source so that any additional downstream usages of it in the future get that pruning for free. (I don't think that CA would ever want to operate upon claim devices that are tagged for AdminAccess.)

wdyt?

sounds good to me

Do you think other calls to groupAllocatedDevices would expect adminAccess resource requests to be removed?

MenD32 · 2025-04-29T09:57:20Z

cluster-autoscaler/simulator/utilization/info_test.go

 			wantErr:      cmpopts.AnyError,
 		},
 	} {
+		if tc.testName != "DRA slices and claims present, DRA enabled -> DRA util returned despite being lower than CPU" {


my bad, that was for debugging the test, i need to remove this

jackfrancis · 2025-04-29T17:06:22Z

/ok-to-test

jackfrancis · 2025-04-29T17:06:47Z

/lgtm

/assign @towca

MenD32 · 2025-05-26T11:51:26Z

@towca, could you please take a look at this PR when you get a chance?

cluster-autoscaler/simulator/dynamicresources/utils/utilization.go

towca · 2025-06-04T18:44:42Z

cluster-autoscaler/simulator/dynamicresources/utils/utilization_test.go

 			wantHighestUtilization:     0.2,
 			wantHighestUtilizationName: apiv1.ResourceName(fmt.Sprintf("%s/%s", fooDriver, "pool1")),
 		},
+		{


Could you add a test case with both kinds of claims together?

towca · 2025-06-04T18:45:11Z

cluster-autoscaler/simulator/dynamicresources/utils/utilization_test.go

 						Devices: resourceapi.DeviceAllocationResult{
 							Results: []resourceapi.DeviceRequestAllocationResult{
-								{Request: fmt.Sprintf("request-%d", podDevIndex), Driver: driverName, Pool: poolName, Device: devName},
+								{Request: devReqName, Driver: driverName, Pool: poolName, Device: devName},


The result has an AdminAccess field as well.

nice catch, totally missed that, I'll fix

towca · 2025-06-04T20:06:25Z

cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go

 		claimCopy.UID = uuid.NewUUID()
 		claimCopy.Name = fmt.Sprintf("%s-%s", claim.Name, nameSuffix)
 		claimCopy.OwnerReferences = []metav1.OwnerReference{PodClaimOwnerReference(newOwner)}
+		claimCopy = ClaimWithoutAdminAccessRequests(claimCopy)


Hmm, it doesn't feel right modifying the claims to remove the admin-access allocation results. The claims are checked and possibly allocated by the DRA scheduler plugin during Filter. They just don't "block" the allocated devices from being allocated for other claims. If we remove the results we essentially have an "invalid" allocated claim where not all requests have an allocation. Not sure if the DRA scheduler plugin Filters would pass for such a Pod.

IMO we should duplicate the admin-access allocation results that are not Node-local without sanitization. The non-Node-local devices are then "double-booked", but this is fine because admin-access doesn't actually book them. We should still sanitize the Node-local results to avoid pointing to devices that definitely aren't available on the new Node. This should leave the claim in a relatively valid state - Node-local allocations are correctly pointing to the devices from the new Node, non-Node-local allocations point to the same devices as the initial claim did. The only assumption we're making is that if a non-Node-local device is available on oldNode, it will be available on newNode as well.

It seems that we can just slightly modify this function to achieve this:

If a result Pool isn't in oldNodePoolNames but it has admin-access set - add it to sanitizedAllocations as-is instead of returning an error.

I wonder if we can just remove the nodeSelector check, it's pretty redundant with checking against oldNodePoolNames. Otherwise we'd have to move the check after sanitizing result and do something like "don't error if there were any non-Node-local, admin-access results during sanitization".

I'd also definitely add new test cases to unit tests for this function.

@MenD32 ping on this comment. Or maybe you responded and Github is just hiding it from me 😅?

Hi, I forgot to ping you when I addressed that in a commit... sorry about that

MenD32 · 2025-06-07T21:05:21Z

Just finished addressing the review comments, so this PR is ready for review again, @jackfrancis @towca

jackfrancis · 2025-10-03T15:31:17Z

cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go

+	for _, deviceRequestAllocationResult := range claimCopy.Status.Allocation.Devices.Results {
+		// Device requests with AdminAccess don't reserve their allocated resources, and are ignored when scheuling.
+		devReq := getDeviceResultRequest(claim, &deviceRequestAllocationResult)
+		if devReq != nil && devReq.AdminAccess != nil && *devReq.AdminAccess {


We already have the ""k8s.io/utils/ptr" package in our go.mod, so we can change these pointer usages to leverage that package. E.g.:

if ptr.Deref(devReq.AdminAccess, false) {

Ref:

https://pkg.go.dev/k8s.io/utils/ptr#Deref

jackfrancis · 2025-10-03T20:03:21Z

cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go

+		}
+		deviceRequestAllocationResults = append(deviceRequestAllocationResults, deviceRequestAllocationResult)
+	}
+	if claimCopy.Status.Allocation != nil {


L122 already guarantees that claimCopy won't be nil at this point in the execution flow, so we can simply assign deviceRequestAllocationResults to claimCopy.Status.Allocation.Devices.Results without checking for nil here.

jackfrancis · 2025-10-03T20:05:41Z

cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go

+
+// getDeviceResultRequest returns the DeviceRequest for the provided DeviceRequestAllocationResult in the provided ResourceClaim. If no result is found, nil is returned.
+func getDeviceResultRequest(claim *resourceapi.ResourceClaim, deviceRequestAllocationResult *resourceapi.DeviceRequestAllocationResult) *resourceapi.DeviceRequest {
+	if claim.Status.Allocation == nil {


Do we need this defensive check here since we're only calling this from ClaimWithoutAdminAccessRequests (which as implemented would never pass a nil Allocation)

I think its fair to add this check to this function so it could be insulated from how it might be called in the future, not just in ClaimWithoutAdminAccessRequests

Will there be every be a non-nil claim.Spec.Devices.Requests when there is a nil claim.Status.Allocation?

I think so, there could be a race-condition in the DRA plugin where is first adds the device requests and then updates the claim status

I think the future value of this protection is unclear, and this function would be made more clear and maintainable if we enumerated through claim.Spec.Devices.Requests. Having an orthogonal check against claim.Status.Allocation inside the function itself, which seems not integrally related to the if claim.Spec.Devices.Requests[i].Name == deviceRequestAllocationResult.Request { evaluation, is a bit perplexing for naive maintainers (like me!)

@nojnhuh @towca to sanity check my thinking here

I guess I can remove it, or maybe move the check to before the function call, does that sound good?

move the check to before the function call

that makes the most sense to me

alright, should be a quick fix

just noticed we already check the claim's deepcopy so it can just be removed entirely

jackfrancis · 2025-10-03T20:10:58Z

cluster-autoscaler/simulator/dynamicresources/utils/utilization.go

 		}

 		for _, deviceAlloc := range alloc.Devices.Results {
+			if deviceAlloc.AdminAccess != nil && *deviceAlloc.AdminAccess {


can shorten to if ptr.Deref(devAlloc.AdminAccess, false) { (have to import "k8s.io/utils/ptr" into this file as well)

MenD32 · 2025-10-03T21:14:24Z

I wasn't aware of ptr.Deref, this is so useful! especially when working with complex Kubernetes structs

jackfrancis · 2025-10-06T20:00:46Z

/hold

/lgtm
/approve

@towca might have some more thoughts on this

MenD32 · 2025-11-03T20:37:01Z

/hold

/lgtm /approve

@towca might have some more thoughts on this

@towca did you get a change to look at this PR?

towca · 2025-11-14T18:42:42Z

@MenD32 Sorry for the delay in reviews, I've been pretty swamped with them recently. I should be able to take a look early next week!

MenD32 · 2025-11-28T14:37:44Z

@MenD32 Sorry for the delay in reviews, I've been pretty swamped with them recently. I should be able to take a look early next week!

Have you gotten a chance to look at this PR yet? (just a friendly ping :) )

towca · 2025-12-01T21:04:29Z

@MenD32 Thanks for the ping and again apologies for the long reviews. I'm almost finished reviewing this one, I was just going to publish the comments but noticed that I completely misread a crucial detail and some of the comments need to be updated. I should have the review out tomorrow!

cluster-autoscaler/simulator/dynamicresources/utils/utilization.go

cluster-autoscaler/simulator/dynamicresources/utils/utilization_test.go

cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go

cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go

cluster-autoscaler/simulator/dynamicresources/utils/sanitize_test.go

MenD32 · 2025-12-02T20:00:32Z

@towca, I amended the PR, feel free to review the changes when you're available!

towca

Thanks a lot for the iteration @MenD32! Just two very minor comments left and we can merge.

Plus a third one - could you squash the commits into a single one?

cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go

cluster-autoscaler/simulator/dynamicresources/utils/utilization.go

cluster-autoscaler/simulator/dynamicresources/utils/utilization_test.go

cluster-autoscaler/simulator/dynamicresources/utils/resource_claims.go

cluster-autoscaler/simulator/dynamicresources/utils/sanitize.go

towca · 2025-12-05T18:45:47Z

cluster-autoscaler/simulator/dynamicresources/utils/sanitize_test.go

 			},
 		},
+		{
+			testName: "allocated and reserved pod-owned claims are sanitized",


This test name is duplicated, could you distinguish it (only non-admin-access allocations are sanitized in this case)?

cluster-autoscaler/simulator/dynamicresources/utils/sanitize_test.go

MenD32 · 2025-12-06T08:34:50Z

Thanks a lot for the iteration @MenD32! Just two very minor comments left and we can merge.

Plus a third one - could you squash the commits into a single one?

Sure!

QQ: what is the difference between squashing manually and using the tide/merge-method-squash label?

Signed-off-by: MenD32 <[email protected]> tests: added utilization test for adminaccess resourceclaims Signed-off-by: MenD32 <[email protected]> fix: ran gofmt Signed-off-by: MenD32 <[email protected]> fix: false positives on SanitizedPodResourceClaims Signed-off-by: MenD32 <[email protected]> fix: non admin results get removed from resourceclaim when requests is empty Signed-off-by: MenD32 <[email protected]> feat: Support DRA Admin Access Signed-off-by: MenD32 <[email protected]> refactor: moved device filtering to groupAllocatedDevices Signed-off-by: MenD32 <[email protected]> tests: added test of combined admin access and non-admin access utilization calculation Signed-off-by: MenD32 <[email protected]> tests: added test for admin access request sanitization Signed-off-by: MenD32 <[email protected]> feat: Support DRA Admin Access Signed-off-by: MenD32 <[email protected]> tests: added utilization test for adminaccess resourceclaims Signed-off-by: MenD32 <[email protected]> fix: ran gofmt Signed-off-by: MenD32 <[email protected]> fix: false positives on SanitizedPodResourceClaims Signed-off-by: MenD32 <[email protected]> fix: non admin results get removed from resourceclaim when requests is empty Signed-off-by: MenD32 <[email protected]> feat: Support DRA Admin Access Signed-off-by: MenD32 <[email protected]> refactor: moved device filtering to groupAllocatedDevices Signed-off-by: MenD32 <[email protected]> fix: removed ClaimWithoutAdminAccessRequests sanitization Signed-off-by: MenD32 <[email protected]> chore: migrated to v1 Signed-off-by: MenD32 <[email protected]> fix: changed methodology to using ptr.Deref Signed-off-by: MenD32 <[email protected]> fix: changed methodology to using ptr.Deref Signed-off-by: MenD32 <[email protected]> removed allocation status check from getDeviceResultRequest Signed-off-by: MenD32 <[email protected]> fix: added handling logic for daemonsets with non-adminAccess resource claims (and also style fixes) Signed-off-by: MenD32 <[email protected]> tests: changed duplicated test name Signed-off-by: MenD32 <[email protected]> style: changed variable name to hasRegularAllocations Signed-off-by: MenD32 <[email protected]>

towca · 2025-12-09T16:02:45Z

QQ: what is the difference between squashing manually and using the tide/merge-method-squash label?

Probably not much, sorry I didn't notice the tide/squash label. You have more control over the commit message if you do it manually I guess, otherwise the default tide/squash behavior will just copy all the intermediate commit titles into the commit message.

Thanks for the changes!

/lgtm
/approve
/unhold

k8s-ci-robot · 2025-12-09T16:02:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, MenD32, towca

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster-autoscaler/OWNERS~~ [jackfrancis,towca]

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 added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 27, 2025

k8s-ci-robot requested review from vadasambar and x13n April 27, 2025 18:45

k8s-ci-robot added area/cluster-autoscaler size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 27, 2025

jackfrancis reviewed Apr 28, 2025

View reviewed changes

MenD32 commented Apr 29, 2025

View reviewed changes

MenD32 requested a review from jackfrancis April 29, 2025 10:07

MenD32 force-pushed the feat/dra-admin-access branch from 94b849e to bece2b0 Compare April 29, 2025 10:23

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 Apr 29, 2025

k8s-ci-robot assigned towca and jackfrancis Apr 29, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2025

towca reviewed Jun 4, 2025

View reviewed changes

MenD32 force-pushed the feat/dra-admin-access branch from 358338b to 4fc845c Compare June 7, 2025 20:30

MenD32 requested a review from towca June 7, 2025 20:31

MenD32 force-pushed the feat/dra-admin-access branch from 4fc845c to 38083af Compare June 7, 2025 20:54

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 3, 2025

jackfrancis reviewed Oct 3, 2025

View reviewed changes

MenD32 requested a review from jackfrancis October 3, 2025 16:56

jackfrancis reviewed Oct 3, 2025

View reviewed changes

MenD32 requested a review from jackfrancis October 3, 2025 21:25

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 6, 2025

towca reviewed Dec 2, 2025

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2025

MenD32 requested a review from towca December 2, 2025 19:45

towca reviewed Dec 5, 2025

View reviewed changes

MenD32 force-pushed the feat/dra-admin-access branch from 7046138 to f7ffeb4 Compare December 6, 2025 08:53

MenD32 requested a review from towca December 6, 2025 08:54

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 9, 2025

k8s-ci-robot merged commit c7a214c into kubernetes:master Dec 9, 2025
8 checks passed

feat: Support DRA Admin Access #8063

feat: Support DRA Admin Access #8063

Conversation

MenD32 commented Apr 27, 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?

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

Uh oh!

k8s-ci-robot commented Apr 27, 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!

jackfrancis commented Apr 29, 2025

Uh oh!

jackfrancis commented Apr 29, 2025

Uh oh!

MenD32 commented May 26, 2025

Uh oh!

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!

MenD32 Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MenD32 commented Jun 7, 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!

Choose a reason for hiding this comment

Uh oh!

MenD32 commented Oct 3, 2025

Uh oh!

jackfrancis commented Oct 6, 2025

Uh oh!

MenD32 commented Nov 3, 2025

Uh oh!

towca commented Nov 14, 2025

Uh oh!

MenD32 commented Nov 28, 2025

MenD32 commented Apr 27, 2025 •

edited

Loading

MenD32 Jul 5, 2025 •

edited

Loading