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.

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

fix Resource Less/LessEqual #484

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Conversation

wangyuqing4
Copy link
Contributor

@wangyuqing4 wangyuqing4 commented Oct 16, 2019

when i use job.yaml

apiVersion: batch.volcano.sh/v1alpha1
kind: Job
metadata:
  generateName: xxx-
spec:
  minAvailable: 1
  schedulerName: volcano
  tasks:
  - replicas: 10
    name: ta
    template:
      spec:
        containers:
        - image: "nginx:latest"
          name: mpimaster
          resources:
            requests:
              cpu: 5
              memory: 10Gi
            limits:
              cpu: 5
              memory: 10Gi

on nodes like this

apiVersion: v1
kind: Node
......
status:
  allocatable:
    cpu: "72"
    ephemeral-storage: "18903225108"
    hugepages-1Gi: "0"
    localdir: 1048600Mi
    memory: 232Gi
    pods: "110"

i use kubectl create -f job.yaml, but no pods were scheduled.

pod events:

Events:
  Type     Reason         Age                From     Message
  ----     ------         ----               ----     -------
  Warning  Unschedulable  0s (x19 over 18s)  volcano  1/10 tasks in gang unschedulable: job is not ready, 1 minAvailable, 10 Pending.


  1. i read func Less/LessEqual very confused, so i edit

  2. func Less, if map r.ScalarResources and rr.ScalarResources are all nil, i think should not compare, should according to cpu and memory.

  3. func Less, if map r.ScalarResources nil, rr.ScalarResources is not nil and value <= minMilliScalarResources, i think is not less, should return false. in other words, rr.ScalarResources should large enough if map is not nil.

e.g.
ScalarResources request of job is nil, but ScalarResources of node Allocatable is not nil(like hugepages-1Gi: "0")

in proportion plugin, line 136, if attr.request.Less(attr.deserved) == true {meet}, the deserved of queue can not be larger, the job can not be scheduled any more.
  1. func LessEqual, the same as point 3

@volcano-sh-bot volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 16, 2019
@TravisBuddy
Copy link

Travis tests have failed

Hey @wangyuqing4,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 604118b0-efec-11e9-983c-41f05746e0df

@k82cn
Copy link
Member

k82cn commented Oct 16, 2019

/lgtm
/approve

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2019
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, wangyuqing4

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 Oct 16, 2019
@TravisBuddy
Copy link

Hey @wangyuqing4,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 576518a0-eff5-11e9-983c-41f05746e0df

@@ -278,7 +278,7 @@ func TestLessEqual(t *testing.T) {
ScalarResources: map[v1.ResourceName]float64{"scalar.test/scalar1": 1},
},
resource2: &Resource{},
expected: false,
expected: true,
Copy link
Member

Choose a reason for hiding this comment

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

resource1 < resource2 in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, resource1.ScalarResources map is not nil, but value <= minMilliScalarResources, so continue, point 4

Copy link
Member

Choose a reason for hiding this comment

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

how about the others, e.g. cpu, memory? we need to make sure all resources meet LessEqual.

@k82cn
Copy link
Member

k82cn commented Oct 17, 2019

/lgtm cancel

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2019
@k82cn
Copy link
Member

k82cn commented Oct 17, 2019

/lgtm

Thanks :)

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2019
@k82cn k82cn merged commit 03b3053 into volcano-sh:master Oct 17, 2019
mateuszlitwin added a commit to mateuszlitwin/kube-batch that referenced this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants