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 9f46b816 [YUNIKORN-2794] Resource: Change SubOnlyExisting() to same
signature as AddOnlyExisting() (#949)
9f46b816 is described below
commit 9f46b81698a43798e87f9edaac37631b9f592a1e
Author: Tzu-Hua Lan <[email protected]>
AuthorDate: Mon Aug 19 11:30:32 2024 -0500
[YUNIKORN-2794] Resource: Change SubOnlyExisting() to same signature as
AddOnlyExisting() (#949)
Closes: #949
Signed-off-by: Craig Condit <[email protected]>
---
pkg/common/resources/resources.go | 16 ++++----
pkg/common/resources/resources_test.go | 72 +++++++++++++++-------------------
pkg/scheduler/objects/application.go | 4 +-
pkg/scheduler/objects/preemption.go | 21 ++++------
pkg/scheduler/objects/queue.go | 4 +-
pkg/scheduler/ugm/queue_tracker.go | 3 +-
6 files changed, 53 insertions(+), 67 deletions(-)
diff --git a/pkg/common/resources/resources.go
b/pkg/common/resources/resources.go
index c5bc7c54..0f6083ff 100644
--- a/pkg/common/resources/resources.go
+++ b/pkg/common/resources/resources.go
@@ -363,18 +363,18 @@ func Sub(left, right *Resource) *Resource {
return out
}
-// SubOnlyExisting subtract delta from defined resource.
-// Ignore any type not defined in the base resource (ie receiver).
-// Used as part of the headroom updates as undefined resources are unlimited
-func (r *Resource) SubOnlyExisting(delta *Resource) {
+// SubOnlyExisting subtracts delta from base resource, ignoring any type not
defined in the base resource.
+func SubOnlyExisting(base, delta *Resource) *Resource {
// check nil inputs and shortcut
- if r == nil || delta == nil {
- return
+ if base == nil || delta == nil {
+ return base.Clone()
}
// neither are nil, subtract the delta
- for k := range r.Resources {
- r.Resources[k] = subVal(r.Resources[k], delta.Resources[k])
+ result := NewResource()
+ for k := range base.Resources {
+ result.Resources[k] = subVal(base.Resources[k],
delta.Resources[k])
}
+ return result
}
// AddOnlyExisting adds delta to base resource, ignoring any type not defined
in the base resource.
diff --git a/pkg/common/resources/resources_test.go
b/pkg/common/resources/resources_test.go
index 88c2c421..1cfe3500 100644
--- a/pkg/common/resources/resources_test.go
+++ b/pkg/common/resources/resources_test.go
@@ -1058,20 +1058,6 @@ func TestSubFrom(t *testing.T) {
}
}
-func TestSubOnlyExistingNil(t *testing.T) {
- // simple case (nil checks)
- defer func() {
- if r := recover(); r != nil {
- t.Fatal("SubOnlyExisting panic on nil resource")
- }
- }()
- var r *Resource
- r.SubOnlyExisting(nil)
- if r != nil {
- t.Errorf("sub nil resources did not return nil resource: %v", r)
- }
-}
-
func TestSubEliminateNegative(t *testing.T) {
// simple case (nil checks)
result := SubEliminateNegative(nil, nil)
@@ -1081,32 +1067,38 @@ func TestSubEliminateNegative(t *testing.T) {
}
func TestSubOnlyExisting(t *testing.T) {
- // remove nil from empty resource
- left := NewResource()
- left.SubOnlyExisting(nil)
- assert.Equal(t, len(left.Resources), 0, "sub nil resource did not
return unchanged resource")
-
- // remove from empty resource
- delta := NewResourceFromMap(map[string]Quantity{"a": 5})
- left.SubOnlyExisting(delta)
- assert.Equal(t, len(left.Resources), 0, "sub simple resource did not
return unchanged resource")
-
- // complex case: just checking the resource merge, values check is
secondary
- left = &Resource{Resources: map[string]Quantity{"a": 0}}
- delta = &Resource{Resources: map[string]Quantity{"a": 1, "b": 0}}
- left.SubOnlyExisting(delta)
- expected := &Resource{Resources: map[string]Quantity{"a": -1}}
- assert.Equal(t, len(left.Resources), 1, "sub with 1 resource returned
more or less types")
- assert.Assert(t, Equals(left, expected), "sub failed expected %v,
actual %v", expected, left)
-
- // complex case: just checking the resource merge, values check is
secondary
- left = &Resource{Resources: map[string]Quantity{"a": 1, "b": 0}}
- delta = &Resource{Resources: map[string]Quantity{"a": 1}}
- left.SubOnlyExisting(delta)
- assert.Equal(t, len(left.Resources), 2, "sub with 2 resource returned
more or less types")
-
- expected = &Resource{Resources: map[string]Quantity{"a": 0, "b": 0}}
- assert.Assert(t, Equals(left, expected), "sub failed expected %v,
actual %v", expected, left)
+ var tests = []struct {
+ caseName string
+ base map[string]Quantity
+ delta map[string]Quantity
+ expected map[string]Quantity
+ }{
+ {"nil resources", nil, nil, nil},
+ {"nil base", nil, map[string]Quantity{"a": 5}, nil},
+ {"nil delta", map[string]Quantity{"a": 5}, nil,
map[string]Quantity{"a": 5}},
+ {"empty base", map[string]Quantity{}, map[string]Quantity{"a":
5}, map[string]Quantity{}},
+ {"single type base matched", map[string]Quantity{"a": 5},
map[string]Quantity{"a": 3, "b": 1}, map[string]Quantity{"a": 2}},
+ {"single type base unmatched", map[string]Quantity{"a": 5},
map[string]Quantity{"b": 1}, map[string]Quantity{"a": 5}},
+ {"multi type base partial match", map[string]Quantity{"a": 5,
"b": 3}, map[string]Quantity{"a": 2, "c": 1}, map[string]Quantity{"a": 3, "b":
3}},
+ {"multi type base match", map[string]Quantity{"a": 5, "b": 3},
map[string]Quantity{"a": 2, "b": 1}, map[string]Quantity{"a": 3, "b": 2}},
+ {"negative result", map[string]Quantity{"a": 1},
map[string]Quantity{"a": 3}, map[string]Quantity{"a": -2}},
+ }
+ for _, tt := range tests {
+ t.Run(tt.caseName, func(t *testing.T) {
+ var base, delta, expected *Resource
+ if tt.base != nil {
+ base = NewResourceFromMap(tt.base)
+ }
+ if tt.delta != nil {
+ delta = NewResourceFromMap(tt.delta)
+ }
+ if tt.expected != nil {
+ expected = NewResourceFromMap(tt.expected)
+ }
+ result := SubOnlyExisting(base, delta)
+ assert.Assert(t, Equals(result, expected), "incorrect
result returned")
+ })
+ }
}
func TestAddOnlyExisting(t *testing.T) {
diff --git a/pkg/scheduler/objects/application.go
b/pkg/scheduler/objects/application.go
index b796f831..1ccde74e 100644
--- a/pkg/scheduler/objects/application.go
+++ b/pkg/scheduler/objects/application.go
@@ -926,8 +926,8 @@ func (sa *Application) getOutstandingRequests(headRoom
*resources.Resource, user
// if headroom is still enough for the resources
*total = append(*total, request)
}
- headRoom.SubOnlyExisting(request.GetAllocatedResource())
-
userHeadRoom.SubOnlyExisting(request.GetAllocatedResource())
+ headRoom = resources.SubOnlyExisting(headRoom,
request.GetAllocatedResource())
+ userHeadRoom = resources.SubOnlyExisting(userHeadRoom,
request.GetAllocatedResource())
}
}
}
diff --git a/pkg/scheduler/objects/preemption.go
b/pkg/scheduler/objects/preemption.go
index 6b00edab..40190fe4 100644
--- a/pkg/scheduler/objects/preemption.go
+++ b/pkg/scheduler/objects/preemption.go
@@ -768,21 +768,17 @@ func (qps *QueuePreemptionSnapshot) Duplicate(copy
map[string]*QueuePreemptionSn
}
func (qps *QueuePreemptionSnapshot) GetPreemptableResource()
*resources.Resource {
- if qps == nil {
- return nil
- }
- parentPreemptableResource := qps.Parent.GetPreemptableResource()
- actual := qps.AllocatedResource.Clone()
-
// No usage, so nothing to preempt
- if actual.IsEmpty() {
+ if qps == nil || qps.AllocatedResource.IsEmpty() {
return nil
}
- actual.SubOnlyExisting(qps.PreemptingResource)
+
+ parentPreemptableResource := qps.Parent.GetPreemptableResource()
+ actual := resources.SubOnlyExisting(qps.AllocatedResource,
qps.PreemptingResource)
// Calculate preemptable resource. +ve means Over utilized, -ve means
Under utilized, 0 means correct utilization
guaranteed := qps.GuaranteedResource
- actual.SubOnlyExisting(guaranteed)
+ actual = resources.SubOnlyExisting(actual, guaranteed)
preemptableResource := actual
// Keep only the resource type which needs to be preempted
@@ -815,7 +811,7 @@ func (qps *QueuePreemptionSnapshot)
GetRemainingGuaranteedResource() *resources.
return nil
}
parent := qps.Parent.GetRemainingGuaranteedResource()
- remainingGuaranteed := qps.GuaranteedResource.Clone()
+ remainingGuaranteed := qps.GuaranteedResource
// No Guaranteed set, so nothing remaining
// In case of guaranteed not set for queues at specific level, inherits
the same from parent queue.
@@ -824,9 +820,8 @@ func (qps *QueuePreemptionSnapshot)
GetRemainingGuaranteedResource() *resources.
if parent.IsEmpty() && remainingGuaranteed.IsEmpty() {
return nil
}
- used := qps.AllocatedResource.Clone()
- used.SubOnlyExisting(qps.PreemptingResource)
- remainingGuaranteed.SubOnlyExisting(used)
+ used := resources.SubOnlyExisting(qps.AllocatedResource,
qps.PreemptingResource)
+ remainingGuaranteed = resources.SubOnlyExisting(remainingGuaranteed,
used)
if qps.AskQueue != nil {
// In case ask queue has guaranteed set, its own values carries
higher precedence over the parent or ancestor
if qps.AskQueue.QueuePath == qps.QueuePath &&
!remainingGuaranteed.IsEmpty() {
diff --git a/pkg/scheduler/objects/queue.go b/pkg/scheduler/objects/queue.go
index 2a277eb6..dd13b22b 100644
--- a/pkg/scheduler/objects/queue.go
+++ b/pkg/scheduler/objects/queue.go
@@ -1208,7 +1208,7 @@ func (sq *Queue) getMaxHeadRoom() *resources.Resource {
func (sq *Queue) internalHeadRoom(parentHeadRoom *resources.Resource)
*resources.Resource {
sq.RLock()
defer sq.RUnlock()
- headRoom := sq.maxResource.Clone()
+ headRoom := sq.maxResource
// if we have no max set headroom is always the same as the parent
if headRoom == nil {
@@ -1217,7 +1217,7 @@ func (sq *Queue) internalHeadRoom(parentHeadRoom
*resources.Resource) *resources
// calculate what we have left over after removing all allocation
// ignore unlimited resource types (ie the ones not defined in max)
- headRoom.SubOnlyExisting(sq.allocatedResource)
+ headRoom = resources.SubOnlyExisting(headRoom, sq.allocatedResource)
// check the minimum of the two: parentHeadRoom is nil for root
if parentHeadRoom == nil {
diff --git a/pkg/scheduler/ugm/queue_tracker.go
b/pkg/scheduler/ugm/queue_tracker.go
index 9e77c098..d4e1d373 100644
--- a/pkg/scheduler/ugm/queue_tracker.go
+++ b/pkg/scheduler/ugm/queue_tracker.go
@@ -219,8 +219,7 @@ func (qt *QueueTracker) headroom(hierarchy []string,
trackType trackingType) *re
// arrived at the leaf or on the way out: check against current max if
set
if !resources.IsZero(qt.maxResources) {
- headroom = qt.maxResources.Clone()
- headroom.SubOnlyExisting(qt.resourceUsage)
+ headroom = resources.SubOnlyExisting(qt.maxResources,
qt.resourceUsage)
}
if headroom == nil {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
