This is an automated email from the ASF dual-hosted git repository.
pbacsko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/yunikorn-core.git
The following commit(s) were added to refs/heads/master by this push:
new d1589f9d [YUNIKORN-1816] Fixed placement rule no longer works due to
incorrect validation (#572)
d1589f9d is described below
commit d1589f9d9032021e9aa545d91f6c211d8366abbc
Author: Peter Bacsko <[email protected]>
AuthorDate: Thu Jun 22 16:46:08 2023 +0200
[YUNIKORN-1816] Fixed placement rule no longer works due to incorrect
validation (#572)
Closes: #572
Signed-off-by: Peter Bacsko <[email protected]>
---
pkg/common/configs/config_test.go | 5 +-
pkg/common/configs/configvalidator.go | 173 +++++++++++++++++++----------
pkg/common/configs/configvalidator_test.go | 139 ++++++++++++++++++++---
3 files changed, 243 insertions(+), 74 deletions(-)
diff --git a/pkg/common/configs/config_test.go
b/pkg/common/configs/config_test.go
index 7261a886..ca67a47a 100644
--- a/pkg/common/configs/config_test.go
+++ b/pkg/common/configs/config_test.go
@@ -732,9 +732,12 @@ partitions:
- name: default
queues:
- name: root
+ queues:
+ - name: default
+ parent: false
placementrules:
- name: fixed
- value: root
+ value: root.default
- name: tag
value: Just Any value
`
diff --git a/pkg/common/configs/configvalidator.go
b/pkg/common/configs/configvalidator.go
index 1652b0b3..ab7dc550 100644
--- a/pkg/common/configs/configvalidator.go
+++ b/pkg/common/configs/configvalidator.go
@@ -53,8 +53,16 @@ const (
// app sort priority values
ApplicationSortPriorityEnabled = "enabled"
ApplicationSortPriorityDisabled = "disabled"
+
+ // placement rule validation
+ placementOK placementPathCheckResult = iota
+ errNonExistingQueue
+ errQueueNotLeaf
+ errLastQueueLeaf
)
+type placementPathCheckResult int
+
// Priority
var MinPriority int32 = math.MinInt32
var MaxPriority int32 = math.MaxInt32
@@ -79,20 +87,13 @@ var SpecialRegExp = regexp.MustCompile(`[\^$*+?()\[{}|]`)
var RuleNameRegExp = regexp.MustCompile(`^[_a-zA-Z][a-zA-Z0-9_]*$`)
type placementStaticPath struct {
- path string
- ruleChain string
- create bool
- ruleNo int
+ path string
+ ruleChain string
+ create bool
+ hasDynamicPart bool
+ ruleNo int
}
-type placementPathCheckResult int
-
-const (
- checkOK placementPathCheckResult = iota
- nonExistingQueue
- queueNotParent
-)
-
// Check the ACL
func checkACL(acl string) error {
// trim any white space
@@ -295,33 +296,52 @@ func checkPlacementRules(partition *PartitionConfig)
error {
}
}
- placementStaticPaths :=
getLongestPlacementPaths(partition.PlacementRules)
+ placementStaticPaths, err :=
getLongestPlacementPaths(partition.PlacementRules)
+ if err != nil {
+ return err
+ }
for _, staticPath := range placementStaticPaths {
queuePath := staticPath.path
+ create := staticPath.create
+ hasDynamicPart := staticPath.hasDynamicPart
+
parts := strings.Split(strings.ToLower(queuePath), DOT)
- result := checkQueueHierarchyForPlacement(parts,
staticPath.create, partition.Queues)
- if result == queueNotParent {
- return fmt.Errorf("placement rule no. #%d (%s)
references a queue (%s) which is a leaf",
+ result, lastQueue := checkQueueHierarchyForPlacement(parts,
create, hasDynamicPart, partition.Queues, nil)
+ if result == errQueueNotLeaf {
+ return fmt.Errorf("placement rule no. #%d (%s)
references a queue (%s) which is not a leaf",
staticPath.ruleNo, staticPath.ruleChain,
queuePath)
}
- if result == nonExistingQueue {
+ if result == errNonExistingQueue {
return fmt.Errorf("placement rule no. #%d (%s)
references non-existing queues (%s) and create is 'false'",
staticPath.ruleNo, staticPath.ruleChain,
queuePath)
}
+ if result == errLastQueueLeaf {
+ return fmt.Errorf("placement rule no. #%d (%s)
references non-existing queues (%s) which cannot be created because the last
queue (%s) in the hierarchy is a leaf",
+ staticPath.ruleNo, staticPath.ruleChain,
queuePath, lastQueue)
+ }
}
return nil
}
-func checkQueueHierarchyForPlacement(path []string, create bool, conf
[]QueueConfig) placementPathCheckResult {
+func checkQueueHierarchyForPlacement(path []string, create, hasDynamicPart
bool, conf []QueueConfig, parentConf *QueueConfig) (result
placementPathCheckResult, lastQueueName string) {
queueName := path[0]
+ lastQueueName = ""
// no more queues in the configuration
if len(conf) == 0 {
+ if !parentConf.Parent {
+ // path in the hierarchy is shorter, but the last queue
is a leaf
+ result = errLastQueueLeaf
+ lastQueueName = parentConf.Name
+ return
+ }
if !create {
- return nonExistingQueue
+ result = errNonExistingQueue
+ return
}
- return checkOK
+ result = placementOK
+ return
}
var queueConf *QueueConfig
@@ -336,21 +356,34 @@ func checkQueueHierarchyForPlacement(path []string,
create bool, conf []QueueCon
// queue not found on this level
if queueConf == nil {
if !create {
- return nonExistingQueue
+ result = errNonExistingQueue
+ return
}
- return checkOK
- }
-
- if !queueConf.Parent {
- return queueNotParent
+ result = placementOK
+ return
}
if len(path) == 1 {
- return checkOK
+ if hasDynamicPart {
+ // the "fixed" rule is followed by other rules like
tag, user, etc. (root.dev.<user>),
+ // which means that the "fixed" part must point to a
parent
+ if queueConf.Parent {
+ result = placementOK
+ return
+ }
+ result = errQueueNotLeaf
+ return
+ }
+ if queueConf.Parent {
+ result = errQueueNotLeaf
+ return
+ }
+ result = placementOK
+ return
}
path = path[1:]
- return checkQueueHierarchyForPlacement(path, create, queueConf.Queues)
+ return checkQueueHierarchyForPlacement(path, create, hasDynamicPart,
queueConf.Queues, queueConf)
}
// Check the specific rule for syntax.
@@ -738,51 +771,79 @@ func Validate(newConfig *SchedulerConfig) error {
}
// returns the longest fixed queue path defined by the placement rule chain
-// e.g. the chain is user->tag->fixed, returns something like
"root.users.<tag>.<user>",
+// e.g. the chain is fixed->tag->user, returns something like
"root.users.<tag>.<user>",
// the longest static part is "root.users"
-func getLongestPlacementPaths(rules []PlacementRule) []placementStaticPath {
+func getLongestPlacementPaths(rules []PlacementRule) ([]placementStaticPath,
error) {
paths := make([]placementStaticPath, 0)
for i, rule := range rules {
- path, ruleChain, _ := getLongestStaticPath(rule)
+ path, ruleChain, hasDynamicPart, err :=
getLongestStaticPath(rule)
+ if err != nil {
+ return nil, err
+ }
+ if strings.Index(path, RootQueue) != 0 {
+ continue
+ }
placementPath := placementStaticPath{
- path: path,
- create: rule.Create,
- ruleChain: ruleChain,
- ruleNo: i,
+ path: path,
+ create: rule.Create,
+ ruleChain: ruleChain,
+ hasDynamicPart: hasDynamicPart,
+ ruleNo: i,
}
paths = append(paths, placementPath)
}
- return paths
+ return paths, nil
}
-func getLongestStaticPath(rule PlacementRule) (string, string, bool) {
- var parentPath string
- var ruleChain string
- dynamicParent := false
+func getLongestStaticPath(rule PlacementRule) (staticPath, ruleChain string,
foundDynamicRule bool, err error) {
+ rules := getRuleChain(rule)
- if rule.Parent != nil {
- var chain string
- parentPath, chain, dynamicParent =
getLongestStaticPath(*rule.Parent)
- ruleChain = rule.Name + "->" + chain
- } else {
- ruleChain = rule.Name
- parentPath = RootQueue
- }
+ for _, r := range rules {
+ if ruleChain == "" {
+ ruleChain = r.Name
+ } else {
+ ruleChain = ruleChain + "->" + r.Name
+ }
+ if foundDynamicRule {
+ continue
+ }
- if rule.Name == types.Fixed {
- queueName := strings.ToLower(rule.Value)
+ if r.Name != types.Fixed {
+ if staticPath == "" {
+ staticPath = "<dynamic>"
+ }
+ foundDynamicRule = true
+ continue
+ }
+
+ queueName := r.Value
qualified := strings.HasPrefix(queueName, RootQueue)
if qualified {
- return queueName, ruleChain, false
+ if staticPath != "" {
+ // error, only the first fixed rule can be
fully qualified
+ err = fmt.Errorf("illegal fully qualified
'fixed' rule with value %s", queueName)
+ return staticPath, ruleChain, foundDynamicRule,
err
+ }
+ staticPath = queueName
+ continue
}
- // there is a parent rule other than "fixed", we can't do
anything about that
- if dynamicParent {
- return RootQueue, ruleChain, true
+ if staticPath == "" {
+ staticPath = RootQueue
}
- return parentPath + "." + queueName, ruleChain, false
+ staticPath = staticPath + "." + queueName
+ }
+
+ return staticPath, ruleChain, foundDynamicRule, nil
+}
+
+func getRuleChain(r PlacementRule) []PlacementRule {
+ rules := make([]PlacementRule, 0)
+ if r.Parent != nil {
+ rules = append(rules, getRuleChain(*r.Parent)...)
}
- return parentPath, ruleChain, true
+ rules = append(rules, r)
+ return rules
}
diff --git a/pkg/common/configs/configvalidator_test.go
b/pkg/common/configs/configvalidator_test.go
index 6c6b82b3..9b871899 100644
--- a/pkg/common/configs/configvalidator_test.go
+++ b/pkg/common/configs/configvalidator_test.go
@@ -170,38 +170,97 @@ func TestCheckQueueMaxApplicationsForQueue(t *testing.T) {
}
func TestGetLongestPlacementPath(t *testing.T) {
- staticPaths := getLongestPlacementPaths(createPlacementRules())
+ staticPaths, err := getLongestPlacementPaths(createPlacementRules())
+ assert.NilError(t, err)
assert.Equal(t, 2, len(staticPaths))
path0 := staticPaths[0]
assert.Equal(t, "root.users", path0.path)
assert.Equal(t, 0, path0.ruleNo)
- assert.Equal(t, "user->tag->fixed", path0.ruleChain)
+ assert.Equal(t, "fixed->tag->user", path0.ruleChain)
assert.Equal(t, false, path0.create)
path1 := staticPaths[1]
assert.Equal(t, "root.admins.dev", path1.path)
assert.Equal(t, 1, path1.ruleNo)
assert.Equal(t, "fixed->fixed", path1.ruleChain)
assert.Equal(t, true, path1.create)
+
+ // illegal: two "fixed" with fully qualified path
+ illegal := []PlacementRule{
+ {
+ Name: "fixed",
+ Value: "root.dev",
+ Create: true,
+ Parent: &PlacementRule{
+ Name: "fixed",
+ Value: "root.admins",
+ },
+ },
+ }
+ _, err = getLongestPlacementPaths(illegal)
+ assert.ErrorContains(t, err, "illegal fully qualified 'fixed' rule")
}
func TestCheckQueueHierarchyForPlacement(t *testing.T) {
queues := createQueueConfig()
- parts := strings.Split(strings.ToLower("root.users"), DOT)
- result := checkQueueHierarchyForPlacement(parts, false, queues)
- assert.Equal(t, checkOK, result)
-
- parts = strings.Split(strings.ToLower("root.users.dev"), DOT)
- result = checkQueueHierarchyForPlacement(parts, true, queues)
- assert.Equal(t, checkOK, result)
+ // case #1 - referring to existing queue which is a parent
+ parts := strings.Split("root.users", DOT)
+ result, queueName := checkQueueHierarchyForPlacement(parts, false,
false, queues, nil)
+ assert.Equal(t, errQueueNotLeaf, result)
+ assert.Equal(t, "", queueName)
+
+ // case #2 - referring to an existing queue which is a leaf
+ parts = strings.Split("root.default", DOT)
+ result, queueName = checkQueueHierarchyForPlacement(parts, true, false,
queues, nil)
+ assert.Equal(t, placementOK, result)
+ assert.Equal(t, "", queueName)
+
+ // case #3 - referring a path which is incomplete in the hierarchy,
"users" is parent, create = true
+ parts = strings.Split("root.users.alice", DOT)
+ result, queueName = checkQueueHierarchyForPlacement(parts, true, false,
queues, nil)
+ assert.Equal(t, placementOK, result)
+ assert.Equal(t, "", queueName)
+
+ // case #4 - referring a path which is incomplete in the hierarchy,
"users" is parent, create = false
+ result, queueName = checkQueueHierarchyForPlacement(parts, false,
false, queues, nil)
+ assert.Equal(t, errNonExistingQueue, result)
+ assert.Equal(t, "", queueName)
+
+ // case #5 - referring a path which is incomplete in the hierarchy,
"users" is leaf, create = true
queues[0].Queues[0].Parent = false
- result = checkQueueHierarchyForPlacement(parts, false, queues)
- assert.Equal(t, queueNotParent, result)
-
+ result, queueName = checkQueueHierarchyForPlacement(parts, true, false,
queues, nil)
+ assert.Equal(t, errLastQueueLeaf, result)
+ assert.Equal(t, "users", queueName)
+
+ // case #6 - referring a path which is incomplete in the hierarchy,
"users" is leaf, create = false
+ result, queueName = checkQueueHierarchyForPlacement(parts, false,
false, queues, nil)
+ assert.Equal(t, errLastQueueLeaf, result)
+ assert.Equal(t, "users", queueName)
+
+ // case #7 - hierarchy is long enough, but no matching queue found,
create = true
+ parts = strings.Split("root.devs.test", DOT)
+ result, queueName = checkQueueHierarchyForPlacement(parts, true, false,
queues, nil)
+ assert.Equal(t, placementOK, result)
+ assert.Equal(t, "", queueName)
+
+ // case #8 - hierarchy is long enough, but no matching queue found,
create = false
+ result, queueName = checkQueueHierarchyForPlacement(parts, false,
false, queues, nil)
+ assert.Equal(t, errNonExistingQueue, result)
+ assert.Equal(t, "", queueName)
+
+ // case #9 - rule chain ends with a dynamic part, last queue is a leaf
+ parts = strings.Split("root.users", DOT)
+ result, queueName = checkQueueHierarchyForPlacement(parts, false, true,
queues, nil)
+ assert.Equal(t, errQueueNotLeaf, result)
+ assert.Equal(t, "", queueName)
+
+ // case #10 - rule chain ends with a dynamic part, last queue is a
parent
queues[0].Queues[0].Parent = true
- result = checkQueueHierarchyForPlacement(parts, false, queues)
- assert.Equal(t, nonExistingQueue, result)
+ parts = strings.Split("root.users", DOT)
+ result, queueName = checkQueueHierarchyForPlacement(parts, false, true,
queues, nil)
+ assert.Equal(t, placementOK, result)
+ assert.Equal(t, "", queueName)
}
func TestCheckPlacementRules(t *testing.T) {
@@ -210,17 +269,49 @@ func TestCheckPlacementRules(t *testing.T) {
Queues: createQueueConfig(),
}
+ // default case, no error
err := checkPlacementRules(conf)
assert.NilError(t, err)
- conf.Queues[0].Queues[0].Parent = false
+ // referencing "root.users", but "users" is a leaf
+ conf.PlacementRules = []PlacementRule{
+ {
+ Name: "fixed",
+ Value: "root.users",
+ },
+ }
err = checkPlacementRules(conf)
- assert.ErrorContains(t, err, "placement rule no. #0 (user->tag->fixed)
references a queue (root.users) which is a leaf")
+ assert.ErrorContains(t, err, "placement rule no. #0 (fixed) references
a queue (root.users) which is not a leaf")
- conf.Queues[0].Queues[0].Parent = true
+ // referencing "root.admins.dev" which doesn't exist but 'create' is
false
+ conf.PlacementRules = createPlacementRules()
conf.PlacementRules[1].Create = false
err = checkPlacementRules(conf)
assert.ErrorContains(t, err, "placement rule no. #1 (fixed->fixed)
references non-existing queues (root.admins.dev) and create is 'false'")
+
+ // referencing "root.default", but queues under "default" cannot be
created due to "default" being leaf
+ conf.PlacementRules = []PlacementRule{
+ {
+ Name: "fixed",
+ Value: "root.default.leaf",
+ },
+ }
+ err = checkPlacementRules(conf)
+ assert.ErrorContains(t, err, "placement rule no. #0 (fixed) references
non-existing queues (root.default.leaf) which cannot be created because the
last queue (default) in the hierarchy is a leaf")
+
+ // two "fixed" rule in a chain with both having fully qualified queues
+ conf.PlacementRules = []PlacementRule{
+ {
+ Name: "fixed",
+ Value: "root.default.leaf",
+ Parent: &PlacementRule{
+ Name: "fixed",
+ Value: "root.default",
+ },
+ },
+ }
+ err = checkPlacementRules(conf)
+ assert.ErrorContains(t, err, "illegal fully qualified 'fixed' rule with
value root.default.leaf")
}
func createQueueConfig() []QueueConfig {
@@ -233,6 +324,20 @@ func createQueueConfig() []QueueConfig {
Name: "users",
Parent: true,
},
+ {
+ Name: "devs",
+ Parent: true,
+ Queues: []QueueConfig{
+ {
+ Name: "yunikorn",
+ Parent: true,
+ },
+ },
+ },
+ {
+ Name: "default",
+ Parent: false,
+ },
{
Name: "admins",
Parent: true,
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
