[YUNIKORN-2997] add unit test for tryPlaceholderAllocate

[kousei47747]

What is this PR for?

Add unit test for tryPlaceholderAllocate, including 4 test cases:

• No placeholder is allocated
• Requested resource in the real allocation is smaller than the placeholder
• Requested resource in the real allocation is larger than the placeholder
• Allocation request has a different task group than the placeholder

What type of PR is it?

• - Bug Fix
• - Improvement
• - Feature
• - Documentation
• - Hot Fix
• - Refactoring

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2997

[wilfred-s]

No code coverage report as the previous commit was a non code change (e2e test change) The commit did not submit a coverage report to diff against.

[chenyulin0719]

Hi @kousei47747, thanks for the nice patch.
Could we add one more test case to cover the path "allocate placeholder to non-placeholder node"? It occurs when node.preReserveConditions(request) != nil.

• yunikorn-core/pkg/scheduler/objects/application.go

  Line 1199 in 149820f

  if node != nil && node.preReserveConditions(request) == nil {

• yunikorn-core/pkg/scheduler/objects/application.go

  Lines 1261 to 1276 in 149820f

  	// allocation worked: on a non placeholder node update resultType and return
  	// double link to make it easier to find
  	// alloc (the real one) releases points to the placeholder in the releases list
  	reqFit.SetRelease(phFit)
  	// placeholder point to the real one in the releases list
  	phFit.SetRelease(reqFit)
  	// mark placeholder as released
  	phFit.SetReleased(true)
  	// bind node here so it will be handled properly upon replacement
  	reqFit.SetBindTime(time.Now())
  	reqFit.SetNodeID(node.NodeID)
  	reqFit.SetInstanceType(node.GetInstanceType())
  	result := newReplacedAllocationResult(node.NodeID, reqFit)
  	allocResult = result
  	return false

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.15%. Comparing base (149820f) to head (8613413).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
+ Coverage   81.97%   82.15%   +0.17%     
==========================================
  Files          97       97              
  Lines       15621    15621              
==========================================
+ Hits        12806    12833      +27     
+ Misses       2537     2510      -27     
  Partials      278      278              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kousei47747]

@chenyulin0719 thanks for reviewing! I've added the test case, please check.

yunikorn-core/pkg/scheduler/objects/application_test.go

Lines 3557 to 3598 in a2829b0

	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})
	plugins.RegisterSchedulerPlugin(mockPlugin)
	defer plugins.UnregisterSchedulerPlugins()
	// should allocate on node2
	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 not be nil")
	assert.Equal(t, Replaced, result.ResultType, "result type should be Replaced")
	assert.Equal(t, nodeID2, result.NodeID, "result should be on node2")
	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)
	}

[chenyulin0719]

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() will pass.

So it make sense to me now.

[chenyulin0719]

+1 LGTM

[pbacsko]

LGTM

[wilfred-s]

LGTM +1