This is an automated email from the ASF dual-hosted git repository.
wilfreds 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 5a40e8ee [YUNIKORN-2197] remove FitInMaxUndef from node code (#735)
5a40e8ee is described below
commit 5a40e8ee01b4b43c9454fb4541e180df0c751c57
Author: Wilfred Spiegelenburg <[email protected]>
AuthorDate: Wed Nov 29 14:48:29 2023 +1100
[YUNIKORN-2197] remove FitInMaxUndef from node code (#735)
Nodes should perform a strict FitIn for the resources requested not
allowing resource types in allocation that are not defined on the node.
Includes start of the Resources.FitIn refactor
This is a followup of the cleanup from YUNIKORN-1125.
Discovered while testing YUNIKORN-2174.
Closes: #735
Signed-off-by: Wilfred Spiegelenburg <[email protected]>
---
pkg/common/resources/resources.go | 12 ++++-
pkg/common/resources/resources_test.go | 53 +++++++------------
pkg/scheduler/objects/node.go | 28 +++++-----
pkg/scheduler/objects/node_test.go | 94 ++++++++++++++++++++++++++--------
4 files changed, 118 insertions(+), 69 deletions(-)
diff --git a/pkg/common/resources/resources.go
b/pkg/common/resources/resources.go
index efa94bc4..7b64fe1a 100644
--- a/pkg/common/resources/resources.go
+++ b/pkg/common/resources/resources.go
@@ -417,14 +417,22 @@ func subNonNegative(left, right *Resource) (*Resource,
string) {
return out, message
}
-// Check if smaller fits in larger
+// FitIn Checks if smaller fits in larger
// Types not defined in the larger resource are considered 0 values for
Quantity
// A nil resource is treated as an empty resource (all types are 0)
+// Deprecated: use receiver version Resource.FitIn
func FitIn(larger, smaller *Resource) bool {
return larger.fitIn(smaller, false)
}
-// Check if smaller fits in the defined resource
+// FitIn checks if smaller fits in the defined resource
+// Types not defined in resource this is called against are considered 0 for
Quantity
+// A nil resource is treated as an empty resource (no types defined)
+func (r *Resource) FitIn(smaller *Resource) bool {
+ return r.fitIn(smaller, false)
+}
+
+// FitInMaxUndef checks if smaller fits in the defined resource
// Types not defined in resource this is called against are considered the
maximum value for Quantity
// A nil resource is treated as an empty resource (no types defined)
func (r *Resource) FitInMaxUndef(smaller *Resource) bool {
diff --git a/pkg/common/resources/resources_test.go
b/pkg/common/resources/resources_test.go
index 5c32095c..b3846e6c 100644
--- a/pkg/common/resources/resources_test.go
+++ b/pkg/common/resources/resources_test.go
@@ -1034,42 +1034,27 @@ func TestEqualsOrEmpty(t *testing.T) {
}
func TestFitIn(t *testing.T) {
- // simple case (nil checks)
- empty := NewResource()
- if !FitIn(nil, empty) {
- t.Error("fitin empty in nil resource failed")
- }
- if !FitIn(empty, nil) {
- t.Error("fitin nil in empty resource failed")
- }
- // zero set resources
- smaller := &Resource{Resources: map[string]Quantity{"a": 1}}
- if FitIn(empty, smaller) {
- t.Errorf("fitin resource with value %v should not fit in
empty", smaller)
+ tests := []struct {
+ name string
+ larger *Resource
+ smaller *Resource
+ want bool
+ }{
+ {"nil larger", nil, NewResource(), true},
+ {"nil smaller", NewResource(), nil, true},
+ {"zero set", NewResource(),
NewResourceFromMap(map[string]Quantity{"a": 1}), false},
+ {"same type", NewResourceFromMap(map[string]Quantity{"a": 5}),
NewResourceFromMap(map[string]Quantity{"a": 1}), true},
+ {"not in smaller",
NewResourceFromMap(map[string]Quantity{"not-in-smaller": 1}),
NewResourceFromMap(map[string]Quantity{"a": 1}), false},
+ {"not in larger",
NewResourceFromMap(map[string]Quantity{"not-in-smaller": 1}),
NewResourceFromMap(map[string]Quantity{"not-in-larger": 1}), false},
+ {"negative larger", NewResourceFromMap(map[string]Quantity{"a":
-10}), NewResourceFromMap(map[string]Quantity{"a": 0, "b": -10}), true},
+ {"negative smaller",
NewResourceFromMap(map[string]Quantity{"a": -5}),
NewResourceFromMap(map[string]Quantity{"a": 0, "b": 10}), false},
}
-
- // simple resources, same type
- larger := NewResourceFromMap(map[string]Quantity{"a": 5})
- if !FitIn(larger, smaller) {
- t.Errorf("fitin smaller resource with value %v should fit in
larger %v", smaller, larger)
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ assert.Equal(t, FitIn(tt.larger, tt.smaller), tt.want,
"unexpected FitIn result (parameter method)")
+ assert.Equal(t, tt.larger.FitIn(tt.smaller), tt.want,
"unexpected FitIn result (receiver method)")
+ })
}
-
- // check undefined in larger
- larger = &Resource{Resources: map[string]Quantity{"not-in-smaller": 1}}
- assert.Assert(t, !FitIn(larger, smaller), "different type in smaller %v
should not fit in larger %v", smaller, larger)
-
- // check undefined in smaller
- smaller = &Resource{Resources: map[string]Quantity{"not-in-larger": 1}}
- assert.Assert(t, !FitIn(larger, smaller), "different type in smaller %v
should not fit in larger %v", smaller, larger)
-
- // complex case: just checking the resource merge with negative values,
positive is already tested above
- larger = &Resource{Resources: map[string]Quantity{"a": -10}}
- smaller = &Resource{Resources: map[string]Quantity{"a": 0, "b": -10}}
- assert.Assert(t, FitIn(larger, smaller), "fitin smaller resource with
zero or neg value %v should fit in larger %v", smaller, larger)
-
- larger = &Resource{Resources: map[string]Quantity{"a": -5}}
- smaller = &Resource{Resources: map[string]Quantity{"a": 0, "b": 10}}
- assert.Assert(t, !FitIn(larger, smaller), "fitin smaller resource with
value %v should not fit in larger %v", smaller, larger)
}
// simple cases (nil checks)
diff --git a/pkg/scheduler/objects/node.go b/pkg/scheduler/objects/node.go
index cde53ea7..5794c313 100644
--- a/pkg/scheduler/objects/node.go
+++ b/pkg/scheduler/objects/node.go
@@ -284,10 +284,14 @@ func (sn *Node) GetUtilizedResource() *resources.Resource
{
return &resources.Resource{Resources: utilizedResource}
}
+// FitInNode checks if the request fits in the node.
+// All resources types requested must match the resource types provided by the
nodes.
+// A request may ask for only a subset of the types, but the node must provide
at least the
+// resource types requested in a larger or equal quantity as requested.
func (sn *Node) FitInNode(resRequest *resources.Resource) bool {
sn.RLock()
defer sn.RUnlock()
- return sn.totalResource.FitInMaxUndef(resRequest)
+ return sn.totalResource.FitIn(resRequest)
}
// Remove the allocation to the node.
@@ -311,8 +315,8 @@ func (sn *Node) RemoveAllocation(uuid string) *Allocation {
return nil
}
-// Add the allocation to the node. Used resources will increase available will
decrease.
-// A nil Allocation makes no changes. Pre-empted resources must have been
released already.
+// AddAllocation adds the allocation to the node. Used resources will increase
available will decrease.
+// A nil Allocation makes no changes. Preempted resources must have been
released already.
// Do a sanity check to make sure it still fits in the node and nothing has
changed
func (sn *Node) AddAllocation(alloc *Allocation) bool {
if alloc == nil {
@@ -321,9 +325,9 @@ func (sn *Node) AddAllocation(alloc *Allocation) bool {
defer sn.notifyListeners()
sn.Lock()
defer sn.Unlock()
- // check if this still fits: it might have changed since pre check
+ // check if this still fits: it might have changed since pre-check
res := alloc.GetAllocatedResource()
- if sn.availableResource.FitInMaxUndef(res) {
+ if sn.availableResource.FitIn(res) {
sn.allocations[alloc.GetUUID()] = alloc
sn.allocatedResource.AddTo(res)
sn.availableResource.SubFrom(res)
@@ -349,7 +353,7 @@ func (sn *Node) ReplaceAllocation(uuid string, replace
*Allocation, delta *resou
// The allocatedResource and availableResource should be updated in the
same way
sn.allocatedResource.AddTo(delta)
sn.availableResource.SubFrom(delta)
- if !resources.FitIn(before, sn.allocatedResource) {
+ if !before.FitIn(sn.allocatedResource) {
log.Log(log.SchedNode).Warn("unexpected increase in node usage
after placeholder replacement",
zap.String("placeholder uuid", uuid),
zap.String("allocation uuid", replace.GetUUID()),
@@ -357,12 +361,12 @@ func (sn *Node) ReplaceAllocation(uuid string, replace
*Allocation, delta *resou
}
}
-// Check if the proposed allocation fits in the available resources.
+// CanAllocate checks if the proposed allocation fits in the available
resources.
// If the proposed allocation does not fit false is returned.
func (sn *Node) CanAllocate(res *resources.Resource) bool {
sn.RLock()
defer sn.RUnlock()
- return sn.availableResource.FitInMaxUndef(res)
+ return sn.availableResource.FitIn(res)
}
// Checking pre-conditions in the shim for an allocation.
@@ -405,8 +409,8 @@ func (sn *Node) preConditions(ask *AllocationAsk, allocate
bool) bool {
return true
}
-// Check if the node should be considered as a possible node to allocate on.
-// This is a lock free call. No updates are made this only performs a pre
allocate checks
+// preAllocateCheck checks if the node should be considered as a possible node
to allocate on.
+// No updates are made this only performs a pre allocate checks
func (sn *Node) preAllocateCheck(res *resources.Resource, resKey string) bool {
// cannot allocate zero or negative resource
if !resources.StrictlyGreaterThanZero(res) {
@@ -427,7 +431,7 @@ func (sn *Node) preAllocateCheck(res *resources.Resource,
resKey string) bool {
sn.RLock()
defer sn.RUnlock()
// returns true/false based on if the request fits in what we have
calculated
- return sn.availableResource.FitInMaxUndef(res)
+ return sn.availableResource.FitIn(res)
}
// Return if the node has been reserved by any application
@@ -479,7 +483,7 @@ func (sn *Node) Reserve(app *Application, ask
*AllocationAsk) error {
return fmt.Errorf("reservation creation failed app or ask are
nil on nodeID %s", sn.NodeID)
}
// reservation must fit on the empty node
- if !resources.FitIn(sn.totalResource, ask.GetAllocatedResource()) {
+ if !sn.totalResource.FitIn(ask.GetAllocatedResource()) {
log.Log(log.SchedNode).Debug("reservation does not fit on the
node",
zap.String("nodeID", sn.NodeID),
zap.String("appID", app.ApplicationID),
diff --git a/pkg/scheduler/objects/node_test.go
b/pkg/scheduler/objects/node_test.go
index d37d696c..26d33c2f 100644
--- a/pkg/scheduler/objects/node_test.go
+++ b/pkg/scheduler/objects/node_test.go
@@ -130,6 +130,10 @@ func TestPreAllocateCheck(t *testing.T) {
assert.Assert(t, node.preAllocateCheck(resSmall, ""), "small resource
should have fitted on node")
assert.Assert(t, !node.preAllocateCheck(resLarge, ""), "too large
resource should not have fitted on node")
+ // unknown resource type in request is rejected always
+ resOther :=
resources.NewResourceFromMap(map[string]resources.Quantity{"unknown": 1})
+ assert.Assert(t, !node.preAllocateCheck(resOther, ""), "unknown
resource type should not have fitted on node")
+
// set allocated resource
node.AddAllocation(newAllocation(appID1, "UUID1", nodeID, resSmall))
assert.Assert(t, node.preAllocateCheck(resSmall, ""), "small resource
should have fitted in available allocation")
@@ -156,19 +160,30 @@ func TestPreAllocateCheck(t *testing.T) {
// Only test the CanAllocate code, the used logic in preAllocateCheck has its
own test
func TestCanAllocate(t *testing.T) {
- node := newNode(nodeID1, map[string]resources.Quantity{"first": 10})
- if node == nil || node.NodeID != nodeID1 {
- t.Fatalf("node create failed which should not have %v", node)
- }
- // normal alloc
- res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
- if !node.CanAllocate(res) {
- t.Error("node should have accepted allocation")
- }
- // check one that pushes node over its size
- res =
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 11})
- if node.CanAllocate(res) {
- t.Error("node should have rejected allocation (oversize)")
+ available :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 10,
"second": 10})
+ request :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 1,
"second": 1})
+ other :=
resources.NewResourceFromMap(map[string]resources.Quantity{"unknown": 1})
+ tests := []struct {
+ name string
+ available *resources.Resource
+ request *resources.Resource
+ want bool
+ }{
+ {"all nils", nil, nil, true},
+ {"nil node available", nil, request, false},
+ {"all matching, req smaller", available, request, true},
+ {"partial match request", available,
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5}), true},
+ {"all matching, req larger", available, resources.Add(request,
available), false},
+ {"partial match available", available, resources.Add(request,
other), false},
+ {"unmatched request", available, other, false},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ sn := &Node{
+ availableResource: tt.available,
+ }
+ assert.Equal(t, sn.CanAllocate(tt.request), tt.want,
"unexpected node can run result")
+ })
}
}
@@ -193,16 +208,24 @@ func TestNodeReservation(t *testing.T) {
t.Errorf("illegal reservation requested but did not fail: error
%v", err)
}
+ // too large for node
res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 15})
ask := newAllocationAsk(aKey, appID1, res)
app := newApplication(appID1, "default", "root.unknown")
-
- // too large for node
err = node.Reserve(app, ask)
if err == nil {
t.Errorf("requested reservation does not fit in node resource
but did not fail: error %v", err)
}
+ // resource type not available on node
+ res =
resources.NewResourceFromMap(map[string]resources.Quantity{"unknown": 1})
+ ask = newAllocationAsk(aKey, appID1, res)
+ app = newApplication(appID1, "default", "root.unknown")
+ err = node.Reserve(app, ask)
+ if err == nil {
+ t.Errorf("requested reservation does not match node resource
types but did not fail: error %v", err)
+ }
+
res =
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
ask = newAllocationAsk(aKey, appID1, res)
app = newApplication(appID1, "default", "root.unknown")
@@ -364,13 +387,14 @@ func TestAddAllocation(t *testing.T) {
}
// check nil alloc
- node.AddAllocation(nil)
- if len(node.GetAllAllocations()) > 0 {
- t.Fatalf("nil allocation should not have been added: %v", node)
- }
+ assert.Assert(t, !node.AddAllocation(nil), "nil allocation should not
have been added: %v", node)
+ // check alloc that does not match
+ unknown :=
resources.NewResourceFromMap(map[string]resources.Quantity{"unknown": 1})
+ assert.Assert(t, !node.AddAllocation(newAllocation(appID1, "1",
nodeID1, unknown)), "unmatched resource type in allocation should not have been
added: %v", node)
+
// allocate half of the resources available and check the calculation
half :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 50,
"second": 100})
- node.AddAllocation(newAllocation(appID1, "1", nodeID1, half))
+ assert.Assert(t, node.AddAllocation(newAllocation(appID1, "1", nodeID1,
half)), "add allocation 1 should not have failed")
if node.GetAllocation("1") == nil {
t.Fatal("failed to add allocations: allocation not returned")
}
@@ -386,7 +410,7 @@ func TestAddAllocation(t *testing.T) {
}
// second and check calculation
piece :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 25,
"second": 50})
- node.AddAllocation(newAllocation(appID1, "2", nodeID1, piece))
+ assert.Assert(t, node.AddAllocation(newAllocation(appID1, "2", nodeID1,
piece)), "add allocation 2 should not have failed")
if node.GetAllocation("2") == nil {
t.Fatal("failed to add allocations: allocation not returned")
}
@@ -747,3 +771,31 @@ func TestNodeEvents(t *testing.T) {
assert.Equal(t, si.EventRecord_REMOVE, event.EventChangeType)
assert.Equal(t, si.EventRecord_NODE_RESERVATION,
event.EventChangeDetail)
}
+
+func TestNode_FitInNode(t *testing.T) {
+ total :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 10,
"second": 10})
+ request :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 1,
"second": 1})
+ other :=
resources.NewResourceFromMap(map[string]resources.Quantity{"unknown": 1})
+ tests := []struct {
+ name string
+ totalRes *resources.Resource
+ resRequest *resources.Resource
+ want bool
+ }{
+ {"all nils", nil, nil, true},
+ {"nil node size", nil, request, false},
+ {"all matching, req smaller", total, request, true},
+ {"partial match request", total,
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5}), true},
+ {"all matching, req larger", total, resources.Add(request,
total), false},
+ {"partial match total", total, resources.Add(request, other),
false},
+ {"unmatched request", total, other, false},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ sn := &Node{
+ totalResource: tt.totalRes,
+ }
+ assert.Equal(t, sn.FitInNode(tt.resRequest), tt.want,
"unexpected node fit result")
+ })
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
