From 83509a092f388405b381ce8f099211d101c48865 Mon Sep 17 00:00:00 2001 From: Michael Taufen Date: Tue, 15 May 2018 16:08:46 -0700 Subject: [PATCH] Refactor test utils that deal with Kubelet metrics for clarity I found these functions hard to understand, because the names did not accurately reflect their behavior. For example, GetKubeletMetrics assumed that all of the metrics passed in were measuring latency. The caller of GetKubeletMetrics was implicitly making this assumption, but it was not obvious at the call site. --- test/e2e/framework/kubelet_stats.go | 23 +++++++++++++---------- test/e2e_node/eviction_test.go | 6 +++--- test/e2e_node/util.go | 10 ++++++---- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/test/e2e/framework/kubelet_stats.go b/test/e2e/framework/kubelet_stats.go index 7bc06edde8..1304cbfcf5 100644 --- a/test/e2e/framework/kubelet_stats.go +++ b/test/e2e/framework/kubelet_stats.go @@ -97,10 +97,11 @@ func getKubeletMetrics(c clientset.Interface, nodeName string) (metrics.KubeletM return kubeletMetrics, nil } -// GetKubeletLatencyMetrics gets all latency related kubelet metrics. Note that the KubeletMetrcis -// passed in should not contain subsystem prefix. -func GetKubeletLatencyMetrics(ms metrics.KubeletMetrics) KubeletLatencyMetrics { - latencyMethods := sets.NewString( +// GetDefaultKubeletLatencyMetrics calls GetKubeletLatencyMetrics with a set of default metricNames +// identifying common latency metrics. +// Note that the KubeletMetrics passed in should not contain subsystem prefix. +func GetDefaultKubeletLatencyMetrics(ms metrics.KubeletMetrics) KubeletLatencyMetrics { + latencyMetricNames := sets.NewString( kubeletmetrics.PodWorkerLatencyKey, kubeletmetrics.PodWorkerStartLatencyKey, kubeletmetrics.PodStartLatencyKey, @@ -109,13 +110,15 @@ func GetKubeletLatencyMetrics(ms metrics.KubeletMetrics) KubeletLatencyMetrics { kubeletmetrics.PodWorkerStartLatencyKey, kubeletmetrics.PLEGRelistLatencyKey, ) - return GetKubeletMetrics(ms, latencyMethods) + return GetKubeletLatencyMetrics(ms, latencyMetricNames) } -func GetKubeletMetrics(ms metrics.KubeletMetrics, methods sets.String) KubeletLatencyMetrics { +// GetKubeletLatencyMetrics filters ms to include only those contained in the metricNames set, +// then constructs a KubeletLatencyMetrics list based on the samples associated with those metrics. +func GetKubeletLatencyMetrics(ms metrics.KubeletMetrics, filterMetricNames sets.String) KubeletLatencyMetrics { var latencyMetrics KubeletLatencyMetrics - for method, samples := range ms { - if !methods.Has(method) { + for name, samples := range ms { + if !filterMetricNames.Has(name) { continue } for _, sample := range samples { @@ -131,7 +134,7 @@ func GetKubeletMetrics(ms metrics.KubeletMetrics, methods sets.String) KubeletLa latencyMetrics = append(latencyMetrics, KubeletLatencyMetric{ Operation: operation, - Method: method, + Method: name, Quantile: quantile, Latency: time.Duration(int64(latency)) * time.Microsecond, }) @@ -265,7 +268,7 @@ func HighLatencyKubeletOperations(c clientset.Interface, threshold time.Duration if err != nil { return KubeletLatencyMetrics{}, err } - latencyMetrics := GetKubeletLatencyMetrics(ms) + latencyMetrics := GetDefaultKubeletLatencyMetrics(ms) sort.Sort(latencyMetrics) var badMetrics KubeletLatencyMetrics logFunc("\nLatency metrics for node %v", nodeName) diff --git a/test/e2e_node/eviction_test.go b/test/e2e_node/eviction_test.go index fb59ca781f..c93d54dbb3 100644 --- a/test/e2e_node/eviction_test.go +++ b/test/e2e_node/eviction_test.go @@ -411,7 +411,7 @@ func runEvictionTest(f *framework.Framework, pressureTimeout time.Duration, expe framework.Logf("Node does NOT have %s", expectedNodeCondition) } } - logKubeletMetrics(kubeletmetrics.EvictionStatsAgeKey) + logKubeletLatencyMetrics(kubeletmetrics.EvictionStatsAgeKey) logFunc() return verifyEvictionOrdering(f, testSpecs) }, pressureTimeout, evictionPollInterval).Should(BeNil()) @@ -425,7 +425,7 @@ func runEvictionTest(f *framework.Framework, pressureTimeout time.Duration, expe By(fmt.Sprintf("Waiting for NodeCondition: %s to no longer exist on the node", expectedNodeCondition)) Eventually(func() error { logFunc() - logKubeletMetrics(kubeletmetrics.EvictionStatsAgeKey) + logKubeletLatencyMetrics(kubeletmetrics.EvictionStatsAgeKey) if expectedNodeCondition != noPressure && hasNodeCondition(f, expectedNodeCondition) { return fmt.Errorf("Conditions havent returned to normal, node still has %s", expectedNodeCondition) } @@ -438,7 +438,7 @@ func runEvictionTest(f *framework.Framework, pressureTimeout time.Duration, expe return fmt.Errorf("%s dissappeared and then reappeared", expectedNodeCondition) } logFunc() - logKubeletMetrics(kubeletmetrics.EvictionStatsAgeKey) + logKubeletLatencyMetrics(kubeletmetrics.EvictionStatsAgeKey) return verifyEvictionOrdering(f, testSpecs) }, postTestConditionMonitoringPeriod, evictionPollInterval).Should(BeNil()) }) diff --git a/test/e2e_node/util.go b/test/e2e_node/util.go index 4df85bcdeb..52ec5a90f4 100644 --- a/test/e2e_node/util.go +++ b/test/e2e_node/util.go @@ -331,17 +331,19 @@ func getLocalNode(f *framework.Framework) *apiv1.Node { return &nodeList.Items[0] } -// logs prometheus metrics from the local kubelet. -func logKubeletMetrics(metricKeys ...string) { +// logKubeletLatencyMetrics logs KubeletLatencyMetrics computed from the Prometheus +// metrics exposed on the current node and identified by the metricNames. +// The Kubelet subsystem prefix is automatically prepended to these metric names. +func logKubeletLatencyMetrics(metricNames ...string) { metricSet := sets.NewString() - for _, key := range metricKeys { + for _, key := range metricNames { metricSet.Insert(kubeletmetrics.KubeletSubsystem + "_" + key) } metric, err := metrics.GrabKubeletMetricsWithoutProxy(framework.TestContext.NodeName + ":10255") if err != nil { framework.Logf("Error getting kubelet metrics: %v", err) } else { - framework.Logf("Kubelet Metrics: %+v", framework.GetKubeletMetrics(metric, metricSet)) + framework.Logf("Kubelet Metrics: %+v", framework.GetKubeletLatencyMetrics(metric, metricSet)) } }