From 5466badd90826d69f2744d01572ff3c2179f0be6 Mon Sep 17 00:00:00 2001 From: Anatolii Dutchak Date: Fri, 30 Apr 2021 16:03:57 +0300 Subject: [PATCH 1/5] Added pod.DeletionTimestamp condition check Signed-off-by: Anatolii Dutchak --- pkg/collector/pod_collector.go | 9 +++-- pkg/collector/pod_collector_test.go | 59 +++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/pkg/collector/pod_collector.go b/pkg/collector/pod_collector.go index e5cb2db..d4317ee 100644 --- a/pkg/collector/pod_collector.go +++ b/pkg/collector/pod_collector.go @@ -99,11 +99,14 @@ func (c *PodCollector) GetMetrics() ([]CollectedMetric, error) { isPodReady, podReadyAge := GetPodReadyAge(pod) if isPodReady { - if podReadyAge >= c.minPodReadyAge { - go c.getPodMetric(pod, ch, errCh) - } else { + if pod.DeletionTimestamp != nil { + skippedPodsCount++ + c.logger.Warnf("Skipping metrics collection for pod %s/%s because it is seems to be scheduled for termination (DeletionTimestamp: %s)", pod.Namespace, pod.Name, pod.DeletionTimestamp) + } else if podReadyAge < c.minPodReadyAge { skippedPodsCount++ c.logger.Warnf("Skipping metrics collection for pod %s/%s because it's ready age is %s and min-pod-ready-age is set to %s", pod.Namespace, pod.Name, podReadyAge, c.minPodReadyAge) + } else { + go c.getPodMetric(pod, ch, errCh) } } else { skippedPodsCount++ diff --git a/pkg/collector/pod_collector_test.go b/pkg/collector/pod_collector_test.go index c7bbe43..d35c10f 100644 --- a/pkg/collector/pod_collector_test.go +++ b/pkg/collector/pod_collector_test.go @@ -48,7 +48,8 @@ func TestPodCollector(t *testing.T) { lastReadyTransitionTimeTimestamp := v1.NewTime(time.Now().Add(time.Duration(-30) * time.Second)) minPodReadyAge := time.Duration(0 * time.Second) podCondition := corev1.PodCondition{Type: corev1.PodReady, Status: corev1.ConditionTrue, LastTransitionTime: lastReadyTransitionTimeTimestamp} - makeTestPods(t, host, port, "test-metric", client, 5, podCondition) + podDeletionTimestamp := time.Time{} + makeTestPods(t, host, port, "test-metric", client, 5, podCondition,podDeletionTimestamp) testHPA := makeTestHPA(t, client) testConfig := makeTestConfig(port, minPodReadyAge) collector, err := plugin.NewCollector(testHPA, testConfig, testInterval) @@ -87,7 +88,8 @@ func TestPodCollectorWithMinPodReadyAge(t *testing.T) { // Pods that are not older that 60 seconds (all in this case) should not be processed minPodReadyAge := time.Duration(60 * time.Second) podCondition := corev1.PodCondition{Type: corev1.PodReady, Status: corev1.ConditionTrue, LastTransitionTime: lastReadyTransitionTimeTimestamp} - makeTestPods(t, host, port, "test-metric", client, 5, podCondition) + podDeletionTimestamp := time.Time{} + makeTestPods(t, host, port, "test-metric", client, 5, podCondition, podDeletionTimestamp) testHPA := makeTestHPA(t, client) testConfig := makeTestConfig(port, minPodReadyAge) collector, err := plugin.NewCollector(testHPA, testConfig, testInterval) @@ -123,9 +125,50 @@ func TestPodCollectorWithPodCondition(t *testing.T) { host, port, metricsHandler := makeTestHTTPServer(t, tc.metrics) lastScheduledTransitionTimeTimestamp := v1.NewTime(time.Now().Add(time.Duration(-30) * time.Second)) minPodReadyAge := time.Duration(0 * time.Second) + podDeletionTimestamp := time.Time{} //Pods in state corev1.PodReady == corev1.ConditionFalse should not be processed podCondition := corev1.PodCondition{Type: corev1.PodReady, Status: corev1.ConditionFalse, LastTransitionTime: lastScheduledTransitionTimeTimestamp} - makeTestPods(t, host, port, "test-metric", client, 5, podCondition) + makeTestPods(t, host, port, "test-metric", client, 5, podCondition, podDeletionTimestamp) + testHPA := makeTestHPA(t, client) + testConfig := makeTestConfig(port, minPodReadyAge) + collector, err := plugin.NewCollector(testHPA, testConfig, testInterval) + require.NoError(t, err) + metrics, err := collector.GetMetrics() + require.NoError(t, err) + require.Equal(t, len(metrics), int(metricsHandler.calledCounter)) + var values []int64 + for _, m := range metrics { + values = append(values, m.Custom.Value.Value()) + } + require.ElementsMatch(t, tc.result, values) + }) + } +} + + +func TestPodCollectorWithPodTerminatingCondition(t *testing.T) { + for _, tc := range []struct { + name string + metrics [][]int64 + result []int64 + }{ + { + name: "simple-with-pod-condition", + metrics: [][]int64{{1}, {3}, {8}, {5}, {2}}, + result: []int64{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + client := fake.NewSimpleClientset() + plugin := NewPodCollectorPlugin(client) + makeTestDeployment(t, client) + host, port, metricsHandler := makeTestHTTPServer(t, tc.metrics) + lastScheduledTransitionTimeTimestamp := v1.NewTime(time.Now().Add(time.Duration(-30) * time.Second)) + minPodReadyAge := time.Duration(0 * time.Second) + //Pods with podDeletionTimestamp should not be processed + podDeletionTimestamp := time.Now() + podCondition := corev1.PodCondition{Type: corev1.PodReady, Status: corev1.ConditionTrue, LastTransitionTime: lastScheduledTransitionTimeTimestamp} + makeTestPods(t, host, port, "test-metric", client, 5, podCondition, podDeletionTimestamp) testHPA := makeTestHPA(t, client) testConfig := makeTestConfig(port, minPodReadyAge) collector, err := plugin.NewCollector(testHPA, testConfig, testInterval) @@ -183,7 +226,9 @@ func makeTestConfig(port string, minPodReadyAge time.Duration) *MetricConfig { } } -func makeTestPods(t *testing.T, testServer string, metricName string, port string, client kubernetes.Interface, replicas int, podCondition corev1.PodCondition) { +func makeTestPods(t *testing.T, testServer string, metricName string, port string, client kubernetes.Interface, replicas int, podCondition corev1.PodCondition, podDeletionTimestamp time.Time) { + + for i := 0; i < replicas; i++ { testPod := &corev1.Pod{ ObjectMeta: v1.ObjectMeta{ @@ -198,6 +243,12 @@ func makeTestPods(t *testing.T, testServer string, metricName string, port strin Conditions: []corev1.PodCondition{podCondition}, }, } + + if podDeletionTimestamp.IsZero(){ + testPod.ObjectMeta.DeletionTimestamp = nil + } else { + testPod.ObjectMeta.DeletionTimestamp = &v1.Time{Time: podDeletionTimestamp} + } _, err := client.CoreV1().Pods(testNamespace).Create(context.TODO(), testPod, v1.CreateOptions{}) require.NoError(t, err) } From b5432fb1f3574f525f54e1eb606a7999e86b766d Mon Sep 17 00:00:00 2001 From: Anatolii Dutchak Date: Fri, 30 Apr 2021 17:58:39 +0300 Subject: [PATCH 2/5] Fixed formatting Signed-off-by: Anatolii Dutchak --- pkg/collector/pod_collector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/collector/pod_collector.go b/pkg/collector/pod_collector.go index d4317ee..9852b09 100644 --- a/pkg/collector/pod_collector.go +++ b/pkg/collector/pod_collector.go @@ -106,7 +106,7 @@ func (c *PodCollector) GetMetrics() ([]CollectedMetric, error) { skippedPodsCount++ c.logger.Warnf("Skipping metrics collection for pod %s/%s because it's ready age is %s and min-pod-ready-age is set to %s", pod.Namespace, pod.Name, podReadyAge, c.minPodReadyAge) } else { - go c.getPodMetric(pod, ch, errCh) + go c.getPodMetric(pod, ch, errCh) } } else { skippedPodsCount++ From cf5872ef0879ac695e4d627066f4d673a57ebe8d Mon Sep 17 00:00:00 2001 From: Anatolii Dutchak Date: Fri, 30 Apr 2021 18:00:21 +0300 Subject: [PATCH 3/5] Fixed formatting Signed-off-by: Anatolii Dutchak --- pkg/collector/pod_collector_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/collector/pod_collector_test.go b/pkg/collector/pod_collector_test.go index d35c10f..c5a3661 100644 --- a/pkg/collector/pod_collector_test.go +++ b/pkg/collector/pod_collector_test.go @@ -227,8 +227,6 @@ func makeTestConfig(port string, minPodReadyAge time.Duration) *MetricConfig { } func makeTestPods(t *testing.T, testServer string, metricName string, port string, client kubernetes.Interface, replicas int, podCondition corev1.PodCondition, podDeletionTimestamp time.Time) { - - for i := 0; i < replicas; i++ { testPod := &corev1.Pod{ ObjectMeta: v1.ObjectMeta{ From 52bfbbb1b0fb24d3f1f172ce416d84c52d1fa785 Mon Sep 17 00:00:00 2001 From: Anatolii Dutchak Date: Fri, 30 Apr 2021 18:08:25 +0300 Subject: [PATCH 4/5] Goimported pod_collector_test.go Signed-off-by: Anatolii Dutchak --- pkg/collector/pod_collector_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/collector/pod_collector_test.go b/pkg/collector/pod_collector_test.go index c5a3661..b33e590 100644 --- a/pkg/collector/pod_collector_test.go +++ b/pkg/collector/pod_collector_test.go @@ -49,7 +49,7 @@ func TestPodCollector(t *testing.T) { minPodReadyAge := time.Duration(0 * time.Second) podCondition := corev1.PodCondition{Type: corev1.PodReady, Status: corev1.ConditionTrue, LastTransitionTime: lastReadyTransitionTimeTimestamp} podDeletionTimestamp := time.Time{} - makeTestPods(t, host, port, "test-metric", client, 5, podCondition,podDeletionTimestamp) + makeTestPods(t, host, port, "test-metric", client, 5, podCondition, podDeletionTimestamp) testHPA := makeTestHPA(t, client) testConfig := makeTestConfig(port, minPodReadyAge) collector, err := plugin.NewCollector(testHPA, testConfig, testInterval) @@ -145,7 +145,6 @@ func TestPodCollectorWithPodCondition(t *testing.T) { } } - func TestPodCollectorWithPodTerminatingCondition(t *testing.T) { for _, tc := range []struct { name string @@ -241,8 +240,8 @@ func makeTestPods(t *testing.T, testServer string, metricName string, port strin Conditions: []corev1.PodCondition{podCondition}, }, } - - if podDeletionTimestamp.IsZero(){ + + if podDeletionTimestamp.IsZero() { testPod.ObjectMeta.DeletionTimestamp = nil } else { testPod.ObjectMeta.DeletionTimestamp = &v1.Time{Time: podDeletionTimestamp} From e16bacb24e990e2b71088a87ddea9709ab7368ff Mon Sep 17 00:00:00 2001 From: Anatolii Dutchak Date: Wed, 5 May 2021 11:22:51 +0300 Subject: [PATCH 5/5] Changed message text and level Signed-off-by: Anatolii Dutchak --- pkg/collector/pod_collector.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/collector/pod_collector.go b/pkg/collector/pod_collector.go index 9852b09..bf4bd11 100644 --- a/pkg/collector/pod_collector.go +++ b/pkg/collector/pod_collector.go @@ -101,7 +101,7 @@ func (c *PodCollector) GetMetrics() ([]CollectedMetric, error) { if isPodReady { if pod.DeletionTimestamp != nil { skippedPodsCount++ - c.logger.Warnf("Skipping metrics collection for pod %s/%s because it is seems to be scheduled for termination (DeletionTimestamp: %s)", pod.Namespace, pod.Name, pod.DeletionTimestamp) + c.logger.Debugf("Skipping metrics collection for pod %s/%s because it is being terminated (DeletionTimestamp: %s)", pod.Namespace, pod.Name, pod.DeletionTimestamp) } else if podReadyAge < c.minPodReadyAge { skippedPodsCount++ c.logger.Warnf("Skipping metrics collection for pod %s/%s because it's ready age is %s and min-pod-ready-age is set to %s", pod.Namespace, pod.Name, podReadyAge, c.minPodReadyAge) @@ -110,7 +110,7 @@ func (c *PodCollector) GetMetrics() ([]CollectedMetric, error) { } } else { skippedPodsCount++ - c.logger.Warnf("Skipping metrics collection for pod %s/%s because it's status is not Ready.", pod.Namespace, pod.Name) + c.logger.Debugf("Skipping metrics collection for pod %s/%s because it's status is not Ready.", pod.Namespace, pod.Name) } }