dra scheduler: ensure that we never have nil claim/class parameters #123828

klueska · 2024-03-08T23:41:05Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR adds logic to define default claim/class parameters that will allow
claim allocation to proceed even if an end user doesn't provide claim
parameters themselves.

Without this change, the scheduler was crashing in newClaimController() in pkg/scheduler/framework/plugins/dynamicresources/structuredparameters.go

The code in newClaimController() assumes that claim parameters are not nil.
Furthermore it assumes that there is at least one DriverRequest populated in
order to allocate any resources to a claim.

However, we shouldn't force a user to supply a ResourceClaimParameters
object if they don't want to. We also shouldn't force a driver implementor to
write a controller just to associate a static ResourceClaimParameters object
with every claim.

In this PR, the "default" claim parameters objects are hard-coded in the
scheduler code. This won't work going forward as we add new structured
models though. What we probably want is a field in the ResourceClass which
points to a default ResourceClaimPrameters object that should be used if one
is not supplied as part of the claim.

None

k8s-ci-robot · 2024-03-08T23:41:14Z

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/test-infra repository.

klueska · 2024-03-08T23:41:29Z

/assign @pohly

klueska · 2024-03-09T00:01:32Z

I'm realizing this is not sufficient. I think we need a fully populated "default" claim parameters object that requests a single resource with no configuration parameters.

klueska · 2024-03-09T07:25:08Z

I have currently hard-coded a "default" claim parameters object in the scheduler code. This won't work going forward as we add new structured models though. What we probably want is a field in the ResourceClass which points to a default ResourceClaimPrameters object that should be used if one is not supplied as part of the claim.

pohly · 2024-03-09T17:02:20Z

This PR adds logic to define default claim/class parameters that will allow
claim allocation to proceed even if an end user doesn't provide claim
parameters themselves.

I wasn't sure whether we want that. The E2E tests use selector: "true", just as in your default. But you are right, this is unnecessarily complex for users. And of course it shouldn't crash... 😅

What we probably want is a field in the ResourceClass which points to a default ResourceClaimPrameters object that should be used if one is not supplied as part of the claim.

... because without claim parameters, the scheduler doesn't know the claim is for a resource using model "named resources" or some future "some-other-resources". Makes sense.

As a stop-gap solution this is okay.

/lgtm
/assign @alculquicondor

For approval.

k8s-ci-robot · 2024-03-09T17:02:25Z

LGTM label has been added.

Git tree hash: da955e7c2a17de521426b6f9513850323e009ed1

ravisantoshgudimetla · 2024-03-09T21:12:41Z

I'm looking into implementing a custom driver and noticed this issue. I like the defaulting idea, if you folks are fine with it, I can take care of it.

pohly · 2024-03-11T07:02:19Z

@klueska: we now have the second commit from #123831 also in this PR. Can you take it out again?

klueska · 2024-03-11T07:04:02Z

I reversed the order so that the other one can merge without further approval. It just needs a new LGTM (and to be added to the milestone of course).

pohly · 2024-03-11T07:15:31Z

@ravisantoshgudimetla: let's discuss in #123858.

Previously we were returning the error string from 'err' (which is nil), when we should have been returning it from result.Error. Without this it is hard to debug issues with NodeUnprepareResources. Signed-off-by: Kevin Klues <kklues@nvidia.com>

alculquicondor · 2024-03-11T13:55:10Z

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

+		for _, f := range classParameters.Filters {
+			if f.DriverName == driverName && f.ResourceFilterModel.NamedResources != nil {
+				filter = f.ResourceFilterModel.NamedResources
+				break
 			}
 		}
 		controller, err := namedresourcesmodel.NewClaimController(filter, perDriver.requests)


Was the existing behavior equivalent in this case?

It looks the same to me. classParameters is now guaranteed to not be nil (a case that I had considered, in contrast to nil claim parameters), so the if check is not needed anymore. If it had been nil before, it's now empty, so the for loop doesn't do anything, just as before.

