Feat: partitionable devices support

[MenD32]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds support for partitionable devices when calculating DRA utilization

Which issue(s) this PR fixes:

Fixes #8053

Special notes for your reviewer:

Does this PR introduce a user-facing change?

DRA: partitionable devices support

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

[k8s-ci-robot]

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.

[MenD32]

@towca this the same PR as #8160 which I had to close because I had some issues reverting #8539.

I'm not sure if reverting was in fact the right move, but in order to add this feature I think there is no way around it...

[jackfrancis]

@towca I had to revert #8539 in this PR because the partitionable devices feature is only available in k8s.io/api/resource/v1beta1 and has not yet been released into k8s.io/api/resource/v1

@nojnhuh can you remind me how this flywheel works?

[nojnhuh]

@towca I had to revert #8539 in this PR because the partitionable devices feature is only available in k8s.io/api/resource/v1beta1 and has not yet been released into k8s.io/api/resource/v1

@nojnhuh can you remind me how this flywheel works?

Doesn't v1 already include everything necessary for partitionable devices? https://github.com/kubernetes/kubernetes/blob/v1.34.1/staging/src/k8s.io/api/resource/v1/types.go#L157-L179

@MenD32 What was the exact issue you were running into that prompted going back to v1beta1?

[MenD32]

When I tried to merge with master I had an issue with Device.Basic.ConsumesCounters so I wrongly assumed it wasn't merged into v1 and kept under v1beta1, Now I see that they changed the Device struct to put ConsumesCounters somewhere else... I'll revert the version rollback

[towca]

@MenD32 thanks a lot for flagging this, haven't bumped into this particular issue before 😅

@jackfrancis @nojnhuh I get that we might be ok for partitionable devices specifically, but will that hold for other features? I.e. can we keep iterating on DRA KEPs in CA while only importing the v1 version of the DRA API? What if there's a KEP that requires some API changes, wouldn't that start in the next beta version? This might be especially painful because only GA APIs get enabled by default, so v1 is the only one we can "rely" on being served for 1.34+.

[nojnhuh]

My understanding is that alpha/beta features that intersect with the existing v1 APIs will be added to v1 and still feature gated, e.g. changes to DeviceClass, ResourceSlice, ResourceClaim(Template).

When a brand new API is introduced, then it will likely land in an alpha/beta API version first, e.g. DeviceTaintRules initially landing in v1alpha3 in 1.33: https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/5055-dra-device-taints-and-tolerations#:~:text=Describe%20the%20mechanism%3A%20resource.k8s.io/v1alpha3%20API%20group

1.35 is the first release cycle where we're adding features since v1 was added, so I'll keep an eye on KEP implementations for this cycle and let you know if that's not actually what happens.

[towca]

Thanks @nojnhuh, that makes sense, I didn't consider feature-gated fields in v1. And in the brand new API case we could import it and have it behind a separate flag.

[jackfrancis]

/release-note-edit

DRA: partitionable devices support

[jackfrancis]

/label tide/merge-method-squash

[MenD32]

Hi, any news regarding the review process for this PR?

[jackfrancis]

/ok-to-test

[jackfrancis]

Is there a chance that more than one resource slice from the passed in resourceSlices will have a counter CounterSet with the same name (Name property)? That would be the only reason to check for existence before initializing TotalConsumedCounters[sharedCounter.Name] = map[string]resource.Quantity{}. Also, if that's true, are we confident that they won't have any collisions with any of the names of the Counters in their Counters map[string]Counter? Otherwise we're overwriting them below.

tl;dr we may be able to simplify this and simply assign TotalConsumedCounters[sharedCounter.Name] = map[string]resource.Quantity{} without having to first check if it's already there, or if not, there may be more checks.

I did this and UT still pass:

$ git diff
diff --git a/cluster-autoscaler/simulator/dynamicresources/utils/utilization.go b/cluster-autoscaler/simulator/dynamicresources/utils/utilization.go
index c717fdfd6..98f7480a6 100644
--- a/cluster-autoscaler/simulator/dynamicresources/utils/utilization.go
+++ b/cluster-autoscaler/simulator/dynamicresources/utils/utilization.go
@@ -74,9 +74,7 @@ func calculatePoolUtil(unallocated, allocated []resourceapi.Device, resourceSlic
        TotalConsumedCounters := map[string]map[string]resource.Quantity{}
        for _, resourceSlice := range resourceSlices {
                for _, sharedCounter := range resourceSlice.Spec.SharedCounters {
-                       if _, ok := TotalConsumedCounters[sharedCounter.Name]; !ok {
-                               TotalConsumedCounters[sharedCounter.Name] = map[string]resource.Quantity{}
-                       }
+                       TotalConsumedCounters[sharedCounter.Name] = map[string]resource.Quantity{}
                        for counter, value := range sharedCounter.Counters {
                                TotalConsumedCounters[sharedCounter.Name][counter] = value.Value
                        }

[MenD32]

My impression from the KEP is that there shouldn't be any collisions in counterset names, since these are unique within a resource pool.

I got the impression that there could be a collision of the same sharedcounter from 2 different resource pools, but this would be high improbable since it'd imply that the same exact device (same device ID) appears in multiple resource pools.

Since this code is within a pool's scope, I think I'll simplify it the way you suggested

[jackfrancis]

nit: is this easier to follow?

			if totalValue.IsZero() {
				continue
			}

(rather then checking for !totalValue.IsZero() two nested iterations later)

[MenD32]

yeah, this is wayyy cleaner. I'll change it

[jackfrancis]

Can we explain how we're able to compare counter allocation (expressed in terms of resource.Quantity) w/ device allocation (expressed in terms of num devices / ints)?

[MenD32]

I think this might be a problem within the code, since if only some devices within the resource pool are non-partitionable, the correct utilization calculation would be to add the highest shared counter util with the ratio of non partitionable devices.

I think this case is also very unlikely since that'd imply that devices within the same resource pool are handled differently by the deviceClass, like partitioning only half on the GPUs in the node.

fix for that should be simple.