New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DRA: add default claim parameters to resource class #124251

Open

ravisantoshgudimetla wants to merge 4 commits into kubernetes:master from ravisantoshgudimetla:default-resource-classes

+336 −190

Contributor

ravisantoshgudimetla commented Apr 10, 2024 •

edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add a default parameter reference to the ResourceClass. This should be created by cluster admin while creating resource class.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add a default parameter reference to the ResourceClass. This should be created by cluster admin while creating resource class.

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

k8s-ci-robot added size/M do-not-merge/release-note-label-needed labels

Contributor

k8s-ci-robot commented Apr 10, 2024

Please note that we're already in Test Freeze for the release-1.30 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.30.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Apr 9 19:44:49 UTC 2024.

k8s-ci-robot added cncf-cla: yes do-not-merge/needs-kind do-not-merge/needs-sig needs-triage needs-priority area/code-generation kind/api-change sig/api-machinery sig/auth and removed do-not-merge/needs-kind do-not-merge/needs-sig labels

k8s-ci-robot requested review from aramase and bart0sh

April 10, 2024 01:21

k8s-ci-robot added release-note and removed do-not-merge/release-note-label-needed labels

k8s-triage-robot commented Apr 10, 2024

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

ravisantoshgudimetla force-pushed the default-resource-classes branch 2 times, most recently from 479c6d6 to a74c0dc Compare

April 10, 2024 05:43

Contributor Author

ravisantoshgudimetla commented Apr 10, 2024

Contributor

bart0sh commented Apr 10, 2024

k8s-ci-robot requested a review from pohly

April 10, 2024 15:58

Contributor

cici37 commented Apr 11, 2024

/remove-sig api-machinery

k8s-ci-robot removed the sig/api-machinery label

pohly reviewed

View reviewed changes

pkg/apis/resource/types.go Outdated

+              	// used by the driver as default when allocating a resource associated with this class.
+              	// It is the responsibility of the cluster administrator to create this default resource
+              	// claim parameter reference when defining the resource class. This field is utilized
+              	// when the ParametersRef of the resource class is nil. If both ParametersRef

Contributor

pohly Apr 12, 2024

Suggested change

      
            	// when the ParametersRef of the resource class is nil. If both ParametersRef
          
            	// only when the ParametersRef of the resource claim is nil. If both ParametersRef

pohly reviewed

View reviewed changes

Contributor

pohly left a comment

Can you include the code which uses the new field?

Contributor Author

ravisantoshgudimetla commented Apr 12, 2024 •

edited

Can you include the code which uses the new field?

You mean the scheduler changes? I'd typically put them in a separate PR to make sure that api folks can review it easily. If you want, I can add them here.

Contributor

pohly commented Apr 13, 2024

Yes. Please add here in a separate commit. We don't want to end up in a situation where an API change is merged, but then the implementation doesn't work as intended or gets delayed.


          lookupClaimParameters should use class's default claim parameter ref

15afb3a

k8s-ci-robot added size/L and removed size/M labels

Contributor

k8s-ci-robot commented Apr 15, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ravisantoshgudimetla
Once this PR has been reviewed and has the lgtm label, please assign smarterclayton for approval. For more information see the Kubernetes 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:

api/OWNERS
pkg/apis/OWNERS
pkg/generated/openapi/OWNERS
pkg/scheduler/OWNERS
staging/src/k8s.io/api/OWNERS
staging/src/k8s.io/client-go/applyconfigurations/OWNERS

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 sig/node sig/scheduling labels

ravisantoshgudimetla commented

View reviewed changes

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go

