This is an automated email from the ASF dual-hosted git repository.
ccondit 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 928b84c7 [YUNIKORN-2789] Queue internalGetMax should use permissive
calculator (#936)
928b84c7 is described below
commit 928b84c738d28541e7d96dce44b886345018d0c9
Author: Wilfred Spiegelenburg <[email protected]>
AuthorDate: Mon Aug 12 14:51:15 2024 -0500
[YUNIKORN-2789] Queue internalGetMax should use permissive calculator (#936)
In the implementation of internalGetMax on the queue, we call resources
ComponentWiseMin(). This returns 0 values for each type that is not
defined in both of the resources passed in. In case there is no overlap it
will return a resource with all types set to "zero". That is contrary to
the documentation on how quotas work and what we do everywhere besides
the internalGetMax implementation.
The ComponentWiseMin function is only used in queue internalGetMax.
Refactor to make the ComponentWiseMinPermissive version the standard
(rename and remove "permissive"), and remove the existing
ComponentWiseMin code.
Closes: #936
Signed-off-by: Craig Condit <[email protected]>
---
pkg/common/configs/configvalidator.go | 6 ++--
pkg/common/resources/resources.go | 20 ++----------
pkg/common/resources/resources_test.go | 58 +---------------------------------
pkg/scheduler/objects/preemption.go | 6 ++--
pkg/scheduler/objects/queue.go | 13 +++-----
pkg/scheduler/objects/queue_test.go | 10 +++---
pkg/scheduler/ugm/manager.go | 2 +-
pkg/scheduler/ugm/queue_tracker.go | 2 +-
8 files changed, 20 insertions(+), 97 deletions(-)
diff --git a/pkg/common/configs/configvalidator.go
b/pkg/common/configs/configvalidator.go
index 176ada84..c80ec026 100644
--- a/pkg/common/configs/configvalidator.go
+++ b/pkg/common/configs/configvalidator.go
@@ -118,7 +118,7 @@ func checkQueueResource(cur QueueConfig, parentM
*resources.Resource) (*resource
if !parentM.FitInMaxUndef(curM) {
return nil, fmt.Errorf("max resource of parent %s is smaller
than maximum resource %s for queue %s", parentM.String(), curM.String(),
cur.Name)
}
- curM = resources.ComponentWiseMinPermissive(curM, parentM)
+ curM = resources.ComponentWiseMin(curM, parentM)
sumG := resources.NewResource()
for _, child := range cur.Queues {
var childG *resources.Resource
@@ -172,7 +172,7 @@ func checkLimitResource(cur QueueConfig, users
map[string]map[string]*resources.
if
!userMaxResource.FitInMaxUndef(limitMaxResources) {
return fmt.Errorf("user %s max resource
%s of queue %s is greater than immediate or ancestor parent maximum resource
%s", user, limitMaxResources.String(), cur.Name, userMaxResource.String())
}
- users[curQueuePath][user] =
resources.ComponentWiseMinPermissive(limitMaxResources, userMaxResource)
+ users[curQueuePath][user] =
resources.ComponentWiseMin(limitMaxResources, userMaxResource)
} else if wildcardMaxResource, ok :=
users[queuePath][common.Wildcard]; user != common.Wildcard && ok {
if
!wildcardMaxResource.FitInMaxUndef(limitMaxResources) {
return fmt.Errorf("user %s max resource
%s of queue %s is greater than wildcard maximum resource %s of immediate or
ancestor parent queue", user, limitMaxResources.String(), cur.Name,
wildcardMaxResource.String())
@@ -189,7 +189,7 @@ func checkLimitResource(cur QueueConfig, users
map[string]map[string]*resources.
return fmt.Errorf("group %s max
resource %s of queue %s is greater than immediate or ancestor parent maximum
resource %s", group, limitMaxResources.String(), cur.Name,
groupMaxResource.String())
}
// Override with min resource
- groups[curQueuePath][group] =
resources.ComponentWiseMinPermissive(limitMaxResources, groupMaxResource)
+ groups[curQueuePath][group] =
resources.ComponentWiseMin(limitMaxResources, groupMaxResource)
} else if wildcardMaxResource, ok :=
groups[queuePath][common.Wildcard]; group != common.Wildcard && ok {
if
!wildcardMaxResource.FitInMaxUndef(limitMaxResources) {
return fmt.Errorf("group %s max
resource %s of queue %s is greater than wildcard maximum resource %s of
immediate or ancestor parent queue", group, limitMaxResources.String(),
cur.Name, wildcardMaxResource.String())
diff --git a/pkg/common/resources/resources.go
b/pkg/common/resources/resources.go
index 0c6eb15c..23cad7b5 100644
--- a/pkg/common/resources/resources.go
+++ b/pkg/common/resources/resources.go
@@ -851,26 +851,10 @@ func StrictlyGreaterThanZero(larger *Resource) bool {
return greater
}
-// Returns a new resource with the smallest value for each quantity in the
resources
-// If either resource passed in is nil a zero resource is returned
-// If a resource type is missing from one of the Resource, it is considered 0
-func ComponentWiseMin(left, right *Resource) *Resource {
- out := NewResource()
- if left != nil && right != nil {
- for k, v := range left.Resources {
- out.Resources[k] = min(v, right.Resources[k])
- }
- for k, v := range right.Resources {
- out.Resources[k] = min(v, left.Resources[k])
- }
- }
- return out
-}
-
-// Returns a new Resource with the smallest value for each quantity in the
Resources
+// ComponentWiseMin returns a new Resource with the smallest value for each
quantity in the Resources
// If either Resource passed in is nil the other Resource is returned
// If a Resource type is missing from one of the Resource, it is considered
empty and the quantity from the other Resource is returned
-func ComponentWiseMinPermissive(left, right *Resource) *Resource {
+func ComponentWiseMin(left, right *Resource) *Resource {
out := NewResource()
if right == nil && left == nil {
return nil
diff --git a/pkg/common/resources/resources_test.go
b/pkg/common/resources/resources_test.go
index a497c187..05e603a7 100644
--- a/pkg/common/resources/resources_test.go
+++ b/pkg/common/resources/resources_test.go
@@ -468,62 +468,6 @@ func TestStrictlyGreaterThanOrEquals(t *testing.T) {
}
func TestComponentWiseMin(t *testing.T) {
- type inputs struct {
- res1 map[string]Quantity
- res2 map[string]Quantity
- sameRef bool
- }
- var tests = []struct {
- caseName string
- input inputs
- expected map[string]Quantity
- }{
- {"nil case", inputs{nil, nil, false},
make(map[string]Quantity)},
- {"nil and zero", inputs{nil, map[string]Quantity{}, false},
make(map[string]Quantity)},
- {"zero and nil", inputs{map[string]Quantity{}, nil, false},
make(map[string]Quantity)},
- {"zero resource", inputs{map[string]Quantity{}, nil, true},
make(map[string]Quantity)},
- {"empty resource and zero resource",
inputs{map[string]Quantity{}, map[string]Quantity{"zero": 0}, false},
map[string]Quantity{"zero": 0}},
- {"zero resource and empty resource",
inputs{map[string]Quantity{"zero": 0}, map[string]Quantity{}, false},
map[string]Quantity{"zero": 0}},
- {"no overlapping resources type",
inputs{map[string]Quantity{"first": 5}, map[string]Quantity{"second": 10},
false}, map[string]Quantity{"first": 0, "second": 0}},
- {"no overlapping resources type",
inputs{map[string]Quantity{"second": 10}, map[string]Quantity{"first": 5},
false}, map[string]Quantity{"first": 0, "second": 0}},
- {"overlapping resources type",
inputs{map[string]Quantity{"first": 5}, map[string]Quantity{"first": 10},
false}, map[string]Quantity{"first": 5}},
- {"overlapping resources type",
inputs{map[string]Quantity{"first": 10}, map[string]Quantity{"first": 5},
false}, map[string]Quantity{"first": 5}},
- {"negative values", inputs{map[string]Quantity{"first": -5,
"second": -5}, map[string]Quantity{"first": 10}, false},
map[string]Quantity{"first": -5, "second": -5}},
- {"negative values", inputs{map[string]Quantity{"first": 10},
map[string]Quantity{"first": -5, "second": -5}, false},
map[string]Quantity{"first": -5, "second": -5}},
- }
- for _, tt := range tests {
- t.Run(tt.caseName, func(t *testing.T) {
- var res1, res2 *Resource
- if tt.input.res1 != nil {
- res1 = NewResourceFromMap(tt.input.res1)
- }
- if tt.input.sameRef {
- res2 = res1
- } else if tt.input.res2 != nil {
- res2 = NewResourceFromMap(tt.input.res2)
- }
-
- result := ComponentWiseMin(res1, res2)
- if result == nil {
- t.Error("Result should be a zero resource
instead of nil")
- } else if len(result.Resources) != len(tt.expected) {
- t.Errorf("Length got %d, expected %d",
len(result.Resources), len(tt.expected))
- }
-
- for expectedKey, expectedValue := range tt.expected {
- if value, ok := result.Resources[expectedKey];
ok {
- if value != expectedValue {
- t.Errorf("Value of %s is wrong,
got %d, expected %d", expectedKey, value, expectedValue)
- }
- } else {
- t.Errorf("resource key %v is not set",
expectedKey)
- }
- }
- })
- }
-}
-
-func TestComponentWiseMinPermissive(t *testing.T) {
smallerRes := NewResourceFromMap(map[string]Quantity{"first": 5,
"second": 15, "third": 6})
higherRes := NewResourceFromMap(map[string]Quantity{"first": 7,
"second": 10, "forth": 6})
expected := NewResourceFromMap(map[string]Quantity{"first": 5,
"second": 10, "third": 6, "forth": 6})
@@ -542,7 +486,7 @@ func TestComponentWiseMinPermissive(t *testing.T) {
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
- result := ComponentWiseMinPermissive(tc.res1, tc.res2)
+ result := ComponentWiseMin(tc.res1, tc.res2)
assert.DeepEqual(t, result, tc.expected)
})
}
diff --git a/pkg/scheduler/objects/preemption.go
b/pkg/scheduler/objects/preemption.go
index fbc23fd0..3303a064 100644
--- a/pkg/scheduler/objects/preemption.go
+++ b/pkg/scheduler/objects/preemption.go
@@ -825,7 +825,7 @@ func (qps *QueuePreemptionSnapshot)
GetRemainingGuaranteedResource() *resources.
used := qps.AllocatedResource.Clone()
used.SubOnlyExisting(qps.PreemptingResource)
remainingGuaranteed.SubOnlyExisting(used)
- return resources.ComponentWiseMinPermissive(remainingGuaranteed, parent)
+ return resources.ComponentWiseMin(remainingGuaranteed, parent)
}
// GetGuaranteedResource computes the current guaranteed resources considering
parent guaranteed
@@ -833,7 +833,7 @@ func (qps *QueuePreemptionSnapshot) GetGuaranteedResource()
*resources.Resource
if qps == nil {
return resources.NewResource()
}
- return
resources.ComponentWiseMinPermissive(qps.Parent.GetGuaranteedResource(),
qps.GuaranteedResource)
+ return resources.ComponentWiseMin(qps.Parent.GetGuaranteedResource(),
qps.GuaranteedResource)
}
// GetMaxResource computes the current max resources considering parent max
@@ -841,7 +841,7 @@ func (qps *QueuePreemptionSnapshot) GetMaxResource()
*resources.Resource {
if qps == nil {
return resources.NewResource()
}
- return
resources.ComponentWiseMinPermissive(qps.Parent.GetMaxResource(),
qps.MaxResource)
+ return resources.ComponentWiseMin(qps.Parent.GetMaxResource(),
qps.MaxResource)
}
// AddAllocation adds an allocation to this snapshot's resource usage
diff --git a/pkg/scheduler/objects/queue.go b/pkg/scheduler/objects/queue.go
index bac9e84b..a905fdd1 100644
--- a/pkg/scheduler/objects/queue.go
+++ b/pkg/scheduler/objects/queue.go
@@ -600,7 +600,7 @@ func (sq *Queue) GetGuaranteedResource()
*resources.Resource {
return sq.guaranteedResource
}
-// GetActualGuaranteedResources returns the actual (including parent)
guaranteed resources for the queue.
+// GetActualGuaranteedResource returns the actual (including parent)
guaranteed resources for the queue.
func (sq *Queue) GetActualGuaranteedResource() *resources.Resource {
if sq == nil {
return resources.NewResource()
@@ -608,7 +608,7 @@ func (sq *Queue) GetActualGuaranteedResource()
*resources.Resource {
parentGuaranteed := sq.parent.GetActualGuaranteedResource()
sq.RLock()
defer sq.RUnlock()
- return resources.ComponentWiseMinPermissive(sq.guaranteedResource,
parentGuaranteed)
+ return resources.ComponentWiseMin(sq.guaranteedResource,
parentGuaranteed)
}
func (sq *Queue) GetPreemptionDelay() time.Duration {
@@ -1201,7 +1201,7 @@ func (sq *Queue) internalHeadRoom(parentHeadRoom
*resources.Resource) *resources
return headRoom
}
// take the minimum value of *all* resource types defined
- return resources.ComponentWiseMinPermissive(headRoom, parentHeadRoom)
+ return resources.ComponentWiseMin(headRoom, parentHeadRoom)
}
// GetMaxResource returns the max resource for the queue. The max resource
should never be larger than the
@@ -1228,12 +1228,10 @@ func (sq *Queue) GetMaxResource() *resources.Resource {
// When defining a limit you therefore should define all resource quantities.
func (sq *Queue) GetMaxQueueSet() *resources.Resource {
// get the limit for the parent first and check against the queue's own
- var limit *resources.Resource
if sq.parent == nil {
return nil
}
- limit = sq.parent.GetMaxQueueSet()
- return sq.internalGetMax(limit)
+ return sq.internalGetMax(sq.parent.GetMaxQueueSet())
}
// internalGetMax does the real max calculation.
@@ -1242,9 +1240,6 @@ func (sq *Queue) internalGetMax(parentLimit
*resources.Resource) *resources.Reso
defer sq.RUnlock()
// no parent queue limit set, not even for root
if parentLimit == nil {
- if sq.maxResource == nil {
- return nil
- }
return sq.maxResource.Clone()
}
// parent limit set, no queue limit return parent
diff --git a/pkg/scheduler/objects/queue_test.go
b/pkg/scheduler/objects/queue_test.go
index 5eaa081f..fd673a64 100644
--- a/pkg/scheduler/objects/queue_test.go
+++ b/pkg/scheduler/objects/queue_test.go
@@ -869,7 +869,7 @@ func TestMaxHeadroomMax(t *testing.T) {
assert.Assert(t, resources.Equals(res, headRoom), "leaf2 queue head
room not as expected %v, got: %v", res, headRoom)
}
-func TestGetMaxUsage(t *testing.T) {
+func TestGetMaxResource(t *testing.T) {
// create the root
root, err := createRootQueue(nil)
assert.NilError(t, err, "queue create failed")
@@ -919,14 +919,14 @@ func TestGetMaxUsage(t *testing.T) {
resMap = map[string]string{"third": "2"}
parent, err = createManagedQueue(root, "parent2", true, resMap)
assert.NilError(t, err, "failed to create parent2 queue")
- res, err = resources.NewResourceFromConf(map[string]string{"first":
"0", "second": "0", "third": "0"})
+ res, err = resources.NewResourceFromConf(map[string]string{"first":
"10", "second": "5", "third": "2"})
assert.NilError(t, err, "failed to create resource")
maxUsage = parent.GetMaxResource()
assert.Assert(t, resources.Equals(res, maxUsage), "parent2 queue should
have max from root set expected %v, got: %v", res, maxUsage)
resMap = map[string]string{"first": "5", "second": "10"}
leaf, err = createManagedQueue(parent, "leaf2", false, resMap)
assert.NilError(t, err, "failed to create leaf2 queue")
- res, err = resources.NewResourceFromConf(map[string]string{"first":
"0", "second": "0", "third": "0"})
+ res, err = resources.NewResourceFromConf(map[string]string{"first":
"5", "second": "5", "third": "2"})
assert.NilError(t, err, "failed to create resource")
maxUsage = leaf.GetMaxResource()
assert.Assert(t, resources.Equals(res, maxUsage), "leaf2 queue should
have reset merged max set expected %v, got: %v", res, maxUsage)
@@ -977,11 +977,11 @@ func TestGetMaxQueueSet(t *testing.T) {
assert.Assert(t, resources.Equals(res, maxSet), "parent2 queue should
have max excluding root expected %v, got: %v", res, maxSet)
// a leaf with max set on different resource than the parent.
- // The parent has limit and root is ignored: expect the merged parent
and leaf to be returned (0 for missing on either)
+ // The parent has limit and root is ignored: expect the merged parent
and leaf to be returned
resMap = map[string]string{"first": "5", "second": "10"}
leaf, err = createManagedQueue(parent, "leaf2", false, resMap)
assert.NilError(t, err, "failed to create leaf2 queue")
- res, err = resources.NewResourceFromConf(map[string]string{"first":
"0", "second": "5", "third": "0"})
+ res, err = resources.NewResourceFromConf(map[string]string{"first":
"5", "second": "5", "third": "2"})
assert.NilError(t, err, "failed to create resource")
maxSet = leaf.GetMaxQueueSet()
assert.Assert(t, resources.Equals(res, maxSet), "leaf2 queue should
have reset merged max set expected %v, got: %v", res, maxSet)
diff --git a/pkg/scheduler/ugm/manager.go b/pkg/scheduler/ugm/manager.go
index 4824038e..517751a1 100644
--- a/pkg/scheduler/ugm/manager.go
+++ b/pkg/scheduler/ugm/manager.go
@@ -660,7 +660,7 @@ func (m *Manager) Headroom(queuePath, applicationID string,
user security.UserGr
return userHeadroom
}
groupHeadroom := groupTracker.headroom(hierarchy)
- return resources.ComponentWiseMinPermissive(userHeadroom, groupHeadroom)
+ return resources.ComponentWiseMin(userHeadroom, groupHeadroom)
}
// CanRunApp checks the maxApplications for this specific application that
runs as the user and group.
diff --git a/pkg/scheduler/ugm/queue_tracker.go
b/pkg/scheduler/ugm/queue_tracker.go
index 0792b5aa..9e77c098 100644
--- a/pkg/scheduler/ugm/queue_tracker.go
+++ b/pkg/scheduler/ugm/queue_tracker.go
@@ -226,7 +226,7 @@ func (qt *QueueTracker) headroom(hierarchy []string,
trackType trackingType) *re
if headroom == nil {
return childHeadroom
}
- return resources.ComponentWiseMinPermissive(headroom, childHeadroom)
+ return resources.ComponentWiseMin(headroom, childHeadroom)
}
// Note: Lock free call. The RLock of the linked tracker (UserTracker and
GroupTracker) should be held before calling this function.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
