Dedicated package for scheduler interaction with DRA structured types #134404

nmn3m · 2025-10-03T15:08:00Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR extends pkg/scheduler/framework/autoscaler_contract/lister_contract_test.go with a lister that uses a data structure defined in the k8s.io/dynamic-resource-allocation/structured package.

Currently, changes to that struct do not fall under SIG Autoscaling review unless explicitly requested. This makes it harder to catch issues during code review.

Why this is needed:
To ensure that changes affecting the autoscaler are visible to SIG Autoscaling reviewers and do not slip through unnoticed.

Which issue(s) this PR is related to:

Fixes #133162

Special notes for your reviewer:

One potential solution applied here is moving the relevant types into a new package:
k8s.io/dynamic-resource-allocation/structured/schedulerapi as @pohly mentioned in the issue.

An OWNERS file is set up there similar to the one under autoscaler_contract, so that the right reviewers are automatically added.

/wg device-management
/sig autoscaling

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2025-10-03T15:08:03Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2025-10-03T15:08:09Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2025-10-03T15:08:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nmn3m
Once this PR has been reviewed and has the lgtm label, please assign gjtempleton, klueska 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:

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

nmn3m · 2025-10-03T15:11:59Z

/verify-owners

jackfrancis · 2025-10-03T21:56:59Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/OWNERS

+# See the OWNERS file documentation: https://git.k8s.io/community/contributors/guide/owners.md
+
+approvers:
+  - kubernetes/sig-autoscaling-reviewers


kubernetes/sig-autoscaling-reviewers should be sig-autoscaling-maintainers

@jackfrancis, Thanks for your feedback, I fixed them.

jackfrancis · 2025-10-03T21:59:46Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/OWNERS

+
+approvers:
+  - kubernetes/sig-autoscaling-reviewers
+  - kubernetes/wg-device-management


There is no wg-device-management in any aliases definition in k/k (that I can tell). The common pattern as of now is @pohly and @klueska for WG Device Management approvers

Thanks for your feedback, I fixed them.

jackfrancis · 2025-10-03T22:00:49Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/OWNERS

+reviewers:
+  - kubernetes/sig-autoscaling-reviewers
+  - kubernetes/wg-device-management
+  - kubernetes/sig-scheduling-reviewers


kubernetes/sig-scheduling-reviewers can be sig-scheduling-maintainers or just sig-scheduling

@jackfrancis, Thanks for your feedback, I fixed them.

pohly · 2025-10-06T06:38:40Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/types_test.go

+
+// TestSchedulerContractTypes validates that all scheduler contract types
+// are properly defined and accessible.
+func TestSchedulerContractTypes(t *testing.T) {


I am not sure what exactly these tests are testing. They seem to exercise some of those types, but for what purpose? What kind of guarantee do we get out them that isn't already covered by "the production code compiles"?

pohly · 2025-10-06T06:43:24Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/types.go

+
+// DeviceID represents a unique identifier for a device in the DRA system.
+// This type is used in the scheduler and autoscaler contract.
+type DeviceID = structured.DeviceID


What we want to achieve with this package is that any code change which changes content or semantic of AllocatedState is visible as a scheduler contract change and triggers the "needs autoscaler approval" OWNERS rule.

The way how the type aliases are set up right now doesn't achieve this. Code changes can be made in k8s.io/dynamic-resource-allocation/structured without autoscaler review and this code here continues to work unchanged.

It has to be the other way around: this package here needs to have the actual struct definitions such that it is self-contained. Then k8s.io/dynamic-resource-allocation/structured can have the type aliases to avoid code churn and simplify using the types.

pohly

This is more or less what I was expecting. Let's see what others think about this approach.

pohly · 2025-10-07T18:38:48Z

pkg/scheduler/framework/autoscaler_contract/lister_contract_test.go

+	// Exercise the methods that use schedulerapi types to ensure they compile
+	_, _ = tracker.ListAllAllocatedDevices()
+	_, _ = tracker.GatherAllocatedState()
+}


I think you can remove this test.

The code exists, therefore it must compile. We don't need a separate unit test for that.

pohly · 2025-10-07T18:41:50Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/OWNERS

+# See the OWNERS file documentation: https://git.k8s.io/community/contributors/guide/owners.md
+
+approvers:
+  - sig-autoscaling-maintainers
+  - klueska
+  - pohly


Suggested change

# See the OWNERS file documentation: https://git.k8s.io/community/contributors/guide/owners.md

approvers:

- sig-autoscaling-maintainers

- klueska

- pohly

# See the OWNERS file documentation: https://git.k8s.io/community/contributors/guide/owners.md

# Disable inheritance as code changes under this package should be known to sig-autoscaling team.

options:

no_parent_owners: true

approvers:

- sig-autoscaling-maintainers

This matches https://github.com/nmn3m/kubernetes/blob/f2a1c9867a3ea5ba6368ef5718e494215446a9a0/pkg/scheduler/framework/autoscaler_contract/OWNERS and ensures that only the autoscaler maintainers can approve changes here.

Thanks got it.

pohly · 2025-10-07T18:43:31Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/types.go

+	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
+	draapi "k8s.io/dynamic-resource-allocation/api"


This is only used for UniqueString, which I think is okay to import instead of defining it here.

pohly · 2025-10-07T18:46:17Z

staging/src/k8s.io/dynamic-resource-allocation/structured/schedulerapi/types.go

+}
+
+// Clone makes a copy of ConsumedCapacity of each capacity.
+func (c ConsumedCapacityCollection) Clone() ConsumedCapacityCollection {


Pre-existing, you are just moving it, but I wonder: can we replace the manually written Clone with generated DeepCopy code?

/cc @sunya-ch

Good catch,
Yes, we can replace the manual Clone methods with generated DeepCopy code.
Would you prefer I do that in this PR, or should we keep this PR focused on moving the
types to establish the contract and handle the DeepCopy generation in a follow-up?

github-project-automation bot added this to Dynamic Resource Allocation Oct 3, 2025

github-project-automation bot moved this to 🆕 New in Dynamic Resource Allocation Oct 3, 2025

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 3, 2025

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 3, 2025

k8s-ci-robot requested review from adrianmoisey and pohly October 3, 2025 15:08

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Oct 3, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs and SIG Scheduling Oct 3, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Oct 3, 2025

k8s-ci-robot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Oct 3, 2025

nmn3m force-pushed the dra-consumable-capacity-autoscaler-contract branch from 78ddad0 to 0e923e4 Compare October 3, 2025 16:08

jackfrancis reviewed Oct 3, 2025

View reviewed changes

nmn3m force-pushed the dra-consumable-capacity-autoscaler-contract branch from 0e923e4 to 92fe2c6 Compare October 4, 2025 09:50

k8s-ci-robot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Oct 4, 2025

nmn3m requested a review from jackfrancis October 4, 2025 10:46

pohly moved this from 🆕 New to 👀 In review in Dynamic Resource Allocation Oct 6, 2025

pohly suggested changes Oct 6, 2025

View reviewed changes

github-project-automation bot moved this to Needs Review in SIG Scheduling Oct 6, 2025

github-project-automation bot moved this from Triage to Waiting on Author in SIG Node: code and documentation PRs Oct 6, 2025

nmn3m force-pushed the dra-consumable-capacity-autoscaler-contract branch from 92fe2c6 to f2a1c98 Compare October 7, 2025 17:58

pohly suggested changes Oct 7, 2025

View reviewed changes

k8s-ci-robot requested a review from sunya-ch October 7, 2025 18:47

Dedicated package for scheduler interaction with DRA structured types

bb3d60a

nmn3m force-pushed the dra-consumable-capacity-autoscaler-contract branch from f2a1c98 to bb3d60a Compare October 7, 2025 19:47

Dedicated package for scheduler interaction with DRA structured types #134404

Are you sure you want to change the base?

Dedicated package for scheduler interaction with DRA structured types #134404

Conversation

nmn3m commented Oct 3, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented Oct 3, 2025

Uh oh!

k8s-ci-robot commented Oct 3, 2025

Uh oh!

k8s-ci-robot commented Oct 3, 2025

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly left a comment

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!

Uh oh!

nmn3m commented Oct 3, 2025 •

edited

Loading