chenyulin0719 commented on code in PR #1004:
URL: https://github.com/apache/yunikorn-core/pull/1004#discussion_r1894631136
##########
pkg/scheduler/objects/application_test.go:
##########
@@ -3438,3 +3427,172 @@ func TestApplication_canAllocationReserve(t *testing.T)
{
})
}
}
+
+func TestTryPlaceHolderAllocateNoPlaceHolders(t *testing.T) {
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ask := newAllocationAsk(aKey, appID0, res)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since there are
no placeholders to allocate")
+}
+
+func TestTryPlaceHolderAllocateSmallerRequest(t *testing.T) {
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request is smaller than placeholder
+ smallerRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 1})
+ ask := newAllocationAsk(aKey, appID0, smallerRes)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result != nil, "result should not be nil since the ask
is smaller than the placeholder")
+ assert.Equal(t, Replaced, result.ResultType, "result type should be
Replaced")
+ assert.Equal(t, nodeID1, result.NodeID, "result should be on the same
node as placeholder")
+ assert.Equal(t, ask, result.Request, "result should contain the ask")
+ assert.Equal(t, ph, result.Request.GetRelease(), "real allocation
should link to placeholder")
+ assert.Equal(t, result.Request, ph.GetRelease(), "placeholder should
link to real allocation")
+ // placeholder data remains unchanged until RM confirms the replacement
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+}
+
+func TestTryPlaceHolderAllocateLargerRequest(t *testing.T) {
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request is larger than placeholder
+ largerRes :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 10})
+ ask := newAllocationAsk(aKey, appID0, largerRes)
+ ask.taskGroupName = tg1
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since the ask is
larger than the placeholder")
+ assert.Assert(t, ph.IsReleased(), "placeholder should have been
released")
+ // placeholder data remains unchanged until RM confirms the release
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+}
+
+func TestTryPlaceHolderAllocateDifferentTaskGroups(t *testing.T) {
+ node := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node}
+ iterator := getNodeIteratorFn(node)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // allocation request has a different task group
+ ask := newAllocationAsk(aKey, appID0, res)
+ ask.taskGroupName = tg2
+ err = app.AddAllocationAsk(ask)
+ assert.NilError(t, err, "ask should have been added to app")
+
+ result := app.tryPlaceholderAllocate(iterator, getNode)
+ assert.Assert(t, result == nil, "result should be nil since the ask has
a different task group")
+}
+
+func TestTryPlaceHolderAllocateDifferentNodes(t *testing.T) {
+ node1 := newNode(nodeID1, map[string]resources.Quantity{"first": 5})
+ node2 := newNode(nodeID2, map[string]resources.Quantity{"first": 5})
+ nodeMap := map[string]*Node{nodeID1: node1, nodeID2: node2}
+ iterator := getNodeIteratorFn(node1, node2)
+ getNode := func(nodeID string) *Node {
+ return nodeMap[nodeID]
+ }
+
+ app := newApplication(appID0, "default", "root.default")
+
+ queue, err := createRootQueue(nil)
+ assert.NilError(t, err, "queue create failed")
+ app.queue = queue
+
+ res :=
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+ ph := newPlaceholderAlloc(appID0, nodeID1, res, tg1)
+ app.AddAllocation(ph)
+ app.addPlaceholderData(ph)
+ assertPlaceholderData(t, app, tg1, 1, 0, 0, res)
+
+ // predicate check fails on node1 and passes on node2
+ mockPlugin := mockCommon.NewPredicatePlugin(false,
map[string]int{nodeID1: 0})
Review Comment:
At the beginning, I'm confused about why using `{nodeID1: 0}) (fail always)`
instead of `{nodeID1: -1}) (fail reserve)`. After rethinking it, I realize
it's reasonable because if we set `{nodeID1: -1})`, `nodeID1` will be picked
again in the following `try all nodes` check , the
[preAllocateCheck()](https://github.com/apache/yunikorn-core/blob/2113cd7e439c840330db313901d8f7855ab3b532/pkg/scheduler/objects/application.go#L1241C1-L1242C1)
will pass.
So it's make sense to me now.
--
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]
