wilfred-s commented on code in PR #891:
URL: https://github.com/apache/yunikorn-core/pull/891#discussion_r1639170903
##########
pkg/common/configs/configvalidator.go:
##########
@@ -673,6 +677,22 @@ func checkQueues(queue *QueueConfig, level int) error {
return nil
}
+func IsQueueNameValid(queueName string) error {
+ if !QueueNameRegExp.MatchString(queueName) {
+ return fmt.Errorf("invalid child name '%s', a name must only
have alphanumeric characters,"+
+ " - or _, and be no longer than 64 characters",
queueName)
+ }
+ return nil
+}
+
+func IsQueuePathValid(queuePath string) error {
Review Comment:
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
##########
pkg/common/configs/configvalidator.go:
##########
@@ -69,7 +69,11 @@ var DefaultPreemptionDelay = 30 * time.Second
// 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 QueueNameRegExp = regexp.MustCompile(`^[a-zA-Z0-9_-]{1,64}$`)
+var QueueNameRegExp = regexp.MustCompile(`^[a-zA-Z0-9_:#/@-]{1,64}$`)
+
+// 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}$`)
Review Comment:
Not sure that this will even work at all...
##########
pkg/scheduler/placement/fixed_rule.go:
##########
@@ -121,12 +121,19 @@ func (fr *fixedRule) placeApplication(app
*objects.Application, queueFn func(str
if parentName == "" {
parentName = configs.RootQueue
}
- queueName = parentName + configs.DOT + fr.queue
+ childQueueName := replaceDot(fr.queue)
+ if err = configs.IsQueueNameValid(childQueueName); err != nil {
+ return "", err
+ }
+ queueName = parentName + configs.DOT + childQueueName
Review Comment:
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
##########
pkg/scheduler/placement/fixed_rule.go:
##########
@@ -121,12 +121,19 @@ func (fr *fixedRule) placeApplication(app
*objects.Application, queueFn func(str
if parentName == "" {
parentName = configs.RootQueue
}
- queueName = parentName + configs.DOT + fr.queue
+ childQueueName := replaceDot(fr.queue)
+ if err = configs.IsQueueNameValid(childQueueName); err != nil {
+ return "", err
+ }
+ queueName = parentName + configs.DOT + childQueueName
}
// Log the result before we check the create flag
log.Log(log.SchedApplication).Debug("Fixed rule intermediate result",
zap.String("application", app.ApplicationID),
zap.String("queue", queueName))
+ if err := configs.IsQueuePathValid(queueName); err != nil {
+ return "", err
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
