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

[YUNIKORN-1568] Placement rules should be validated during config update #516

Merged
merged 2 commits into from Mar 14, 2023

Conversation

pbacsko
Copy link
Contributor

@pbacsko pbacsko commented Mar 10, 2023

What is this PR for?

Validate the placement rules on a best effort basis to see whether it refers to queues which are not parents or not available.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-1568

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@pbacsko pbacsko force-pushed the YUNIKORN-1568 branch 2 times, most recently from d9591c1 to 806a1a7 Compare March 10, 2023 18:04
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #516 (36a9ad9) into master (8c3db7f) will increase coverage by 0.15%.
The diff coverage is 93.25%.

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
+ Coverage   73.65%   73.80%   +0.15%     
==========================================
  Files          69       69              
  Lines       10437    10516      +79     
==========================================
+ Hits         7687     7761      +74     
- Misses       2504     2508       +4     
- Partials      246      247       +1     
Impacted Files Coverage Δ
pkg/scheduler/placement/fixed_rule.go 77.61% <0.00%> (ø)
pkg/common/configs/configvalidator.go 86.59% <93.67%> (+1.90%) ⬆️
pkg/scheduler/placement/provided_rule.go 80.00% <100.00%> (ø)
pkg/scheduler/placement/rule.go 93.93% <100.00%> (ø)
pkg/scheduler/placement/tag_rule.go 80.30% <100.00%> (ø)
pkg/scheduler/placement/testrule.go 100.00% <100.00%> (ø)
pkg/scheduler/placement/user_rule.go 78.18% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@0yukali0 0yukali0 left a comment

Choose a reason for hiding this comment

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

I think that adding some unit tests shows the expected results and improves the coverage.

  1. adding illegal examples in the checkACL unit tests return the queueNotParent message or the nonExistingQueue message.
  2. adding unit test of getLongestStaticPath
  3. checkQueueHierarchyForPlacement

pkg/common/configs/configvalidator.go Show resolved Hide resolved
Copy link
Contributor

@0yukali0 0yukali0 left a comment

Choose a reason for hiding this comment

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

LGTM

@0yukali0 0yukali0 merged commit 4ab9559 into apache:master Mar 14, 2023
3 checks passed
@pbacsko pbacsko deleted the YUNIKORN-1568 branch April 28, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants