Refactor the dynamicResources struct to DynamicResources #128399

JesseStutler · 2024-10-29T03:45:43Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

I'm from volcano community. Currently, volcano wants to introduce DRA in k8s v1.31, we need to use the dynamicresources struct. Now dynamicresources is still private in master branch and can not be accessed outside the package. It needs to be refactored to DynamicResources, therefore we can access directly. I communicated with@pohly before, If I verify in volcano when there is no problem I can refactor the dynamicresources struct to public.

Which issue(s) this PR fixes:

We had a PR before and wanted to refactor this, but this person stopped following up: #124269

Special notes for your reviewer:

Our verification in volcano:
We follow up https://github.com/kubernetes-sigs/dra-example-driver to deploy an example resource driver and deploy the demo yaml gpu-test{1,2,3,4,5}.yaml, the pod can be successfully scheduled:

And allocation result in ResourceClaim is successfully updated:

Does this PR introduce a user-facing change?

The `dynamicResources` has been refactored to `DynamicResources`, now users can introduce the `DynamicResources` struct outside the `dynamicresources` package.

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

linux-foundation-easycla · 2024-10-29T03:45:46Z

The committers listed above are authorized under a signed CLA.

✅ login: JesseStutler / name: Jesse Stutler (f7003c7)

k8s-ci-robot · 2024-10-29T03:45:51Z

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 · 2024-10-29T03:45:51Z

Welcome @JesseStutler!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-10-29T03:45:52Z

Hi @JesseStutler. 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.

pohly

/lgtm

Now might be a good time to merge this, with classic DRA removed and methods thus simplified. Hopefully other pending PRs won't conflict.

@JesseStutler asked for backporting to 1.31 on Slack. I don't think we can do that because it would update an alpha feature and such backports are discouraged. OTOH, the risk is so low here, perhaps it would be acceptable?

k8s-ci-robot · 2024-10-29T06:42:29Z

LGTM label has been added.

Git tree hash: 9ee528a18c85117c1e4ebd90756c90a9417c03ac

JesseStutler · 2024-10-29T06:52:46Z

Now we are actually adapting volcano to k8s v1.31, so it will be nicer to merge it into 1.31, but if 1.31 does not have a patch version anymore, I think v1.32 also is fine. We currently make a copy of all the DRA codes of the latest master branch into volcano to make our DRA plugin.

pohly · 2024-10-29T09:34:13Z

/retest

"cluster supports count/resourceclaims.resource.k8s.io ResourceQuota" flakes occasionally

pohly · 2024-10-29T12:02:22Z

/assign @kerthcet

For approval.

kannon92 · 2024-10-29T13:43:05Z

How will the volcano community handle this with different versions of kubernetes?

I think if you are consuming this as part of a library you would want versioned APIs.

I worry that volcano is going to include a Kubernetes/Kubernetes plus our library packages. I think that may cause some confusion with dependency updates.

googs1025 · 2024-10-29T13:48:59Z

How will the volcano community handle this with different versions of kubernetes?

I think if you are consuming this as part of a library you would want versioned APIs.

I worry that volcano is going to include a Kubernetes/Kubernetes plus our library packages. I think that may cause some confusion with dependency updates.

I had communicated with @JesseStutler offline, I suggested that we (in volcano community) adapt this after DRA promoted to beta, because beta is a relatively stable version.

aojea · 2024-10-29T15:15:53Z

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

