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

skip 'pods' resource when checking if the Resource is empty #3223

Merged
merged 1 commit into from
Nov 25, 2023
Merged

skip 'pods' resource when checking if the Resource is empty #3223

merged 1 commit into from
Nov 25, 2023

Conversation

Lily922
Copy link
Contributor

@Lily922 Lily922 commented Nov 23, 2023 •
edited
Loading

fix issue: #3227

Design：
Skip 'pods' resource when checking if the Resource is empty.

1. "allocate" action:
   if resources.IsEmpty() :    // task with empty requsted resource:
       skip to schedule it
   else:
       try to schedule it

2. "preempt" action:
   if !task.IsEmpty():
      ignore task
   else:
      try to preempt:
          if target task.IsEmpty():
              ignore task
          else:
              add task to victims queue

3. "backfill" action:
   if resources.IsEmpty() :    // task with empty requsted resource:
       try to schedule it
   else:
       skip to schedule it
  1. Pods which request none resource except "pods" number resource will be scheduled in "backfill" action
  2. Pods request any resource including scalar resources will be scheduled in "allocate" action
  3. Pods which should be scheduled in "backfill" action can never become a preemptor or usurper of resources in "preempt" action，even if it has a higher or lower priority

@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 23, 2023
@Lily922 Lily922 mentioned this pull request Nov 24, 2023
Merged
lowang-bh
lowang-bh approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@lowang-bh lowang-bh left a comment

Choose a reason for hiding this comment

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

/LGTM

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 24, 2023
@neujie
Copy link

neujie commented Nov 25, 2023

/LGTM

@volcano-sh-bot
Copy link
Contributor

@neujie: changing LGTM is restricted to collaborators

In response to this:

/LGTM

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.

@wangyang0616
Copy link
Member

/lgtm

william-wang
william-wang reviewed Nov 25, 2023
View reviewed changes
pkg/scheduler/api/resource_info.go Outdated
// All pods request one "pods" resource now, no need to check it
var emptyCheckIgnoredScalarResources = sets.NewString(string(v1.ResourcePods))

func IsEmptyCheckIgnoredScalarResource(name v1.ResourceName) bool {
Copy link
Member

Choose a reason for hiding this comment

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

It's better to change a readable function name. For example, IsIgnoredScalarResource or IsInBlacklist

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

william-wang
william-wang reviewed Nov 25, 2023
View reviewed changes
pkg/scheduler/api/resource_info.go Outdated
func IsEmptyCheckIgnoredScalarResource(name v1.ResourceName) bool {
return emptyCheckIgnoredScalarResources.Has(string(name))
}

// IsEmpty returns false if any kind of resource is not less than min value, otherwise returns true
Copy link
Member

@william-wang william-wang Nov 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The role has changed, please update the description for this function

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

@Lily922
skip 'pods' resource when checking if the Resource is empty
96fdaa1 
Signed-off-by: lili <lili_9309@163.com>
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2023
@wangyang0616
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2023
lowang-bh
lowang-bh reviewed Nov 25, 2023
View reviewed changes
pkg/scheduler/api/resource_info.go
func (r *Resource) IsEmpty() bool {
if !(r.MilliCPU < minResource && r.Memory < minResource) {
return false
}

for _, rQuant := range r.ScalarResources {
for rName, rQuant := range r.ScalarResources {
if IsIgnoredScalarResource(rName) {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't use "ignoredScalarResources.Has" direclty to judge it? Currenty, there will be one more function calling.

william-wang
william-wang approved these changes Nov 25, 2023
View reviewed changes
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/approve

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lowang-bh, william-wang

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

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2023
@volcano-sh-bot volcano-sh-bot merged commit 01ab0cb into volcano-sh:master Nov 25, 2023
13 checks passed
@Monokaix Monokaix mentioned this pull request Dec 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lowang-bh lowang-bh lowang-bh approved these changes

@william-wang william-wang william-wang approved these changes

@Thor-wl Thor-wl Awaiting requested review from Thor-wl

Assignees

@william-wang william-wang

@lowang-bh lowang-bh

@wangyang0616 wangyang0616

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Lily922 @neujie @volcano-sh-bot @wangyang0616 @william-wang @lowang-bh