Skip to content
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

Assigning the same flavor to codependent resources #296

Merged
merged 1 commit into from
Aug 5, 2022
Merged

Assigning the same flavor to codependent resources #296

merged 1 commit into from
Aug 5, 2022

Conversation

alculquicondor
Copy link
Contributor

@alculquicondor alculquicondor commented Jul 13, 2022 •
edited

What type of PR is this?

/kind bug

What this PR does / why we need it:

When adding a ClusterQueue to the cache, identify the resources that share the same list of flavors, which we call codependent.

When admitting, iterate through flavors, checking that all the codependent resources have enough quota for the flavor.

Which issue(s) this PR fixes:

Fixes #167

Special notes for your reviewer:

This assumes that #308 is implemented.

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 13, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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:

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 k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 13, 2022
@kerthcet
Copy link
Contributor

What's this pr fixes?

@alculquicondor
Copy link
Contributor Author

Oops, wrong link. It should have been #167, but it's still WIP.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2022
@alculquicondor alculquicondor force-pushed the codep-res branch 2 times, most recently from 456c9b4 to 7b716fa Compare July 29, 2022 17:54
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2022
@alculquicondor alculquicondor force-pushed the codep-res branch 2 times, most recently from 4a90d5a to ec934c7 Compare July 29, 2022 20:09
@alculquicondor alculquicondor changed the title WIP Codependent resources Assing the same flavor to codependent resources Jul 29, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2022
@alculquicondor
Copy link
Contributor Author

@kerthcet this is ready for review :D

@alculquicondor alculquicondor mentioned this pull request Jul 29, 2022
Closed
@kerthcet
Copy link
Contributor

kerthcet commented Aug 1, 2022

Some discussions under #308 (comment)

@ahg-g ahg-g changed the title Assing the same flavor to codependent resources Assigning the same flavor to codependent resources Aug 1, 2022
ahg-g
ahg-g reviewed Aug 2, 2022
View reviewed changes
pkg/scheduler/scheduler.go
rFlavor, borrow, status := findFlavorForResource(log, resName, reqVal, wUsed[resName], resourceFlavors, cq, &e.Obj.Spec.PodSets[i].Spec)
assignedFlavors := make(map[corev1.ResourceName]string, len(podSet.Requests))
for resName := range podSet.Requests {
if assignedFlavors[resName] != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a comment explaining that this resource has been assigned a flavor while checking another resource with shared flavors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pkg/scheduler/scheduler.go Outdated
borrow, s := fitsFlavorLimits(name, val+wUsed[flavor.Name], cq, &flvLimit)
if s.IsSuccess() {
return flavor.Name, borrow, nil
fitAll := true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fitAll := true
fitsAll := true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pkg/scheduler/scheduler.go
for _, flvLimit := range cq.RequestableResources[name] {
// Since all the resources share the same flavors, they use the same selector.
selector := flavorSelector(spec, cq.LabelKeys[rName])
for i, flvLimit := range cq.RequestableResources[rName].Flavors {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we were not favoring flavors that don't cause borrowing, we just follow the flavor order in CQ; with this change, doing that will get a little more complicated, but I think still doable (like check all flavors, and return the one that causes the minimum borrowing across dependent resources).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, except that different resources might have a different scale (cpus are expressed in milli, while gpu are scalar). Maybe we can simply look it as a percentage of the total request that is being borrowed.

@alculquicondor
Assign the same flavor to resources that have the same set of flavors
262d208 
Change-Id: I495a05c1ff5044e82996842c88f05f09eaa4605e
@ahg-g
Copy link
Contributor

ahg-g commented Aug 5, 2022

/lgtm
/hold

hold just in case you are expecting more discussion on the PR, feel free to remove it.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2022
alculquicondor
alculquicondor commented Aug 5, 2022
View reviewed changes
Copy link
Contributor Author

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, forgot to send my answers

pkg/scheduler/scheduler.go
rFlavor, borrow, status := findFlavorForResource(log, resName, reqVal, wUsed[resName], resourceFlavors, cq, &e.Obj.Spec.PodSets[i].Spec)
assignedFlavors := make(map[corev1.ResourceName]string, len(podSet.Requests))
for resName := range podSet.Requests {
if assignedFlavors[resName] != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pkg/scheduler/scheduler.go
for _, flvLimit := range cq.RequestableResources[name] {
// Since all the resources share the same flavors, they use the same selector.
selector := flavorSelector(spec, cq.LabelKeys[rName])
for i, flvLimit := range cq.RequestableResources[rName].Flavors {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, except that different resources might have a different scale (cpus are expressed in milli, while gpu are scalar). Maybe we can simply look it as a percentage of the total request that is being borrowed.

pkg/scheduler/scheduler.go Outdated
borrow, s := fitsFlavorLimits(name, val+wUsed[flavor.Name], cq, &flvLimit)
if s.IsSuccess() {
return flavor.Name, borrow, nil
fitAll := true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ahg-g
Copy link
Contributor

ahg-g commented Aug 5, 2022

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2022
@k8s-ci-robot k8s-ci-robot merged commit c6c74ed into kubernetes-sigs:main Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahg-g ahg-g ahg-g left review comments

@ArangoGutierrez ArangoGutierrez Awaiting requested review from ArangoGutierrez

@denkensk denkensk Awaiting requested review from denkensk

Assignees

@ahg-g ahg-g

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using the same flavors in different resources might lead to unschedulable pods
4 participants
@alculquicondor @k8s-ci-robot @kerthcet @ahg-g