[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #205: [YUNIKORN-326]Add rest API to retrieve cluster nodes resource utilization

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

kingamarton commented on a change in pull request #205:
URL: https://github.com/apache/incubator-yunikorn-core/pull/205#discussion_r498744191



##########
File path: pkg/webservice/dao/node_util.go
##########
@@ -0,0 +1,31 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package dao
+
+type NodesUtilDAOInfo struct {
+	ResourceType string             `json:"type"`
+	NodesUtil    []*NodeUtilDAOInfo `json:"utilization"`
+}
+
+type NodeUtilDAOInfo struct {
+	BucketID   string   `json:"bucketID"`

Review comment:
       I think we can use the bucketName as the ID, since that is will be unique as well. I don't think we need a separate id.

##########
File path: pkg/webservice/handlers.go
##########
@@ -263,6 +281,44 @@ func getNodeJSON(nodeInfo *cache.NodeInfo) *dao.NodeDAOInfo {
 	}
 }
 
+func getNodesUtilJSON(partition *cache.PartitionInfo, name string) *dao.NodesUtilDAOInfo {
+	mapResult := make([]int, 10)
+	mapName := make([][]string, 10)
+	var v float64
+	var nodeUtil []*dao.NodeUtilDAOInfo
+	for _, node := range partition.GetNodes() {
+		total, ok := node.GetCapacity().Resources[name]
+		if !ok {
+			total = 0
+		}
+		resourceAllocated, ok := node.GetAllocatedResource().Resources[name]
+		if !ok {
+			resourceAllocated = 0
+		}
+		if float64(total) > 0 {
+			v = float64(resourceAllocated) / float64(total)
+		} else {
+			v = float64(0)
+		}

Review comment:
       Float divisions are 0 safe in go. It will return + or - inf. If the total resource is 0, but we have some usage, 0 is returned, what am not sure it is OK, because that means there is some misconfiguration/error and we will return that we have no usage. I would rather return N/A or "Inf", because we cannot calculate it

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -443,3 +444,105 @@ func prepareSchedulerForConfigChange(t *testing.T) {
 		t.Fatalf("Error when load clusterInfo from config %v", err)
 	}
 }
+
+func TestGetNodesUtilJSON(t *testing.T) {
+	const configDefault = `
+partitions:
+  - name: default
+    queues:
+      - name: root
+        queues:
+        - name: default
+`
+	rmID := "Util"
+	policyGroup := "default-policy-group"
+	clusterInfo := cache.NewClusterInfo()
+
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	if _, err := cache.SetClusterInfoFromConfigFile(clusterInfo, rmID, policyGroup); err != nil {
+		t.Fatalf("Error when load clusterInfo from config %v", err)
+	}
+	assert.Equal(t, 1, len(clusterInfo.ListPartitions()))
+
+	// Check test partition
+	partitionName := "[" + rmID + "]default"
+	partition := clusterInfo.GetPartition(partitionName)
+	assert.Equal(t, partitionName, partition.Name)
+	// create test application
+	queueName := "root.default"
+	appInfo := cache.CreateNewApplicationInfo("appID-1", "default", queueName)
+	err := cache.AddNewApplicationForTest(partition, appInfo, true)
+	if err != nil {
+		t.Errorf("add application to partition should not have failed: %v", err)
+	}
+
+	memVal := resources.Quantity(1000)
+	coreVal := resources.Quantity(1000)
+	// create test nodes
+	node1 := cache.NewNodeForTest("node-1", resources.NewResourceFromMap(

Review comment:
       Please extract node-1 and node-2 in a variable, since there are used in multiple places

##########
File path: pkg/webservice/handlers.go
##########
@@ -263,6 +281,44 @@ func getNodeJSON(nodeInfo *cache.NodeInfo) *dao.NodeDAOInfo {
 	}
 }
 
+func getNodesUtilJSON(partition *cache.PartitionInfo, name string) *dao.NodesUtilDAOInfo {
+	mapResult := make([]int, 10)
+	mapName := make([][]string, 10)
+	var v float64
+	var nodeUtil []*dao.NodeUtilDAOInfo
+	for _, node := range partition.GetNodes() {
+		total, ok := node.GetCapacity().Resources[name]
+		if !ok {
+			total = 0
+		}
+		resourceAllocated, ok := node.GetAllocatedResource().Resources[name]
+		if !ok {
+			resourceAllocated = 0
+		}
+		if float64(total) > 0 {
+			v = float64(resourceAllocated) / float64(total)
+		} else {
+			v = float64(0)
+		}

Review comment:
       In resources.go file there is already a method for calculating absolute usage: func CalculateAbsUsedCapacity(capacity, used *Resource) *Resource please use that one if possible.

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -443,3 +444,105 @@ func prepareSchedulerForConfigChange(t *testing.T) {
 		t.Fatalf("Error when load clusterInfo from config %v", err)
 	}
 }
+
+func TestGetNodesUtilJSON(t *testing.T) {
+	const configDefault = `
+partitions:
+  - name: default
+    queues:
+      - name: root
+        queues:
+        - name: default
+`
+	rmID := "Util"
+	policyGroup := "default-policy-group"
+	clusterInfo := cache.NewClusterInfo()
+
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	if _, err := cache.SetClusterInfoFromConfigFile(clusterInfo, rmID, policyGroup); err != nil {
+		t.Fatalf("Error when load clusterInfo from config %v", err)
+	}
+	assert.Equal(t, 1, len(clusterInfo.ListPartitions()))
+
+	// Check test partition
+	partitionName := "[" + rmID + "]default"
+	partition := clusterInfo.GetPartition(partitionName)
+	assert.Equal(t, partitionName, partition.Name)
+	// create test application
+	queueName := "root.default"
+	appInfo := cache.CreateNewApplicationInfo("appID-1", "default", queueName)
+	err := cache.AddNewApplicationForTest(partition, appInfo, true)
+	if err != nil {
+		t.Errorf("add application to partition should not have failed: %v", err)
+	}
+
+	memVal := resources.Quantity(1000)
+	coreVal := resources.Quantity(1000)
+	// create test nodes
+	node1 := cache.NewNodeForTest("node-1", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))

Review comment:
       please use MEMORY and VCORE constants defined in resources.go

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -443,3 +444,105 @@ func prepareSchedulerForConfigChange(t *testing.T) {
 		t.Fatalf("Error when load clusterInfo from config %v", err)
 	}
 }
+
+func TestGetNodesUtilJSON(t *testing.T) {
+	const configDefault = `
+partitions:
+  - name: default
+    queues:
+      - name: root
+        queues:
+        - name: default
+`
+	rmID := "Util"
+	policyGroup := "default-policy-group"
+	clusterInfo := cache.NewClusterInfo()
+
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	if _, err := cache.SetClusterInfoFromConfigFile(clusterInfo, rmID, policyGroup); err != nil {
+		t.Fatalf("Error when load clusterInfo from config %v", err)
+	}
+	assert.Equal(t, 1, len(clusterInfo.ListPartitions()))
+
+	// Check test partition
+	partitionName := "[" + rmID + "]default"
+	partition := clusterInfo.GetPartition(partitionName)
+	assert.Equal(t, partitionName, partition.Name)
+	// create test application
+	queueName := "root.default"
+	appInfo := cache.CreateNewApplicationInfo("appID-1", "default", queueName)
+	err := cache.AddNewApplicationForTest(partition, appInfo, true)
+	if err != nil {
+		t.Errorf("add application to partition should not have failed: %v", err)
+	}
+
+	memVal := resources.Quantity(1000)
+	coreVal := resources.Quantity(1000)
+	// create test nodes
+	node1 := cache.NewNodeForTest("node-1", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))
+	node2 := cache.NewNodeForTest("node-2", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))

Review comment:
       please use MEMORY and VCORE constants

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -443,3 +444,105 @@ func prepareSchedulerForConfigChange(t *testing.T) {
 		t.Fatalf("Error when load clusterInfo from config %v", err)
 	}
 }
+
+func TestGetNodesUtilJSON(t *testing.T) {
+	const configDefault = `
+partitions:
+  - name: default
+    queues:
+      - name: root
+        queues:
+        - name: default
+`
+	rmID := "Util"
+	policyGroup := "default-policy-group"
+	clusterInfo := cache.NewClusterInfo()
+
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	if _, err := cache.SetClusterInfoFromConfigFile(clusterInfo, rmID, policyGroup); err != nil {
+		t.Fatalf("Error when load clusterInfo from config %v", err)
+	}
+	assert.Equal(t, 1, len(clusterInfo.ListPartitions()))
+
+	// Check test partition
+	partitionName := "[" + rmID + "]default"
+	partition := clusterInfo.GetPartition(partitionName)
+	assert.Equal(t, partitionName, partition.Name)
+	// create test application
+	queueName := "root.default"
+	appInfo := cache.CreateNewApplicationInfo("appID-1", "default", queueName)
+	err := cache.AddNewApplicationForTest(partition, appInfo, true)
+	if err != nil {
+		t.Errorf("add application to partition should not have failed: %v", err)
+	}
+
+	memVal := resources.Quantity(1000)
+	coreVal := resources.Quantity(1000)
+	// create test nodes
+	node1 := cache.NewNodeForTest("node-1", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))
+	node2 := cache.NewNodeForTest("node-2", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))
+	// create test allocations
+	resAlloc1 := &si.Resource{
+		Resources: map[string]*si.Quantity{
+			resources.MEMORY: {Value: 500},
+			resources.VCORE:  {Value: 300},
+		},
+	}
+	resAlloc2 := &si.Resource{
+		Resources: map[string]*si.Quantity{
+			resources.MEMORY: {Value: 300},
+			resources.VCORE:  {Value: 500},
+		},
+	}
+	alloc1 := &si.Allocation{
+		AllocationKey:    "alloc-1",
+		ResourcePerAlloc: resAlloc1,
+		QueueName:        queueName,
+		NodeID:           "node-1",
+		ApplicationID:    "appID-1",
+	}
+	alloc2 := &si.Allocation{
+		AllocationKey:    "alloc-2",
+		ResourcePerAlloc: resAlloc2,
+		QueueName:        queueName,
+		NodeID:           "node-2",
+		ApplicationID:    "appID-1",
+	}
+	alloc1.UUID = "alloc-1-uuid"
+	alloc2.UUID = "alloc-2-uuid"
+	allocs1 := []*si.Allocation{alloc1}
+	allocs2 := []*si.Allocation{alloc2}
+	// add allocation to nodes
+	err = cache.AddNewNodeForTest(partition, node1, allocs1)
+	if err != nil || partition.GetNode("node-1") == nil {
+		t.Fatalf("add node to partition should not have failed: %v", err)
+	}
+	err = cache.AddNewNodeForTest(partition, node2, allocs2)
+	if err != nil || partition.GetNode("node-2") == nil {
+		t.Fatalf("add node to partition should not have failed: %v", err)
+	}
+	// get nodes utilization
+	res1 := getNodesUtilJSON(partition, "memory")
+	res2 := getNodesUtilJSON(partition, "vcore")
+	resNon := getNodesUtilJSON(partition, "non-exist")
+	subres1 := res1.NodesUtil
+	subres2 := res2.NodesUtil
+	subresNon := resNon.NodesUtil
+
+	assert.Equal(t, res1.ResourceType, "memory")

Review comment:
       Use constant

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -443,3 +444,105 @@ func prepareSchedulerForConfigChange(t *testing.T) {
 		t.Fatalf("Error when load clusterInfo from config %v", err)
 	}
 }
