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

Signed-off-by: MenD32 <[email protected]>

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.

Signed-off-by: MenD32 <[email protected]>

…s empty Signed-off-by: MenD32 <[email protected]>

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

Signed-off-by: MenD32 <[email protected]>

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?

towca · 2025-06-04T18:16:40Z

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

+		// remove AdminAccessRequests from the claim before calculating utilization
+		claims[i] = ClaimWithoutAdminAccessRequests(claim)
+	}
 	allocatedDevices, err := groupAllocatedDevices(claims)


Why not just modify groupAllocatedDevices to skip Devices with AdminAccess=true? Seems much simpler and doesn't require any copying.

This would make the code cleaner... do you think this can add unintentional side effects for function calls?

Most callsites would probably want to skip the admin-access devices because of the same reason as util calculations, but we can always add a parameter to the function.

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

Signed-off-by: MenD32 <[email protected]>

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

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

Signed-off-by: MenD32 <[email protected]>

k8s-triage-robot · 2025-10-03T15:15:20Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

MenD32 · 2025-10-03T15:20:59Z

/remove-lifecycle-stale

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

/label tide/merge-method-squash

jackfrancis · 2025-10-03T15:23:02Z

/remove-lifecycle stale

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

Signed-off-by: MenD32 <[email protected]>

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

Signed-off-by: MenD32 <[email protected]>

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

/hold

/lgtm
/approve

@towca might have some more thoughts on this

k8s-ci-robot · 2025-10-06T20:00:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

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?

feat: Support DRA Admin Access

d2d21f4

Signed-off-by: MenD32 <[email protected]>

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

MenD32 added 4 commits April 27, 2025 22:08

tests: added utilization test for adminaccess resourceclaims

e7b9ad4

Signed-off-by: MenD32 <[email protected]>

fix: ran gofmt

76ca2c9

Signed-off-by: MenD32 <[email protected]>

fix: false positives on SanitizedPodResourceClaims

05dcfc6

Signed-off-by: MenD32 <[email protected]>

fix: non admin results get removed from resourceclaim when requests i…

a389604

…s empty Signed-off-by: MenD32 <[email protected]>

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

feat: Support DRA Admin Access

bece2b0

Signed-off-by: MenD32 <[email protected]>

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

refactor: moved device filtering to groupAllocatedDevices

31237c6

Signed-off-by: MenD32 <[email protected]>

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 7, 2025

tests: added test for admin access request sanitization

38083af

Signed-off-by: MenD32 <[email protected]>

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

MenD32 added 2 commits June 11, 2025 14:11

Merge branch 'master' into feat/dra-admin-access

d846fd2

fix: removed ClaimWithoutAdminAccessRequests sanitization

63c63ee

Signed-off-by: MenD32 <[email protected]>

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

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 3, 2025

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 added 3 commits October 3, 2025 19:44

Merge branch 'master' into feat/dra-admin-access

eee7143

chore: migrated to v1

57805ff

Signed-off-by: MenD32 <[email protected]>

fix: changed methodology to using ptr.Deref

fb24953

Signed-off-by: MenD32 <[email protected]>

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

jackfrancis reviewed Oct 3, 2025

View reviewed changes

fix: changed methodology to using ptr.Deref

3a4e76a

Signed-off-by: MenD32 <[email protected]>

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

removed allocation status check from getDeviceResultRequest

141158d

Signed-off-by: MenD32 <[email protected]>

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. labels Oct 6, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2025

feat: Support DRA Admin Access #8063

Are you sure you want to change the base?

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!

Choose a reason for hiding this comment

Uh oh!

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

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 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!

k8s-triage-robot commented Oct 3, 2025

Uh oh!

MenD32 commented Oct 3, 2025

Uh oh!

jackfrancis commented Oct 3, 2025

Uh oh!

jackfrancis commented Oct 3, 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

MenD32 commented Apr 27, 2025 •

edited

Loading

MenD32 Jun 7, 2025 •

edited

Loading

MenD32 Jul 5, 2025 •

edited

Loading