Without this, the scheduler was crashing in newClaimController() in pkg/scheduler/framework/plugins/dynamicresources/structuredparameters.go The code in newClaimController() assumes that the parameters are not nil. Furthermore it assumes that there is at least one DriverRequest populated in order to allocate any resources to a claim. This PR adds logic to define default claim/class parameters that will allow allocation to proceed even if an end user doesn't provide any class or claim parameters themselves. Signed-off-by: Kevin Klues <kklues@nvidia.com>

alculquicondor · 2024-03-11T13:57:25Z

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

+							// match the default to how the resources of this
+							// class are being advertized in a ResourceSlice.
+							NamedResources: &resourcev1alpha2.NamedResourcesRequest{
+								Selector: "true",


I don't like that we would have to parse this selector. I would imagine that a case of "no parameters" is fairly common, so we should optimize for by understanding nil.

The problem is that the scheduler doesn't inherently know which resource request model a driver is using to advertise its resources. We only know this by parsing this object and seeing which field is set (in this case NamedResources).

As such, I think a better approach would be to allow a cluster admin to define a default ResourceClaimParameters object and link it to a resource class. However, this is a larger chunk of work (and we are past the code freeze), so I proposed adding this PR as a stop-gap for 1.30.

The problem is that the scheduler doesn't inherently know which resource request model a driver is using.

Does it matter? The point is that it passes the "filter", regardless of the model.

I'll let @pohly comment on that as I'm not intimately familiar with all of the code in this plugin. All I know is some default should be set and the default should allow sharing. Whether that's through an explicit object or a branch off of detecting nil I'll leave up to Patrick.

I like the approach in this PR better because the rest of the code currently expects to determine the model and thus the resources based on the parameters. Making it handle nil adds more special cases.

There's also a clear path towards what we really want (configurable default parameters). We need those because detecting the model may not be enough, we also need to figure out what the filters for a claim with no parameters should be. This depends on the driver and/or cluster, not the model.

#123858 is tracking this next step.

alculquicondor · 2024-03-11T20:19:47Z

/lgtm
/approve
Please have a test case for this before the freeze

k8s-ci-robot · 2024-03-11T20:19:51Z

LGTM label has been added.

Git tree hash: a55bff3202752fa89898e36a5714630b01d5ba17

k8s-ci-robot · 2024-03-11T20:19:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, klueska

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:

~~pkg/kubelet/cm/OWNERS~~ [klueska]
~~pkg/scheduler/OWNERS~~ [alculquicondor]

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

pohly · 2024-03-11T20:21:52Z

I am adding an E2E test for this. Let's prioritize adding the fix first.

pohly · 2024-03-11T20:24:10Z

/cc @kubernetes/release-team

Another bug fix for a new feature in 1.30, please add the milestone.

k8s-ci-robot · 2024-03-11T20:24:13Z

@pohly: GitHub didn't allow me to request PR reviews from the following users: kubernetes/release-team.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @kubernetes/release-team

Another bug fix for a new feature in 1.30, please add the milestone.

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.

katcosgrove · 2024-03-11T20:26:16Z

/milestone v1.30

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 8, 2024

k8s-ci-robot assigned pohly Mar 8, 2024

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. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 8, 2024

k8s-ci-robot requested review from bart0sh and pohly March 8, 2024 23:42

klueska force-pushed the non-nil-parameters branch from e976d81 to 917fa21 Compare March 9, 2024 00:39

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 9, 2024

klueska force-pushed the non-nil-parameters branch 2 times, most recently from 603f049 to e69c531 Compare March 9, 2024 01:24

klueska changed the title ~~dra scheduler: ensure that we never have nil Claim/Class parameters~~ dra scheduler: ensure that we never have nil claim/class parameters Mar 9, 2024

k8s-ci-robot assigned alculquicondor Mar 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2024

klueska force-pushed the non-nil-parameters branch from e69c531 to d067793 Compare March 9, 2024 18:09

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2024

k8s-ci-robot requested a review from alculquicondor March 9, 2024 18:09

k8s-ci-robot added the area/kubelet label Mar 9, 2024

klueska mentioned this pull request Mar 9, 2024

Add StructuredResourceModel to UnprepareResources call #123831

Merged

pohly mentioned this pull request Mar 11, 2024

DRA: structured parameters: handling of claim without claim parameters #123858

Open

dra kubelet: fix error log

fc2134c

Previously we were returning the error string from 'err' (which is nil), when we should have been returning it from result.Error. Without this it is hard to debug issues with NodeUnprepareResources. Signed-off-by: Kevin Klues <kklues@nvidia.com>

alculquicondor reviewed Mar 11, 2024

View reviewed changes

klueska force-pushed the non-nil-parameters branch from d067793 to 538b7ab Compare March 11, 2024 13:56

alculquicondor reviewed Mar 11, 2024

View reviewed changes

klueska force-pushed the non-nil-parameters branch from 538b7ab to 21a0dd1 Compare March 11, 2024 13:57

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2024

k8s-ci-robot added this to the v1.30 milestone Mar 11, 2024

k8s-ci-robot merged commit 3ec6a38 into kubernetes:master Mar 11, 2024
17 checks passed

bart0sh added this to Triage in SIG Node PR Triage Mar 11, 2024

bart0sh moved this from Triage to Done in SIG Node PR Triage Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dra scheduler: ensure that we never have nil claim/class parameters #123828

dra scheduler: ensure that we never have nil claim/class parameters #123828

klueska commented Mar 8, 2024 •

edited by saschagrunert

k8s-ci-robot commented Mar 8, 2024

klueska commented Mar 8, 2024

klueska commented Mar 9, 2024

klueska commented Mar 9, 2024

pohly commented Mar 9, 2024

k8s-ci-robot commented Mar 9, 2024

ravisantoshgudimetla commented Mar 9, 2024 •

edited

pohly commented Mar 11, 2024

klueska commented Mar 11, 2024 •

edited

pohly commented Mar 11, 2024

alculquicondor Mar 11, 2024

pohly Mar 11, 2024

alculquicondor Mar 11, 2024

klueska Mar 11, 2024

alculquicondor Mar 11, 2024

klueska Mar 11, 2024

pohly Mar 11, 2024 •

edited

alculquicondor commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

pohly commented Mar 11, 2024

pohly commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

katcosgrove commented Mar 11, 2024

dra scheduler: ensure that we never have nil claim/class parameters #123828

dra scheduler: ensure that we never have nil claim/class parameters #123828

Conversation

klueska commented Mar 8, 2024 • edited by saschagrunert

What type of PR is this?

What this PR does / why we need it:

k8s-ci-robot commented Mar 8, 2024

klueska commented Mar 8, 2024

klueska commented Mar 9, 2024

klueska commented Mar 9, 2024

pohly commented Mar 9, 2024

k8s-ci-robot commented Mar 9, 2024

ravisantoshgudimetla commented Mar 9, 2024 • edited

pohly commented Mar 11, 2024

klueska commented Mar 11, 2024 • edited

pohly commented Mar 11, 2024

alculquicondor Mar 11, 2024

Choose a reason for hiding this comment

pohly Mar 11, 2024

Choose a reason for hiding this comment

alculquicondor Mar 11, 2024

Choose a reason for hiding this comment

klueska Mar 11, 2024

Choose a reason for hiding this comment

alculquicondor Mar 11, 2024

Choose a reason for hiding this comment

klueska Mar 11, 2024

Choose a reason for hiding this comment

pohly Mar 11, 2024 • edited

Choose a reason for hiding this comment

alculquicondor commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

pohly commented Mar 11, 2024

pohly commented Mar 11, 2024

k8s-ci-robot commented Mar 11, 2024

katcosgrove commented Mar 11, 2024

klueska commented Mar 8, 2024 •

edited by saschagrunert

ravisantoshgudimetla commented Mar 9, 2024 •

edited

klueska commented Mar 11, 2024 •

edited

pohly Mar 11, 2024 •

edited