Merge pull request #612 from knarayan/trimaran1

Trimaran minor bug fixes

Author: k8s-ci-robot

pkg/trimaran/collector.go

@@ -123,7 +123,7 @@ func checkSpecs(trimaranSpec *pluginConfig.TrimaranSpec) error {
 			metricProviderType == string(pluginConfig.Prometheus) ||
 			metricProviderType == string(pluginConfig.SignalFx)
 		if !validMetricProviderType {
-			return fmt.Errorf("invalid MetricProvider.Type, got %T", trimaranSpec.MetricProvider.Type)
+			return fmt.Errorf("invalid MetricProvider.Type, got %v", trimaranSpec.MetricProvider.Type)
 		}
 	}
 	return nil

pkg/trimaran/collector_test.go

@@ -64,6 +64,13 @@ var (
 			},
 		},
 	}
+
+	noWatcherResponseForNode = watcher.WatcherMetrics{
+		Window: watcher.Window{},
+		Data: watcher.Data{
+			NodeMetricsMap: map[string]watcher.NodeMetrics{},
+		},
+	}
 )
 
 func TestNewCollector(t *testing.T) {
@@ -72,6 +79,28 @@ func TestNewCollector(t *testing.T) {
 	assert.Nil(t, err)
 }
 
+func TestNewCollectorSpecs(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
+		bytes, err := json.Marshal(watcherResponse)
+		assert.Nil(t, err)
+		resp.Write(bytes)
+	}))
+	defer server.Close()
+
+	metricProvider := pluginConfig.MetricProviderSpec{
+		Type: "",
+	}
+	trimaranSpec := pluginConfig.TrimaranSpec{
+		WatcherAddress: "",
+		MetricProvider: metricProvider,
+	}
+
+	col, err := NewCollector(&trimaranSpec)
+	assert.Nil(t, col)
+	expectedErr := "invalid MetricProvider.Type, got " + string(metricProvider.Type)
+	assert.EqualError(t, err, expectedErr)
+}
+
 func TestGetAllMetrics(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
 		bytes, err := json.Marshal(watcherResponse)
@@ -127,7 +156,55 @@ func TestGetNodeMetrics(t *testing.T) {
 	assert.NotNil(t, collector)
 	assert.Nil(t, err)
 	nodeName := "node-1"
-	metrics, _ := collector.GetNodeMetrics(nodeName)
+	metrics, allMetrics := collector.GetNodeMetrics(nodeName)
 	expectedMetrics := watcherResponse.Data.NodeMetricsMap[nodeName].Metrics
 	assert.EqualValues(t, expectedMetrics, metrics)
+	expectedAllMetrics := &watcherResponse
+	assert.EqualValues(t, expectedAllMetrics, allMetrics)
+}
+
+func TestGetNodeMetricsNilForNode(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
+		bytes, err := json.Marshal(noWatcherResponseForNode)
+		assert.Nil(t, err)
+		resp.Write(bytes)
+	}))
+	defer server.Close()
+
+	trimaranSpec := pluginConfig.TrimaranSpec{
+		WatcherAddress: server.URL,
+	}
+	collector, err := NewCollector(&trimaranSpec)
+	assert.NotNil(t, collector)
+	assert.Nil(t, err)
+	nodeName := "node-1"
+	metrics, allMetrics := collector.GetNodeMetrics(nodeName)
+	expectedMetrics := noWatcherResponseForNode.Data.NodeMetricsMap[nodeName].Metrics
+	assert.EqualValues(t, expectedMetrics, metrics)
+	assert.NotNil(t, allMetrics)
+	expectedAllMetrics := &noWatcherResponseForNode
+	assert.EqualValues(t, expectedAllMetrics, allMetrics)
+}
+
+func TestNewCollectorLoadWatcher(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
+		bytes, err := json.Marshal(watcherResponse)
+		assert.Nil(t, err)
+		resp.Write(bytes)
+	}))
+	defer server.Close()
+
+	metricProvider := pluginConfig.MetricProviderSpec{
+		Type:    watcher.SignalFxClientName,
+		Address: server.URL,
+		Token:   "PWNED",
+	}
+	trimaranSpec := pluginConfig.TrimaranSpec{
+		WatcherAddress: "",
+		MetricProvider: metricProvider,
+	}
+
+	col, err := NewCollector(&trimaranSpec)
+	assert.NotNil(t, col)
+	assert.Nil(t, err)
 }