-              			},
-              		},
-              	}
+              	if claim.Spec.ParametersRef == nil && class.DefaultClaimParametersRef == nil {

Contributor Author

ravisantoshgudimetla Apr 15, 2024

@pohly - Are these 2 the only cases that we should be concerned about? Let me know if I'm missing something.

ravisantoshgudimetla commented

View reviewed changes

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go Outdated

-              		},
-              	}
+              	if claim.Spec.ParametersRef == nil && class.DefaultClaimParametersRef == nil {
+              		return nil, statusError(logger, fmt.Errorf("error allocating claim, either claim's parameter reference or default parameters for resourceclass need to be set"))

Contributor Author

ravisantoshgudimetla Apr 15, 2024

and also should we return statusError or statusUnScheduleable?

Contributor

pohly Apr 16, 2024

statusUnscheduleable - the pod is permanently unschedulable until claim or class are changed or the parameters are created. The plugin reacts to cluster events for that, so there is no need for periodic retries, which is what statusError implies.

pohly reviewed

View reviewed changes

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go Outdated

+              		return nil, statusError(logger, fmt.Errorf("error allocating claim, either claim's parameter reference or default parameters for resourceclass need to be set"))
+              	}
+              	if claim.Spec.ParametersRef == nil && class.DefaultClaimParametersRef != nil {
+              		// Get the resource class's default parameter and use it

Contributor

pohly Apr 16, 2024

Nit: missing dot at end of sentence.

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go Outdated

+              				}
+              				return nil, statusError(logger, fmt.Errorf("get claim parameters %s: %v", klog.KRef(class.Namespace, class.DefaultClaimParametersRef.Name), err))
+              			}
+              			return parameters, nil

Contributor

pohly Apr 16, 2024

This is the exact same code as below. Can you move it into a helper method?

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources.go Outdated

-              		},
-              	}
+              	if claim.Spec.ParametersRef == nil && class.DefaultClaimParametersRef == nil {
+              		return nil, statusError(logger, fmt.Errorf("error allocating claim, either claim's parameter reference or default parameters for resourceclass need to be set"))

Contributor

pohly Apr 16, 2024

statusUnscheduleable - the pod is permanently unschedulable until claim or class are changed or the parameters are created. The plugin reacts to cluster events for that, so there is no need for periodic retries, which is what statusError implies.

Contributor

pohly commented Apr 16, 2024

/retitle DRA: add default claim parameters to resource class

k8s-ci-robot changed the title ~~Default resource classes~~ DRA: add default claim parameters to resource class

SergeyKanzhelev added this to Triage in SIG Node PR Triage

Contributor

bart0sh commented Apr 17, 2024

/triage accepted
/priority important-longterm

k8s-ci-robot added triage/accepted priority/important-longterm and removed needs-triage needs-priority labels

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage


          Address reviewers comments

5af32c8

Contributor Author

ravisantoshgudimetla commented Apr 22, 2024

/test pull-kubernetes-unit

Contributor

pohly commented Apr 22, 2024

There's a known data race in the dynamicresources unit test, fix is in #124409

Contributor

pohly commented Apr 22, 2024

/retest

1 similar comment

Contributor

pohly commented Apr 22, 2024

/retest

Contributor

k8s-ci-robot commented Apr 22, 2024

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

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-unit	`5af32c8`	link	true	`/test pull-kubernetes-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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

Contributor

pohly commented Apr 22, 2024

@ravisantoshgudimetla: Ignore the retests... the unit test failure is genuine and seems related to changes in this PR.

pohly mentioned this pull request

KEP-3063 and KEP-4381: DRA: "Structured Parameters" as base, "classic" DRA as extension kubernetes/enhancements#4585

Merged

Contributor

pohly commented Apr 29, 2024

/hold

We need to decide what the desired semantic is for this new field. See kubernetes/enhancements#4585 (comment).

Once that KEP update is merged, we can update this PR accordingly (API wording, functionality).

k8s-ci-robot added the do-not-merge/hold label

Contributor Author

ravisantoshgudimetla commented May 27, 2024

@pohly - Just catching up on this after coming from break, so the consensus seems to be kubernetes/enhancements#4585 (comment), which means, there will be no allocation when neither claim not class exists, so this PR can continue?

Contributor

pohly commented May 27, 2024

There are more API changes coming: kubernetes-sigs/wg-device-management#14

We might not have a ResourceClass that gets referenced by a ResourceClaim - let's wait for that decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/code-generation cncf-cla: yes do-not-merge/hold kind/api-change priority/important-longterm release-note sig/api-machinery sig/auth sig/node sig/scheduling size/L triage/accepted