[YUNIKORN-2657] Validate queue generated as part of the placement rules #891

manirajv06 · 2024-06-13T14:31:35Z

What is this PR for?

Queue name being generated as part of the various placement rule should follow similar validation checks already being carried out for queuename coming through config.

What type of PR is it?

- Feature

Todos

- Task

What is the Jira issue?

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

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

Fix lintern issues + see comment regarding the path regexp.

pbacsko · 2024-06-13T15:09:11Z

pkg/common/configs/configvalidator.go

+
+// A queue can be a username with the dot replaced. Most systems allow a 32 character user name.
+// The queue name must thus allow for at least that length with the replacement of dots.
+var QueuePathRegExp = regexp.MustCompile(`^[a-zA-Z0-9_.:#/@-]{1,64}$`)


Fix the comment, that is a copy-paste from QueueNameRegExp.

The maximum length of 64 is a bit too low, not sure what's a reasonable limit here, I'd simply not validate the length of the path at all.

Not sure that this will even work at all...

wilfred-s · 2024-06-14T03:13:32Z

pkg/common/configs/configvalidator.go

+
+// A queue can be a username with the dot replaced. Most systems allow a 32 character user name.
+// The queue name must thus allow for at least that length with the replacement of dots.
+var QueuePathRegExp = regexp.MustCompile(`^[a-zA-Z0-9_.:#/@-]{1,64}$`)


Not sure that this will even work at all...

wilfred-s · 2024-06-14T03:14:29Z

pkg/common/configs/configvalidator.go

