From c39a3bd5858c69c1b530c19224453dab53dc780e Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Tue, 21 Sep 2021 18:10:33 +0400 Subject: [PATCH 1/3] fix: avoid panic because of VPA objects without target ref Signed-off-by: m.nabokikh --- internal/store/verticalpodautoscaler.go | 8 +++- internal/store/verticalpodautoscaler_test.go | 40 ++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/internal/store/verticalpodautoscaler.go b/internal/store/verticalpodautoscaler.go index 90d3fe8daa..faf3ad5769 100644 --- a/internal/store/verticalpodautoscaler.go +++ b/internal/store/verticalpodautoscaler.go @@ -274,9 +274,15 @@ func vpaResourcesToMetrics(containerName string, resources v1.ResourceList) []*m func wrapVPAFunc(f func(*autoscaling.VerticalPodAutoscaler) *metric.Family) func(interface{}) *metric.Family { return func(obj interface{}) *metric.Family { vpa := obj.(*autoscaling.VerticalPodAutoscaler) + targetRef := vpa.Spec.TargetRef + + // targetRef was not a mandatory field, which can lead to a nil pointer exception here. + // Since it is pointless to have a VPA object without target ref, skip exporting metrics. + if targetRef == nil { + return &metric.Family{} + } metricFamily := f(vpa) - targetRef := vpa.Spec.TargetRef for _, m := range metricFamily.Metrics { m.LabelKeys = append(descVerticalPodAutoscalerLabelsDefaultLabels, m.LabelKeys...) diff --git a/internal/store/verticalpodautoscaler_test.go b/internal/store/verticalpodautoscaler_test.go index 481c689ff3..b05a4ec34f 100644 --- a/internal/store/verticalpodautoscaler_test.go +++ b/internal/store/verticalpodautoscaler_test.go @@ -131,6 +131,46 @@ func TestVPAStore(t *testing.T) { "kube_verticalpodautoscaler_status_recommendation_containerrecommendations_uncappedtarget", }, }, + { + Obj: &autoscaling.VerticalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 2, + Name: "vpa-without-target-ref", + Namespace: "ns2", + Labels: map[string]string{ + "app": "foobar", + }, + }, + Spec: autoscaling.VerticalPodAutoscalerSpec{ + UpdatePolicy: &autoscaling.PodUpdatePolicy{ + UpdateMode: &updateMode, + }, + ResourcePolicy: &autoscaling.PodResourcePolicy{ + ContainerPolicies: []autoscaling.ContainerResourcePolicy{ + { + ContainerName: "*", + MinAllowed: v1Resource("1", "4Gi"), + MaxAllowed: v1Resource("4", "8Gi"), + }, + }, + }, + }, + Status: autoscaling.VerticalPodAutoscalerStatus{ + Recommendation: &autoscaling.RecommendedPodResources{ + ContainerRecommendations: []autoscaling.RecommendedContainerResources{ + { + ContainerName: "container1", + LowerBound: v1Resource("1", "4Gi"), + UpperBound: v1Resource("4", "8Gi"), + Target: v1Resource("3", "7Gi"), + UncappedTarget: v1Resource("6", "10Gi"), + }, + }, + }, + }, + }, + Want: metadata, + }, } for i, c := range cases { c.Func = generator.ComposeMetricGenFuncs(vpaMetricFamilies(nil, nil)) From 8579a68e2f987f82c838f9d0059f7bdf8aea5b58 Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Wed, 22 Sep 2021 15:42:58 +0400 Subject: [PATCH 2/3] fix: expose metrics for VPA objects with empty target refs Signed-off-by: m.nabokikh --- internal/store/verticalpodautoscaler.go | 11 ++++--- internal/store/verticalpodautoscaler_test.go | 30 +++++++++++++++++++- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/internal/store/verticalpodautoscaler.go b/internal/store/verticalpodautoscaler.go index faf3ad5769..bacf187bc2 100644 --- a/internal/store/verticalpodautoscaler.go +++ b/internal/store/verticalpodautoscaler.go @@ -19,6 +19,7 @@ package store import ( "context" + k8sautoscaling "k8s.io/api/autoscaling/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -274,16 +275,18 @@ func vpaResourcesToMetrics(containerName string, resources v1.ResourceList) []*m func wrapVPAFunc(f func(*autoscaling.VerticalPodAutoscaler) *metric.Family) func(interface{}) *metric.Family { return func(obj interface{}) *metric.Family { vpa := obj.(*autoscaling.VerticalPodAutoscaler) + + metricFamily := f(vpa) targetRef := vpa.Spec.TargetRef // targetRef was not a mandatory field, which can lead to a nil pointer exception here. - // Since it is pointless to have a VPA object without target ref, skip exporting metrics. + // However, we still want to expose metrics to be able: + // * to alert about VPA objects without target refs + // * to count the right amount of VPA objects in a cluster if targetRef == nil { - return &metric.Family{} + targetRef = &k8sautoscaling.CrossVersionObjectReference{} } - metricFamily := f(vpa) - for _, m := range metricFamily.Metrics { m.LabelKeys = append(descVerticalPodAutoscalerLabelsDefaultLabels, m.LabelKeys...) m.LabelValues = append([]string{vpa.Namespace, vpa.Name, targetRef.APIVersion, targetRef.Kind, targetRef.Name}, m.LabelValues...) diff --git a/internal/store/verticalpodautoscaler_test.go b/internal/store/verticalpodautoscaler_test.go index b05a4ec34f..9e0ace49f2 100644 --- a/internal/store/verticalpodautoscaler_test.go +++ b/internal/store/verticalpodautoscaler_test.go @@ -169,7 +169,35 @@ func TestVPAStore(t *testing.T) { }, }, }, - Want: metadata, + Want: metadata + ` + kube_verticalpodautoscaler_spec_resourcepolicy_container_policies_maxallowed{container="*",namespace="ns2",resource="cpu",target_api_version="",target_kind="",target_name="",unit="core",verticalpodautoscaler="vpa-without-target-ref"} 4 + kube_verticalpodautoscaler_spec_resourcepolicy_container_policies_maxallowed{container="*",namespace="ns2",resource="memory",target_api_version="",target_kind="",target_name="",unit="byte",verticalpodautoscaler="vpa-without-target-ref"} 8.589934592e+09 + kube_verticalpodautoscaler_spec_resourcepolicy_container_policies_minallowed{container="*",namespace="ns2",resource="cpu",target_api_version="",target_kind="",target_name="",unit="core",verticalpodautoscaler="vpa-without-target-ref"} 1 + kube_verticalpodautoscaler_spec_resourcepolicy_container_policies_minallowed{container="*",namespace="ns2",resource="memory",target_api_version="",target_kind="",target_name="",unit="byte",verticalpodautoscaler="vpa-without-target-ref"} 4.294967296e+09 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_lowerbound{container="container1",namespace="ns2",resource="cpu",target_api_version="",target_kind="",target_name="",unit="core",verticalpodautoscaler="vpa-without-target-ref"} 1 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_lowerbound{container="container1",namespace="ns2",resource="memory",target_api_version="",target_kind="",target_name="",unit="byte",verticalpodautoscaler="vpa-without-target-ref"} 4.294967296e+09 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_target{container="container1",namespace="ns2",resource="cpu",target_api_version="",target_kind="",target_name="",unit="core",verticalpodautoscaler="vpa-without-target-ref"} 3 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_target{container="container1",namespace="ns2",resource="memory",target_api_version="",target_kind="",target_name="",unit="byte",verticalpodautoscaler="vpa-without-target-ref"} 7.516192768e+09 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_uncappedtarget{container="container1",namespace="ns2",resource="cpu",target_api_version="",target_kind="",target_name="",unit="core",verticalpodautoscaler="vpa-without-target-ref"} 6 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_uncappedtarget{container="container1",namespace="ns2",resource="memory",target_api_version="",target_kind="",target_name="",unit="byte",verticalpodautoscaler="vpa-without-target-ref"} 1.073741824e+10 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_upperbound{container="container1",namespace="ns2",resource="cpu",target_api_version="",target_kind="",target_name="",unit="core",verticalpodautoscaler="vpa-without-target-ref"} 4 + kube_verticalpodautoscaler_status_recommendation_containerrecommendations_upperbound{container="container1",namespace="ns2",resource="memory",target_api_version="",target_kind="",target_name="",unit="byte",verticalpodautoscaler="vpa-without-target-ref"} 8.589934592e+09 + kube_verticalpodautoscaler_labels{namespace="ns2",target_api_version="",target_kind="",target_name="",verticalpodautoscaler="vpa-without-target-ref"} 1 + kube_verticalpodautoscaler_spec_updatepolicy_updatemode{namespace="ns2",target_api_version="",target_kind="",target_name="",update_mode="Auto",verticalpodautoscaler="vpa-without-target-ref"} 0 + kube_verticalpodautoscaler_spec_updatepolicy_updatemode{namespace="ns2",target_api_version="",target_kind="",target_name="",update_mode="Initial",verticalpodautoscaler="vpa-without-target-ref"} 0 + kube_verticalpodautoscaler_spec_updatepolicy_updatemode{namespace="ns2",target_api_version="",target_kind="",target_name="",update_mode="Off",verticalpodautoscaler="vpa-without-target-ref"} 0 + kube_verticalpodautoscaler_spec_updatepolicy_updatemode{namespace="ns2",target_api_version="",target_kind="",target_name="",update_mode="Recreate",verticalpodautoscaler="vpa-without-target-ref"} 1 + `, + MetricNames: []string{ + "kube_verticalpodautoscaler_labels", + "kube_verticalpodautoscaler_spec_updatepolicy_updatemode", + "kube_verticalpodautoscaler_spec_resourcepolicy_container_policies_minallowed", + "kube_verticalpodautoscaler_spec_resourcepolicy_container_policies_maxallowed", + "kube_verticalpodautoscaler_status_recommendation_containerrecommendations_lowerbound", + "kube_verticalpodautoscaler_status_recommendation_containerrecommendations_upperbound", + "kube_verticalpodautoscaler_status_recommendation_containerrecommendations_target", + "kube_verticalpodautoscaler_status_recommendation_containerrecommendations_uncappedtarget", + }, }, } for i, c := range cases { From 05b3d3b169097d18483276613bf2ff10c2438baa Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Wed, 22 Sep 2021 19:18:12 +0400 Subject: [PATCH 3/3] fix: rename k8sautoscaling import to autoscalingv1 Signed-off-by: m.nabokikh --- internal/store/verticalpodautoscaler.go | 4 ++-- internal/store/verticalpodautoscaler_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/store/verticalpodautoscaler.go b/internal/store/verticalpodautoscaler.go index bacf187bc2..433ddec1d1 100644 --- a/internal/store/verticalpodautoscaler.go +++ b/internal/store/verticalpodautoscaler.go @@ -19,7 +19,7 @@ package store import ( "context" - k8sautoscaling "k8s.io/api/autoscaling/v1" + autoscalingv1 "k8s.io/api/autoscaling/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -284,7 +284,7 @@ func wrapVPAFunc(f func(*autoscaling.VerticalPodAutoscaler) *metric.Family) func // * to alert about VPA objects without target refs // * to count the right amount of VPA objects in a cluster if targetRef == nil { - targetRef = &k8sautoscaling.CrossVersionObjectReference{} + targetRef = &autoscalingv1.CrossVersionObjectReference{} } for _, m := range metricFamily.Metrics { diff --git a/internal/store/verticalpodautoscaler_test.go b/internal/store/verticalpodautoscaler_test.go index 9e0ace49f2..82aa7eb5ae 100644 --- a/internal/store/verticalpodautoscaler_test.go +++ b/internal/store/verticalpodautoscaler_test.go @@ -19,7 +19,7 @@ package store import ( "testing" - k8sautoscaling "k8s.io/api/autoscaling/v1" + autoscalingv1 "k8s.io/api/autoscaling/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -69,7 +69,7 @@ func TestVPAStore(t *testing.T) { }, }, Spec: autoscaling.VerticalPodAutoscalerSpec{ - TargetRef: &k8sautoscaling.CrossVersionObjectReference{ + TargetRef: &autoscalingv1.CrossVersionObjectReference{ APIVersion: "apps/v1", Kind: "Deployment", Name: "deployment1",