pkg/configmap: add upstream resource transformations support #528

sohankunkerkar · 2025-08-14T14:26:28Z

Add Resources field to KueueConfiguration to expose upstream Kueue's resource transformation feature. This allows users to configure resource exclusions and transformations (Replace/Retain strategies) via the operator CR.

openshift-ci · 2025-08-14T14:26:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sohankunkerkar

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:

~~OWNERS~~ [sohankunkerkar]

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

kannon92

/hold

The main reason why we went through API review was to not put a dependency on the kueue API in our API.

It would be worth going through an api-review for this new API.

cc @JoelSpeed @everettraven

JoelSpeed · 2025-08-14T15:37:12Z

pkg/apis/kueueoperator/v1/types.go

+	// Resource transformations allow converting PodSpec resources into Workload resource requests.
+	// This field is optional.
+	// +optional
+	Resources *configapi.Resources `json:"resources,omitempty"`


We recommend not to import types from other APIs. You cannot control the features added to the API and implicitly support anything that the upstream project adds without any form of internal review or testing.

I would suggest bringing the types into the project here directly from the upstream and then we can review them to make sure they conform to our API standards, and that we are happy supporting them in the long term.

Bear in mind that the upstream types are v1beta1 and so could also be subject to change over time, where our APIs are v1 and cannot change in the same ways

Yeah, this makes sense to me. I have created an upstream issue here: kubernetes-sigs/kueue#6582 to understand the future of that feature.

So generally we are interested in getting Kueue APIs graduated to V1. We still should not include these APIs in our own API. I think regardless of V1 promotion, we should still bring the types into this project and define them in accordance to Openshift API standards.

Kueue has a long roadmap planned for v1 API graduation (starting with sometime this year or next, they will promote the APIs to v1beta2). And then eventually they will promote to V1 but the dates for that have not yet been determined.

openshift/api#2222 (comment)

We did have some discussion about this API in the early stages of our API review.

ok, should I propose the change in the openshift enhancement first?

So the process I went through was to propose the full API in openshift/api.

The linter helps get most of the API standards and then we do the review there.

And then we copy those api changes back into this repo.

This is a bit painful and I'd prefer to maybe just have the API review done on the operator repo if possible.

I avoided having the API in the enhacement as it was being discussed in the API review anyway.

It is probably worth setting up the API linting for this repo as well

Add Resources field to KueueConfiguration to expose upstream Kueue's resource transformation feature. This allows users to configure resource exclusions and transformations (Replace/Retain strategies) via the operator CR. Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>

kannon92 · 2025-08-14T21:52:05Z

pkg/apis/kueueoperator/v1/types.go

+	// or other resources that should not be managed by Kueue.
+	// Each prefix should be a resource name prefix (e.g., "example.com/", "ephemeral-storage").
+	// +kubebuilder:validation:MaxItems=100
+	// +kubebuilder:validation:XValidation:rule="self.all(prefix, size(prefix) <= 253 && size(prefix) >= 1)",message="each excludedResourcePrefix must be between 1 and 253 characters"


I don’t think this CEL is correct. What is prefix?

In CEL, the second argument to .all() is a predicate, and the first argument is just a loop variable name for each element in the list. Here, prefix is simply the loop variable name for each string in ExcludedResourcePrefixes. If the name is causing confusion, I can change it to something more generic to make it clear it’s just a local variable.

You are likely better to create a type alias for the string and set the min/max length deliberately on the type alias, then make this list a list of the type alias rather than plain string

kannon92 · 2025-08-15T02:48:09Z

Can you add this to main actually?

openshift-ci · 2025-08-15T12:57:48Z

@sohankunkerkar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/test-e2e-4-17	`e2d8858`	link	false	`/test test-e2e-4-17`
ci/prow/lint	`e2d8858`	link	true	`/test lint`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

sohankunkerkar · 2025-08-18T13:51:25Z

@JoelSpeed could you PTAL?

kannon92 · 2025-08-18T14:00:28Z

pkg/apis/kueueoperator/v1/types.go

+	// +kubebuilder:validation:XValidation:rule="self.all(prefix, size(prefix) <= 253 && size(prefix) >= 1)",message="each excludedResourcePrefix must be between 1 and 253 characters"
+	// +listType=set
+	ExcludedResourcePrefixes []string `json:"excludedResourcePrefixes"`
+	// transformations defines a list of resource transformations that can be applied


should this be unique?

kannon92 · 2025-08-18T14:01:18Z

pkg/apis/kueueoperator/v1/types.go

+	Strategy *ResourceTransformationStrategy `json:"strategy,omitempty"`
+	// outputs defines the resources that the input resource should be transformed into.
+	// Each output specifies a resource name and quantity that should be generated per unit of input.
+	// +kubebuilder:validation:MaxItems=20


Are we sure 20 is a valid and future proof for future MIG?

https://docs.nvidia.com/datacenter/tesla/mig-user-guide/#b200-mig-profiles