+
+func TestGetNodesUtilJSON(t *testing.T) {
+	const configDefault = `
+partitions:
+  - name: default
+    queues:
+      - name: root
+        queues:
+        - name: default
+`
+	rmID := "Util"
+	policyGroup := "default-policy-group"
+	clusterInfo := cache.NewClusterInfo()
+
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	if _, err := cache.SetClusterInfoFromConfigFile(clusterInfo, rmID, policyGroup); err != nil {
+		t.Fatalf("Error when load clusterInfo from config %v", err)
+	}
+	assert.Equal(t, 1, len(clusterInfo.ListPartitions()))
+
+	// Check test partition
+	partitionName := "[" + rmID + "]default"
+	partition := clusterInfo.GetPartition(partitionName)
+	assert.Equal(t, partitionName, partition.Name)
+	// create test application
+	queueName := "root.default"
+	appInfo := cache.CreateNewApplicationInfo("appID-1", "default", queueName)
+	err := cache.AddNewApplicationForTest(partition, appInfo, true)
+	if err != nil {
+		t.Errorf("add application to partition should not have failed: %v", err)
+	}
+
+	memVal := resources.Quantity(1000)
+	coreVal := resources.Quantity(1000)
+	// create test nodes
+	node1 := cache.NewNodeForTest("node-1", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))
+	node2 := cache.NewNodeForTest("node-2", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))
+	// create test allocations
+	resAlloc1 := &si.Resource{
+		Resources: map[string]*si.Quantity{
+			resources.MEMORY: {Value: 500},
+			resources.VCORE:  {Value: 300},
+		},
+	}
+	resAlloc2 := &si.Resource{
+		Resources: map[string]*si.Quantity{
+			resources.MEMORY: {Value: 300},
+			resources.VCORE:  {Value: 500},
+		},
+	}
+	alloc1 := &si.Allocation{
+		AllocationKey:    "alloc-1",
+		ResourcePerAlloc: resAlloc1,
+		QueueName:        queueName,
+		NodeID:           "node-1",
+		ApplicationID:    "appID-1",
+	}
+	alloc2 := &si.Allocation{
+		AllocationKey:    "alloc-2",
+		ResourcePerAlloc: resAlloc2,
+		QueueName:        queueName,
+		NodeID:           "node-2",
+		ApplicationID:    "appID-1",
+	}
+	alloc1.UUID = "alloc-1-uuid"
+	alloc2.UUID = "alloc-2-uuid"
+	allocs1 := []*si.Allocation{alloc1}
+	allocs2 := []*si.Allocation{alloc2}
+	// add allocation to nodes
+	err = cache.AddNewNodeForTest(partition, node1, allocs1)
+	if err != nil || partition.GetNode("node-1") == nil {
+		t.Fatalf("add node to partition should not have failed: %v", err)
+	}
+	err = cache.AddNewNodeForTest(partition, node2, allocs2)
+	if err != nil || partition.GetNode("node-2") == nil {
+		t.Fatalf("add node to partition should not have failed: %v", err)
+	}
+	// get nodes utilization
+	res1 := getNodesUtilJSON(partition, "memory")
+	res2 := getNodesUtilJSON(partition, "vcore")
+	resNon := getNodesUtilJSON(partition, "non-exist")
+	subres1 := res1.NodesUtil
+	subres2 := res2.NodesUtil
+	subresNon := resNon.NodesUtil
+
+	assert.Equal(t, res1.ResourceType, "memory")
+	assert.Equal(t, subres1[2].NumOfNodes, int64(1))
+	assert.Equal(t, subres1[4].NumOfNodes, int64(1))
+	assert.Equal(t, subres1[2].NodeNames[0], "node-2")
+	assert.Equal(t, subres1[4].NodeNames[0], "node-1")
+
+	assert.Equal(t, res2.ResourceType, "vcore")

