wilfred-s commented on code in PR #452:
URL: https://github.com/apache/yunikorn-core/pull/452#discussion_r1041696240
##########
pkg/scheduler/objects/application.go:
##########
@@ -886,6 +886,11 @@ func (sa *Application) tryAllocate(headRoom
*resources.Resource, nodeIterator fu
zap.String("required node",
requiredNode))
return nil
}
+
+ // Are there any non daemon set reservations on
specific required node?
+ // Cancel those reservations to run daemon set pods
+ reservations := node.getReservations()
+ sa.removeReservations(request, node, reservations)
Review Comment:
I would only call this if `len(reservations) > 0`
Need to log the fact that we are going to cancel the reservations and why.
`removeReservations()` does not really cover what is done as we only cancel
reservations that do not have a required node set.
##########
pkg/scheduler/objects/application.go:
##########
@@ -919,6 +924,50 @@ func (sa *Application) tryAllocate(headRoom
*resources.Resource, nodeIterator fu
return nil
}
+func (sa *Application) removeReservations(request *AllocationAsk, node *Node,
reservations map[string]*reservation) {
Review Comment:
* request is only needed for logging see earlier comment about logging,
unneeded I think)
* node is unneeded as the reservation contains the node object already
* we need to return a value for the outcome of our cancel action: if another
reservation exists with a required node you leave that reservation intact. That
fact needs to be logged.
Leaving the reservation will directly cause a nil to be returned by
`tryNode()` that follows as the node is reserved but not for that ask. We then
reserve the node for this app/ask. Currently we have a limit of 1 reservation
per node (node.go @ line 436) so that will fail...
--
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]