pkg/trimaran/resourcestats.go

@@ -198,15 +198,12 @@ func GetNodeRequestsAndLimits(podInfosOnNode []*framework.PodInfo, node *v1.Node
 			// make sure limits not less than requests
 			SetMaxLimits(requested, limits)
 		}
-		reqCpu := requested.MilliCPU
-		reqMem := requested.Memory
-		limitCpu := limits.MilliCPU
-		limitMem := limits.Memory
+
 		// accumulate
-		nodeRequest.MilliCPU += reqCpu
-		nodeRequest.Memory += reqMem
-		nodeLimit.MilliCPU += limitCpu
-		nodeLimit.Memory += limitMem
+		nodeRequest.MilliCPU += requested.MilliCPU
+		nodeRequest.Memory += requested.Memory
+		nodeLimit.MilliCPU += limits.MilliCPU
+		nodeLimit.Memory += limits.Memory
 	}
 	// cap requests by node capacity
 	setMin(&nodeRequest.MilliCPU, capCpu)

pkg/trimaran/resourcestats_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package trimaran
 
 import (
+	"math"
 	"reflect"
 	"strconv"
 	"testing"
@@ -375,6 +376,13 @@ func TestGetNodeRequestsAndLimits(t *testing.T) {
 
 	testNode := st.MakeNode().Name("test-node").Capacity(nr).Obj()
 
+	nr = map[v1.ResourceName]string{
+		v1.ResourceCPU:    "1600m",
+		v1.ResourceMemory: "6Ki",
+	}
+
+	testNodeWithLowNodeCapacity := st.MakeNode().Name("test-node").Capacity(nr).Obj()
+
 	var initCPUReq int64 = 100
 	var initMemReq int64 = 2048
 	contCPUReq := []int64{1000, 500}
@@ -406,6 +414,20 @@ func TestGetNodeRequestsAndLimits(t *testing.T) {
 	podRequests := GetResourceRequested(pod)
 	podLimits := GetResourceLimits(pod)
 
+	allocatableResources := testNodeWithLowNodeCapacity.Status.Allocatable
+	amCpu := allocatableResources[v1.ResourceCPU]
+	capCpu := amCpu.MilliValue()
+	amMem := allocatableResources[v1.ResourceMemory]
+	capMem := amMem.Value()
+
+	contCPULimit = []int64{1000, 400}
+
+	pod4 := getPodWithContainersAndOverhead(0, initCPUReq, initMemReq, contCPUReq, contMemReq)
+	pod4 = getPodWithLimits(pod4, initCPULimit, initMemLimit, contCPULimit, contMemLimit)
+
+	pod4Requests := GetResourceRequested(pod4)
+	pod4Limits := GetResourceLimits(pod4)
+
 	type args struct {
 		podsOnNode  []*framework.PodInfo
 		node        *v1.Node
@@ -485,12 +507,91 @@ func TestGetNodeRequestsAndLimits(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "test-2",
+			// Test case for Node with low capacity than the requested Pod limits
+			args: args{
+				podsOnNode:  []*framework.PodInfo{},
+				node:        testNodeWithLowNodeCapacity,
+				pod:         pod,
+				podRequests: podRequests,
+				podLimits:   podLimits,
+			},
+			want: &NodeRequestsAndLimits{
+				NodeRequest: &framework.Resource{
+					MilliCPU: int64(math.Min(float64(GetResourceRequested(pod).MilliCPU), float64(capCpu))),
+					Memory:   int64(math.Min(float64(GetResourceRequested(pod).Memory), float64(capMem))),
+				},
+				NodeLimit: &framework.Resource{
+					MilliCPU: int64(math.Max(float64(GetResourceRequested(pod).MilliCPU), float64(GetResourceLimits(pod).MilliCPU))),
+					Memory:   int64(math.Max(float64(GetResourceRequested(pod).Memory), float64(GetResourceLimits(pod).Memory))),
+				},
+				NodeRequestMinusPod: &framework.Resource{
+					MilliCPU: 0,
+					Memory:   0,
+				},
+				NodeLimitMinusPod: &framework.Resource{
+					MilliCPU: 0,
+					Memory:   0,
+				},
+				Nodecapacity: &framework.Resource{
+					MilliCPU: capCpu,
+					Memory:   capMem,
+				},
+			},
+		},
+		{
+			name: "test-3",
+			// Test case for Pod with more Requests than Limits
+			args: args{
+				podsOnNode:  []*framework.PodInfo{},
+				node:        testNodeWithLowNodeCapacity,
+				pod:         pod4,
+				podRequests: pod4Requests,
+				podLimits:   pod4Limits,
+			},
+			want: &NodeRequestsAndLimits{
+				NodeRequest: &framework.Resource{
+					MilliCPU: int64(math.Min(float64(GetResourceRequested(pod4).MilliCPU), float64(capCpu))),
+					Memory:   int64(math.Min(float64(GetResourceRequested(pod4).Memory), float64(capMem))),
+				},
+				NodeLimit: &framework.Resource{
+					MilliCPU: int64(math.Min(
+						math.Max(float64(GetResourceRequested(pod4).MilliCPU), float64(GetResourceLimits(pod4).MilliCPU)),
+						float64(capCpu))),
+					Memory: int64(math.Min(
+						math.Max(float64(GetResourceRequested(pod4).Memory), float64(GetResourceLimits(pod4).Memory)),
+						float64(capMem))),
+				},
+				NodeRequestMinusPod: &framework.Resource{
+					MilliCPU: 0,
+					Memory:   0,
+				},
+				NodeLimitMinusPod: &framework.Resource{
+					MilliCPU: 0,
+					Memory:   0,
+				},
+				Nodecapacity: &framework.Resource{
+					MilliCPU: capCpu,
+					Memory:   capMem,
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := GetNodeRequestsAndLimits(tt.args.podsOnNode, tt.args.node, tt.args.pod,
+			var got *NodeRequestsAndLimits
+			SetMaxLimits(tt.args.podRequests, tt.args.podLimits)
+			if got = GetNodeRequestsAndLimits(tt.args.podsOnNode, tt.args.node, tt.args.pod,
 				tt.args.podRequests, tt.args.podLimits); !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("GetNodeRequestsAndLimits() = %v, want %v", got, tt.want)
+				t.Errorf("GetNodeRequestsAndLimits(): got = {%+v, %+v, %+v, %+v, %+v}, want = {%+v, %+v, %+v, %+v, %+v}",
+					*got.NodeRequest, *got.NodeLimit, *got.NodeRequestMinusPod, *got.NodeLimitMinusPod, *got.Nodecapacity,
+					*tt.want.NodeRequest, *tt.want.NodeLimit, *tt.want.NodeRequestMinusPod, *tt.want.NodeLimitMinusPod, *tt.want.Nodecapacity)
+			}
+			// The below asserts should hold good for all test cases, ideally; "test-3" exercises this by setting NodeLimit less than NodeRequest
+			if tt.name == "test-3" {
+				assert.LessOrEqual(t, (*got.NodeRequest).MilliCPU, (*got.NodeLimit).MilliCPU)
+				assert.LessOrEqual(t, (*got.NodeRequest).Memory, (*got.NodeLimit).Memory)
 			}
 		})
 	}