[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #297: [YUNIKORN-791] Allow to register nodes with infinite resources

[GitBox <gi...@apache.org>]

wilfred-s commented on a change in pull request #297:
URL: https://github.com/apache/incubator-yunikorn-core/pull/297#discussion_r690899105



##########
File path: pkg/scheduler/context.go
##########
@@ -590,8 +590,52 @@ func (cc *ClusterContext) updateNodes(request *si.UpdateRequest) {
 func (cc *ClusterContext) addNodes(request *si.UpdateRequest) {
 	acceptedNodes := make([]*si.AcceptedNode, 0)
 	rejectedNodes := make([]*si.RejectedNode, 0)
+	// if we have an unlimited node, reject all new nodes
+	// if we have an unlimited node between the new nodes, register only that node

Review comment:
       This needs to be moved into the loop: nodes are linked to partitions. I _could_ have an unlimited node on one partition but not on the other:
   - create a new node object
   - get the partition: fail if partition not found (existing behaviour)
   - check for the unlimited and limited combination:
     - if new node is unlimited registered partition nodes must be 0
     - if new node is limited registered nodes = 0 or larger than 1 OK otherwise check the single node and make sure it is not unlimited
   - reject / accept nodes are processed as per the normal flow from this point
   

##########
File path: pkg/common/resources/resources.go
##########
@@ -403,6 +403,13 @@ func FitIn(larger, smaller *Resource) bool {
 	return larger.fitIn(smaller, false)
 }
 
+// Check if smaller fits in larger
+// Types not defined in the larger resource are considered 0 values for Quantity
+// A nil resource is treated as max resource
+func FitInUndef(larger, smaller *Resource) bool {

Review comment:
       This is the same as calling `larger.FitInMaxUndef(smaller)` I don't think we need a new function for that.

##########
File path: pkg/scheduler/context.go
##########
@@ -844,3 +888,16 @@ func (cc *ClusterContext) GetNode(nodeID, partitionName string) *objects.Node {
 	}
 	return partition.GetNode(nodeID)
 }
+
+func (cc *ClusterContext) checkForUnlimitedNodes() bool {

Review comment:
       This check should be per partition. A node is linked to one partition. The call for this check must be after the partition retrieval above, see previous comment 




-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org