[YUNIKORN-3139] Decrement 'preemptAttemptsRemaining' counter only when attempt has been made and not for precondition check failures (apache#1035)

Closes: apache#1035

Signed-off-by: mani <[email protected]>

Author: mani

pkg/common/errors.go

@@ -35,9 +35,10 @@ var (
 
 // Constant messages for AllocationLog entries
 const (
-	PreemptionPreconditionsFailed = "Preemption preconditions failed"
-	PreemptionDoesNotGuarantee    = "Preemption queue guarantees check failed"
-	PreemptionShortfall           = "Preemption helped but short of resources"
-	PreemptionDoesNotHelp         = "Preemption does not help"
-	NoVictimForRequiredNode       = "No fit on required node, preemption does not help"
+	PreemptionPreconditionsFailed  = "Preemption preconditions failed"
+	PreemptionDoesNotGuarantee     = "Preemption queue guarantees check failed"
+	PreemptionShortfall            = "Preemption helped but short of resources"
+	PreemptionDoesNotHelp          = "Preemption does not help"
+	NoVictimForRequiredNode        = "No fit on required node, preemption does not help"
+	PreemptionMaxAttemptsExhausted = "Preemption max attempts exhausted"
 )

pkg/scheduler/objects/application.go

@@ -1013,11 +1013,10 @@ func (sa *Application) tryAllocate(headRoom *resources.Resource, allowPreemption
 		// resource must fit in headroom otherwise skip the request (unless preemption could help)
 		if !headRoom.FitInMaxUndef(request.GetAllocatedResource()) {
 			// attempt preemption
-			if allowPreemption && *preemptAttemptsRemaining > 0 {
-				*preemptAttemptsRemaining--
+			if allowPreemption {
 				fullIterator := fullNodeIterator()
 				if fullIterator != nil {
-					if result, ok := sa.tryPreemption(headRoom, preemptionDelay, request, fullIterator, false); ok {
+					if result, ok := sa.tryPreemption(headRoom, preemptionDelay, preemptAttemptsRemaining, request, fullIterator, false); ok {
 						// preemption occurred, and possibly reservation
 						return result
 					}
@@ -1050,11 +1049,10 @@ func (sa *Application) tryAllocate(headRoom *resources.Resource, allowPreemption
 			}
 
 			// no nodes qualify, attempt preemption
-			if allowPreemption && *preemptAttemptsRemaining > 0 {
-				*preemptAttemptsRemaining--
+			if allowPreemption {
 				fullIterator := fullNodeIterator()
 				if fullIterator != nil {
-					if result, ok := sa.tryPreemption(headRoom, preemptionDelay, request, fullIterator, true); ok {
+					if result, ok := sa.tryPreemption(headRoom, preemptionDelay, preemptAttemptsRemaining, request, fullIterator, true); ok {
 						// preemption occurred, and possibly reservation
 						return result
 					}
@@ -1364,14 +1362,23 @@ func (sa *Application) tryReservedAllocate(headRoom *resources.Resource, nodeIte
 	return nil
 }
 
-func (sa *Application) tryPreemption(headRoom *resources.Resource, preemptionDelay time.Duration, ask *Allocation, iterator NodeIterator, nodesTried bool) (*AllocationResult, bool) {
+func (sa *Application) tryPreemption(headRoom *resources.Resource, preemptionDelay time.Duration, preemptionAttemptsRemaining *int, ask *Allocation, iterator NodeIterator, nodesTried bool) (*AllocationResult, bool) {
+	if *preemptionAttemptsRemaining == 0 {
+		log.Log(log.SchedApplication).Debug("Max queue preemption attempts exhausted",
+			zap.String("allocationKey", ask.GetAllocationKey()),
+			zap.String("applicationID", sa.ApplicationID),
+			zap.String("queue", sa.queuePath))
+		ask.LogAllocationFailure(common.PreemptionMaxAttemptsExhausted, true)
+		return nil, false
+	}
 	preemptor := NewPreemptor(sa, headRoom, preemptionDelay, ask, iterator, nodesTried)
 
 	// validate prerequisites for preemption of an ask and mark ask for preemption if successful
 	if !preemptor.CheckPreconditions() {
 		ask.LogAllocationFailure(common.PreemptionPreconditionsFailed, true)
 		return nil, false
 	}
+	*preemptionAttemptsRemaining--
 
 	// track time spent trying preemption
 	tryPreemptionStart := time.Now()

pkg/scheduler/objects/application_test.go

@@ -2237,17 +2237,30 @@ func TestTryAllocatePreemptQueue(t *testing.T) {
 	alloc2 := result2.Request
 	assert.Assert(t, alloc2 != nil, "alloc2 expected")
 
-	// on first attempt, not enough time has passed
+	// preemption max attempts exhausted
+	preemptionAttemptsRemaining = 0
 	result3 := app2.tryAllocate(resources.NewResourceFromMap(map[string]resources.Quantity{"first": 0}), true, 30*time.Second, &preemptionAttemptsRemaining, iterator, iterator, getNode)
 	assert.Assert(t, result3 == nil, "result3 not expected")
 	assert.Assert(t, !alloc2.IsPreempted(), "alloc2 should not have been preempted")
+	log := ask3.GetAllocationLog()
+	assert.Equal(t, log[0].Message, common.PreemptionMaxAttemptsExhausted)
+	assert.Equal(t, preemptionAttemptsRemaining, 0)
+
+	preemptionAttemptsRemaining = 10
+
+	// on first attempt, not enough time has passed
+	result3 = app2.tryAllocate(resources.NewResourceFromMap(map[string]resources.Quantity{"first": 0}), true, 30*time.Second, &preemptionAttemptsRemaining, iterator, iterator, getNode)
+	assert.Assert(t, result3 == nil, "result3 not expected")
+	assert.Assert(t, !alloc2.IsPreempted(), "alloc2 should not have been preempted")
 	assertAllocationLog(t, ask3)
+	assert.Equal(t, preemptionAttemptsRemaining, 10)
 
 	// pass the time and try again
 	ask3.createTime = ask3.createTime.Add(-30 * time.Second)
 	result3 = app2.tryAllocate(resources.NewResourceFromMap(map[string]resources.Quantity{"first": 0}), true, 30*time.Second, &preemptionAttemptsRemaining, iterator, iterator, getNode)
 	assert.Assert(t, result3 != nil && result3.Request != nil && result3.ResultType == Reserved, "alloc3 should be a reservation")
 	assert.Assert(t, alloc2.IsPreempted(), "alloc2 should have been preempted")
+	assert.Equal(t, preemptionAttemptsRemaining, 9)
 }
 
 func TestTryAllocatePreemptNode(t *testing.T) {