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

fix: if the capability cpu or memory is not specified in the hierarchical queue, it will be set to the corresponding value of the parent queue #3917

Merged
merged 1 commit into from
Jan 13, 2025
Merged

fix: if the capability cpu or memory is not specified in the hierarchical queue, it will be set to the corresponding value of the parent queue #3917

merged 1 commit into from
Jan 13, 2025

Conversation

JesseStutler
Copy link
Member

fix #3914

validation

  1. Set the log level of volcano-scheduler to 4
  2. Create a parent-queue, with capability CPU 100, memory 10Gi
  3. Create a child-queue, with capability memory 5Gi
  4. Check if there are any logs related with Failed to check queue's hierarchical structure

Results:
Hierarchical structure can be successfully checked in capacity plugin, without Failed to check queue's hierarchical structure log:
image

@volcano-sh-bot volcano-sh-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 24, 2024
@JesseStutler
Copy link
Member Author

/cc @Monokaix @hwdef @lowang-bh @googs1025 @archlitchi , please have a review thanks

@JesseStutler JesseStutler changed the title fix: if the capability cpu or memory is not specified in the hierarch… fix: if the capability cpu or memory is not specified in the hierarchical queue, it will be set to the corresponding value of the parent queue Dec 24, 2024
@volcano-sh-bot volcano-sh-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 25, 2024
@JesseStutler JesseStutler force-pushed the fix_3914 branch 3 times, most recently from 3ee8eda to 08dcdb4 Compare December 25, 2024 15:32
lowang-bh
lowang-bh reviewed Dec 26, 2024
View reviewed changes
pkg/scheduler/api/test_utils.go
if len(memory) > 0 {
resourceList[v1.ResourceMemory] = resource.MustParse(memory)
}

Copy link
Member

Choose a reason for hiding this comment

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

does scalarResources has same issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

The modification here is just for the convenience of using in UT without passing cpu or memory. You can see that scalarResources is a slice. If it is an empty slice or nil, resourceList will not be assigned.

@lowang-bh
Copy link
Member

/ok-to-test

hwdef
hwdef reviewed Jan 2, 2025
View reviewed changes
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 Jan 2, 2025
@JesseStutler
Copy link
Member Author

cc @Monokaix

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2025
@Monokaix
Copy link
Member

/ok-to-test
/approve

@volcano-sh-bot volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 12, 2025
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

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 Jan 12, 2025
@Monokaix
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2025
@Monokaix
Copy link
Member

/ok-to-test

@hwdef
Copy link
Member

hwdef commented Jan 13, 2025

Why is this pr blocked? @Monokaix @JesseStutler

@JesseStutler
fix: if the capability cpu or memory is not specified in the hierarch…
751d450 
…ical queue, it will be set to the corresponding value of the parent queue

Signed-off-by: JesseStutler <chenzicong4@huawei.com>
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2025
@volcano-sh-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@volcano-sh-bot volcano-sh-bot merged commit 68fba2c into volcano-sh:master Jan 13, 2025
14 checks passed
@JesseStutler
Copy link
Member Author

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k82cn k82cn Awaiting requested review from k82cn

@shinytang6 shinytang6 Awaiting requested review from shinytang6

@hwdef hwdef Awaiting requested review from hwdef

@lowang-bh lowang-bh Awaiting requested review from lowang-bh

Assignees

@hwdef hwdef

@Monokaix Monokaix

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
5 participants
@JesseStutler @lowang-bh @Monokaix @volcano-sh-bot @hwdef