-// dynamicResources is a plugin that ensures that ResourceClaims are allocated.
-type dynamicResources struct {
+// DynamicResources is a plugin that ensures that ResourceClaims are allocated.
+type DynamicResources struct {


Kubernetes/Kubernetes is not meant to be consumed externally

Not meant to, but some things are done that way anyway:

Cluster Autoscaler

any custom scheduler which uses scheduler plugins like those from https://github.com/kubernetes-sigs/scheduler-plugins

just saying, reading the PR description it felt to me people may have some expectations that I think is important to clarify ... but looking at those links I wonder if sig-scheduler should work on moving that path to staging 🤔

Hi @aojea, thanks for your review. Not mean to directly open this PR, but I have discussed it with @pohly on slack, DRA plugin wasn't mature enough so it can't be consumed externally, but after the volcano verification was completed, he felt that it would be ok to open DRA. I saw that lots of other scheduler plugins can also be consumed externally, volcano introduces them the same way. In fact, as long as volcano can access the DRA plugin externally, it doesn’t matter which dependent package the DRA plugin is in.

sig-scheduler should work on moving that path to staging

I agree that this would be cleaner.

I wasn't involved in discussions around it, but I suppose it's not getting done because it would be a major change: it's not sufficient to just move plugins or the framework, for the "custom scheduler" use case everything related to the scheduler would have to be under staging, similar to the apiserver. That's a lot of work for something that mostly just works the way it is...

yeah, I was just curious about it, if people is happy with existing workflow and it works for everybody then it is ok

cc @alculquicondor wdyt is the preferred pattern here? It seems that the changes proposed by @JesseStutler in this PR are the more common pattern for plugins, but strictly speaking abstracting behind a framework.Plugin seems cleaner (the way the DRA plugin currently looks, which this PR is advocating we change).

According to my experience, most custom schedulers are vendoring default-scheduler for particular plugin's implementation wthin their own framework. So things would get complicated and vague if we move things to staging - where we only place generic APIs for now - it'd be like our goal is to build sth. generic for all scheduler-like components, and users would treat them as backwards-compatible API which would be a wrong assumption.

Current workflow works better IMO - for plugin itself, it'd ok to public them as a contract of SDK level of implementation instead of API. And consumers are responsible to upgrade the SDK along with k8s version upgrade.

JesseStutler · 2024-10-30T01:31:30Z

How will the volcano community handle this with different versions of kubernetes?

I think if you are consuming this as part of a library you would want versioned APIs.

I worry that volcano is going to include a Kubernetes/Kubernetes plus our library packages. I think that may cause some confusion with dependency updates.

How will the volcano community handle this with different versions of kubernetes?
I think if you are consuming this as part of a library you would want versioned APIs.
I worry that volcano is going to include a Kubernetes/Kubernetes plus our library packages. I think that may cause some confusion with dependency updates.

I had communicated with @JesseStutler offline, I suggested that we (in volcano community) adapt this after DRA promoted to beta, because beta is a relatively stable version.

If we want to upgrade dependency versions, we will upgrade kubernetes dependencies and API version dependencies together. I have discussed with @pohly before that there will be no major changes to the API, so it's fine to introduce DRA in 1.31 now. When 1.32 comes, the API upgrade will only change the version from the v1alpha3 version to the beta version, the conversion function of v1alpha3 won't be deleted in 1.32 kube-apiserver right?

We are introducing DRA now as an experimental feature to ask users for more feedback（
There will be a volcano version at the end of the year, using k8s v1.31）, so that after we upgrade k8s version to 1.32 early next year, volcano's DRA plugin will also be relatively stable after fixing some bugs.

JesseStutler · 2024-10-31T11:16:19Z

If there are no review opinions, can anyone merge it? Hope it can be carried in v1.32.

pohly · 2024-10-31T11:28:29Z

/assign @Huang-Wei

Huang-Wei · 2024-10-31T23:30:50Z

/lgtm
/approve

k8s-ci-robot · 2024-10-31T23:31:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, JesseStutler, pohly

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/scheduler/OWNERS~~ [Huang-Wei]

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

Huang-Wei · 2024-10-31T23:31:24Z

/ok-to-test

Refactor the dynamicResources struct to DynamicResources

f7003c7

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 29, 2024

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 29, 2024

k8s-ci-robot requested review from bart0sh and pohly October 29, 2024 03:46

pohly approved these changes Oct 29, 2024

View reviewed changes

k8s-ci-robot assigned pohly Oct 29, 2024

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

k8s-ci-robot assigned kerthcet Oct 29, 2024

aojea reviewed Oct 29, 2024

View reviewed changes

JesseStutler mentioned this pull request Oct 31, 2024

feature: Add dynamic resource allocation(DRA) plugin volcano-sh/volcano#3799

Open

k8s-ci-robot assigned Huang-Wei Oct 31, 2024

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

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 31, 2024

k8s-ci-robot merged commit 223ac36 into kubernetes:master Nov 1, 2024
21 checks passed

k8s-ci-robot added this to the v1.32 milestone Nov 1, 2024

pohly mentioned this pull request Nov 1, 2024

kube-scheduler: enhance performance for DRA structured parameters #127277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the dynamicResources struct to DynamicResources #128399

Refactor the dynamicResources struct to DynamicResources #128399

JesseStutler commented Oct 29, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 29, 2024 •

edited

Loading

k8s-ci-robot commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

pohly left a comment

k8s-ci-robot commented Oct 29, 2024

JesseStutler commented Oct 29, 2024

pohly commented Oct 29, 2024 •

edited

Loading

pohly commented Oct 29, 2024

kannon92 commented Oct 29, 2024

googs1025 commented Oct 29, 2024 •

edited

Loading

aojea Oct 29, 2024

pohly Oct 29, 2024

aojea Oct 29, 2024 •

edited

Loading

JesseStutler Oct 30, 2024

pohly Oct 30, 2024

aojea Oct 30, 2024

jackfrancis Oct 31, 2024

Huang-Wei Oct 31, 2024

JesseStutler commented Oct 30, 2024

JesseStutler commented Oct 31, 2024

pohly commented Oct 31, 2024

Huang-Wei commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

Huang-Wei commented Oct 31, 2024

Refactor the dynamicResources struct to DynamicResources #128399

Refactor the dynamicResources struct to DynamicResources #128399

Conversation

JesseStutler commented Oct 29, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

linux-foundation-easycla bot commented Oct 29, 2024 • edited Loading

k8s-ci-robot commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

k8s-ci-robot commented Oct 29, 2024

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 29, 2024

JesseStutler commented Oct 29, 2024

pohly commented Oct 29, 2024 • edited Loading

pohly commented Oct 29, 2024

kannon92 commented Oct 29, 2024

googs1025 commented Oct 29, 2024 • edited Loading

aojea Oct 29, 2024

Choose a reason for hiding this comment

pohly Oct 29, 2024

Choose a reason for hiding this comment

aojea Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

JesseStutler Oct 30, 2024

Choose a reason for hiding this comment

pohly Oct 30, 2024

Choose a reason for hiding this comment

aojea Oct 30, 2024

Choose a reason for hiding this comment

jackfrancis Oct 31, 2024

Choose a reason for hiding this comment

Huang-Wei Oct 31, 2024

Choose a reason for hiding this comment

JesseStutler commented Oct 30, 2024

JesseStutler commented Oct 31, 2024

pohly commented Oct 31, 2024

Huang-Wei commented Oct 31, 2024

k8s-ci-robot commented Oct 31, 2024

Huang-Wei commented Oct 31, 2024

JesseStutler commented Oct 29, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 29, 2024 •

edited

Loading

pohly commented Oct 29, 2024 •

edited

Loading

googs1025 commented Oct 29, 2024 •

edited

Loading

aojea Oct 29, 2024 •

edited

Loading