+	return nil
+}
+
+func IsQueuePathValid(queuePath string) error {


Not sure how this would work as we allow dots which means the length cannot be limited as each part can be 64 characters and I could have 10, 20 or more levels in a queue.

These valid queue fails this check:
root.parent_lvl1.parent_lvl2.parent_lvl3.parent_lvl4.parent_lvl5.parent_lvl6.parent_lvl7.parent_lvl8.parent_lvl9.parent_lvl10.leaf
or
root.company_name.a-long-namespace-name.wilfred_dot_spiegelenburg

Checking each part before building the path should be enough to handle this. See further comments below

Yes, length check is not required. However, I still see a need to do validate that code path because there is an option to pass the full queue path in the config. If we really want to avoid full queuePath check, we should split the string based on dot and apply queueName check itself for each part.

wilfred-s · 2024-06-14T03:14:36Z

pkg/scheduler/placement/fixed_rule.go

+		childQueueName := replaceDot(fr.queue)
+		if err = configs.IsQueueNameValid(childQueueName); err != nil {
+			return "", err
+		}
+		queueName = parentName + configs.DOT + childQueueName


This breaks a use case. I can set the fixed part to a partial path and not qualify it.
So I can set "prod.high" as the fixed part, not qualified. Use a parent rule of User to finally create a queue root.wilfred.prod.high, after this change it would create root.wilfred.prod_dot_high

The fixed part compliance for all parts of the possible unqualified path should be added to the initialise() code

Yes, we should not replace dot in this rule. https://issues.apache.org/jira/browse/YUNIKORN-2660 has been filed to make the changes like you explained.

wilfred-s · 2024-06-14T03:14:38Z

pkg/scheduler/placement/fixed_rule.go

+	if err := configs.IsQueuePathValid(queueName); err != nil {
+		return "", err
+	}


If each part is valid the path must be valid. We should have checked each part before we get to the final queue path assembly.
At this point we cannot distinguish anymore between valid and invalid . dot characters so it adds nothing.

Did not repeat the comment for every rule this was part of.

codecov · 2024-06-19T14:35:00Z

Codecov Report

Attention: Patch coverage is 95.65217% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.07%. Comparing base (5e3535c) to head (cb0e4a7).
Report is 3 commits behind head on master.

Files	Patch %	Lines
pkg/scheduler/placement/testrule.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #891      +/-   ##
==========================================
+ Coverage   78.01%   78.07%   +0.05%     
==========================================
  Files          97       97              
  Lines       12119    12150      +31     
==========================================
+ Hits         9455     9486      +31     
  Misses       2354     2354              
  Partials      310      310

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pbacsko

Some smaller things.

pkg/scheduler/placement/testrule.go

pkg/scheduler/objects/queue.go

pkg/common/configs/configvalidator.go

manirajv06 · 2024-06-20T09:40:02Z

Moved queueName checks to initialise block for fixed rule so that https://issues.apache.org/jira/browse/YUNIKORN-2660 also have been taken care of.

pkg/scheduler/objects/queue.go

pbacsko · 2024-06-20T12:06:25Z

Now it looks good, I just have a final comment left regarding coverage.

pbacsko

LGTM

wilfred-s

LGTM

pkg/common/configs/configvalidator.go

…aces

manirajv06 requested review from wilfred-s, craigcondit and pbacsko June 13, 2024 14:31

manirajv06 self-assigned this Jun 13, 2024

pbacsko requested changes Jun 13, 2024

View reviewed changes

wilfred-s requested changes Jun 14, 2024

View reviewed changes

manirajv06 added 5 commits June 19, 2024 19:53

[YUNIKORN-2657] Validate queue generated as part of the placement rules

adc7a0e

Added config changes

329d9f2

Addressed review comments

dacfb50

Addressed review comments

b58a3dd

Addressed review comments

1faac3c

manirajv06 force-pushed the YUNIKORN-2657 branch from 02eece2 to 1faac3c Compare June 19, 2024 14:32

manirajv06 requested review from wilfred-s and pbacsko June 19, 2024 14:32

pbacsko requested changes Jun 19, 2024

View reviewed changes

pkg/scheduler/placement/testrule.go Show resolved Hide resolved

pkg/scheduler/objects/queue.go Outdated Show resolved Hide resolved

pkg/scheduler/objects/queue.go Outdated Show resolved Hide resolved

pkg/common/configs/configvalidator.go Outdated Show resolved Hide resolved

Addressed review comments

c195ac7

manirajv06 requested a review from pbacsko June 20, 2024 05:26

manirajv06 added 2 commits June 20, 2024 13:02

Fixed test failures

816d3e9

Queuename checks has been placed in the beginning for fixed rule

0610e9a

fixed lint warnings

be009ab

pbacsko reviewed Jun 20, 2024

View reviewed changes

pkg/scheduler/objects/queue.go Show resolved Hide resolved

Enhanced test coverage

dc46f6e

manirajv06 requested a review from pbacsko June 21, 2024 09:34

pbacsko approved these changes Jun 21, 2024

View reviewed changes

wilfred-s approved these changes Jun 24, 2024

View reviewed changes

pkg/common/configs/configvalidator.go Outdated Show resolved Hide resolved

wilfred-s mentioned this pull request Jun 24, 2024

[YUNIKORN-2694] Improve placement rule funtion's test coverage #899

Closed

1 task

Introduced errors.go to keep error constants, used the same in few pl…

cb0e4a7

…aces

manirajv06 closed this in ff2a881 Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YUNIKORN-2657] Validate queue generated as part of the placement rules #891

[YUNIKORN-2657] Validate queue generated as part of the placement rules #891

manirajv06 commented Jun 13, 2024 •

edited

Loading

pbacsko left a comment

pbacsko Jun 13, 2024

wilfred-s Jun 14, 2024

wilfred-s Jun 14, 2024

wilfred-s Jun 14, 2024

manirajv06 Jun 18, 2024

wilfred-s Jun 14, 2024

manirajv06 Jun 18, 2024

wilfred-s Jun 14, 2024

wilfred-s Jun 14, 2024

codecov bot commented Jun 19, 2024 •

edited

Loading

pbacsko left a comment

manirajv06 commented Jun 20, 2024

pbacsko commented Jun 20, 2024

pbacsko left a comment

wilfred-s left a comment

[YUNIKORN-2657] Validate queue generated as part of the placement rules #891

[YUNIKORN-2657] Validate queue generated as part of the placement rules #891

Conversation

manirajv06 commented Jun 13, 2024 • edited Loading

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

pbacsko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2024 • edited Loading

Codecov Report

pbacsko left a comment

Choose a reason for hiding this comment

manirajv06 commented Jun 20, 2024

pbacsko commented Jun 20, 2024

pbacsko left a comment

Choose a reason for hiding this comment

wilfred-s left a comment

Choose a reason for hiding this comment

manirajv06 commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 19, 2024 •

edited

Loading