I'm not entirely sure what values someone should put for this based on A100, H100 or B200.

JoelSpeed · 2025-08-19T10:17:15Z

pkg/apis/kueueoperator/v1/types.go

+	// or other resources that should not be managed by Kueue.
+	// Each prefix should be a resource name prefix (e.g., "example.com/", "ephemeral-storage").
+	// +kubebuilder:validation:MaxItems=100
+	// +kubebuilder:validation:XValidation:rule="self.all(prefix, size(prefix) <= 253 && size(prefix) >= 1)",message="each excludedResourcePrefix must be between 1 and 253 characters"


You are likely better to create a type alias for the string and set the min/max length deliberately on the type alias, then make this list a list of the type alias rather than plain string

JoelSpeed · 2025-08-19T10:17:22Z

pkg/apis/kueueoperator/v1/types.go

+	// +kubebuilder:validation:MaxItems=100
+	// +kubebuilder:validation:XValidation:rule="self.all(prefix, size(prefix) <= 253 && size(prefix) >= 1)",message="each excludedResourcePrefix must be between 1 and 253 characters"
+	// +listType=set
+	ExcludedResourcePrefixes []string `json:"excludedResourcePrefixes"`


Is this optional or required?

JoelSpeed · 2025-08-19T10:18:05Z

pkg/apis/kueueoperator/v1/types.go

+	// +kubebuilder:validation:MaxItems=100
+	// +listType=map
+	// +listMapKey=input
+	Transformations []ResourceTransformation `json:"transformations"`


Optional or required?

JoelSpeed · 2025-08-19T10:18:54Z

pkg/apis/kueueoperator/v1/types.go

+	// Resource transformations allow converting PodSpec resources into Workload resource requests.
+	// This field is optional.
+	// +optional
+	Resources Resources `json:"resources"`


API guidance has recently changed and we are generally avoiding the "discoverability" concept now. This would mean we should add omitempty to all fields. Not sure if we want to think about/talk through making that change now that this API has shipped

JoelSpeed · 2025-08-19T10:20:47Z

pkg/apis/kueueoperator/v1/types.go

+	// +kubebuilder:validation:Enum=Retain;Replace
+	// +kubebuilder:default=Retain
+	// +optional
+	Strategy *ResourceTransformationStrategy `json:"strategy,omitempty"`


Why a pointer? "" isn't a valid value right?

JoelSpeed · 2025-08-19T10:21:47Z

pkg/apis/kueueoperator/v1/types.go

+	// Must be a valid Kubernetes resource quantity (e.g., "1", "100m", "1Gi").
+	// +kubebuilder:validation:Pattern=`^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$`
+	// +required
+	Quantity string `json:"quantity"`


What version of OCP will this change run on? CEL has the option to validate resource quantity style fields for you, but only really working from 4.20 onwards

JoelSpeed · 2025-08-19T10:22:31Z

pkg/apis/kueueoperator/v1/types.go

+	// +kubebuilder:validation:MaxLength=253
+	// +kubebuilder:validation:MinLength=1
+	// +required
+	Resource string `json:"resource"`


Apart from length, what other validation can we add here (e.g. valid characters as a regex)

kannon92 · 2025-08-19T13:25:26Z

@JoelSpeed
Thank you for your review so far.

We met yesterday about this API and we realize that we may want to avoid exposing this API atm.

The configuration of this is complicated and we realize that the only reason why Kueue admins would need to configure this is for batch jobs. For serving workloads, a pure DAS webhook would actually serve most of our targeted users for DAS.

So I think we may table this API while we wait for more clear use cases on batch + DAS integration.

openshift-ci bot requested review from MaysaMacedo and PannagaRao August 14, 2025 14:26

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2025

kannon92 reviewed Aug 14, 2025

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2025

JoelSpeed reviewed Aug 14, 2025

View reviewed changes

sohankunkerkar force-pushed the configure-resource-upstream branch from 2d81c9c to e2d8858 Compare August 14, 2025 20:22

kannon92 reviewed Aug 14, 2025

View reviewed changes

sohankunkerkar changed the base branch from release-1.1 to main August 15, 2025 12:08

kannon92 reviewed Aug 18, 2025

View reviewed changes

JoelSpeed reviewed Aug 19, 2025

View reviewed changes

pkg/configmap: add upstream resource transformations support #528

Are you sure you want to change the base?

pkg/configmap: add upstream resource transformations support #528

Uh oh!

Conversation

sohankunkerkar commented Aug 14, 2025

Uh oh!

openshift-ci bot commented Aug 14, 2025

Uh oh!

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

sohankunkerkar Aug 14, 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!

kannon92 commented Aug 15, 2025

Uh oh!

openshift-ci bot commented Aug 15, 2025

Uh oh!

sohankunkerkar commented Aug 18, 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!

kannon92 commented Aug 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sohankunkerkar Aug 14, 2025 •

edited

Loading