Review comment:
       Use constant

##########
File path: pkg/webservice/handlers_test.go
##########
@@ -443,3 +444,105 @@ func prepareSchedulerForConfigChange(t *testing.T) {
 		t.Fatalf("Error when load clusterInfo from config %v", err)
 	}
 }
+
+func TestGetNodesUtilJSON(t *testing.T) {
+	const configDefault = `
+partitions:
+  - name: default
+    queues:
+      - name: root
+        queues:
+        - name: default
+`
+	rmID := "Util"
+	policyGroup := "default-policy-group"
+	clusterInfo := cache.NewClusterInfo()
+
+	configs.MockSchedulerConfigByData([]byte(configDefault))
+	if _, err := cache.SetClusterInfoFromConfigFile(clusterInfo, rmID, policyGroup); err != nil {
+		t.Fatalf("Error when load clusterInfo from config %v", err)
+	}
+	assert.Equal(t, 1, len(clusterInfo.ListPartitions()))
+
+	// Check test partition
+	partitionName := "[" + rmID + "]default"
+	partition := clusterInfo.GetPartition(partitionName)
+	assert.Equal(t, partitionName, partition.Name)
+	// create test application
+	queueName := "root.default"
+	appInfo := cache.CreateNewApplicationInfo("appID-1", "default", queueName)
+	err := cache.AddNewApplicationForTest(partition, appInfo, true)
+	if err != nil {
+		t.Errorf("add application to partition should not have failed: %v", err)
+	}
+
+	memVal := resources.Quantity(1000)
+	coreVal := resources.Quantity(1000)
+	// create test nodes
+	node1 := cache.NewNodeForTest("node-1", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))
+	node2 := cache.NewNodeForTest("node-2", resources.NewResourceFromMap(
+		map[string]resources.Quantity{"memory": memVal, "vcore": coreVal}))
+	// create test allocations
+	resAlloc1 := &si.Resource{
+		Resources: map[string]*si.Quantity{
+			resources.MEMORY: {Value: 500},
+			resources.VCORE:  {Value: 300},
+		},
+	}
+	resAlloc2 := &si.Resource{
+		Resources: map[string]*si.Quantity{
+			resources.MEMORY: {Value: 300},
+			resources.VCORE:  {Value: 500},
+		},
+	}
+	alloc1 := &si.Allocation{
+		AllocationKey:    "alloc-1",
+		ResourcePerAlloc: resAlloc1,
+		QueueName:        queueName,
+		NodeID:           "node-1",
+		ApplicationID:    "appID-1",
+	}
+	alloc2 := &si.Allocation{
+		AllocationKey:    "alloc-2",
+		ResourcePerAlloc: resAlloc2,
+		QueueName:        queueName,
+		NodeID:           "node-2",
+		ApplicationID:    "appID-1",
+	}
+	alloc1.UUID = "alloc-1-uuid"
+	alloc2.UUID = "alloc-2-uuid"
+	allocs1 := []*si.Allocation{alloc1}
+	allocs2 := []*si.Allocation{alloc2}
+	// add allocation to nodes
+	err = cache.AddNewNodeForTest(partition, node1, allocs1)
+	if err != nil || partition.GetNode("node-1") == nil {
+		t.Fatalf("add node to partition should not have failed: %v", err)
+	}
+	err = cache.AddNewNodeForTest(partition, node2, allocs2)
+	if err != nil || partition.GetNode("node-2") == nil {
+		t.Fatalf("add node to partition should not have failed: %v", err)
+	}
+	// get nodes utilization
+	res1 := getNodesUtilJSON(partition, "memory")

Review comment:
       Please use MEMORY and VCORE constants




----------------------------------------------------------------
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.

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