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

resource compare support only consider the requested resource item #3522

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

lowang-bh
Copy link
Member

@lowang-bh lowang-bh commented Jun 13, 2024

fixes #3520

some times, we need to only compare the required dimensions in the resource.
eg: when check Enqueueable, Allocatable or Preemptive, even overused.
Will use this function to replcae comparision in AddJobEnqueueableFn, AddPreemptiveFn and AddAllocatableFn

  • AddJobEnqueueableFn
  • AddAllocatableFn
  • AddPreemptiveFn
  • AddOverusedFn

@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 13, 2024
@lowang-bh
Copy link
Member Author

/assign @Monokaix @hwdef

Copy link
Member

@hwdef hwdef 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 Jul 10, 2024
ScalarResources: map[v1.ResourceName]float64{"nvidia.com/gpu": 1},
},
expected: false,
},
Copy link
Member

Choose a reason for hiding this comment

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

Why this return false?

Copy link
Member Author

Choose a reason for hiding this comment

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

req need gpu resource with 1, and resource2 has no gpu, so it can not satisfy the request.

// LessEqualWithDimension only compare the resource items in req param
// @param req define the resource item to be compared
// if req is nil, equals r.LessEqual(rr, zero)
func (r *Resource) LessEqualWithDimension(rr *Resource, req *Resource) bool {
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 just remove the dimensions not in req of r, then compare just like before?

Copy link
Member Author

Choose a reason for hiding this comment

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

also ok.

Copy link
Member

Choose a reason for hiding this comment

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

I think this way is more readable.

@Monokaix
Copy link
Member

Monokaix commented Aug 6, 2024

Overused should also be considered.

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2024
Copy link
Member

@hwdef hwdef 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 Aug 6, 2024
@hwdef
Copy link
Member

hwdef commented Aug 6, 2024

/approve

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hwdef

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 Aug 6, 2024
@volcano-sh-bot volcano-sh-bot merged commit 5140644 into volcano-sh:master Aug 6, 2024
14 checks passed
@hwdef
Copy link
Member

hwdef commented Aug 6, 2024

because lgtm has been added by @Monokaix

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

overcommit plugin is no need to check other resources.
4 participants