From 8e4662b26cb0b77000b40028afdc0b372affa3c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Pinho?= Date: Thu, 30 Jan 2020 10:33:15 +0000 Subject: [PATCH] Permit disregarding incompatible HPAs (#95) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * This commit adds a --disregard-incompatible-hpas that makes the HPA provider stop erroring out when a collector cannot be created for a metric in a HPA. Useful when kube-metrics-adapter runs alongside another metrics provider. Fixes issue #94. Signed-off-by: Tomás Pinho * Make tests pass Signed-off-by: Tomás Pinho * Wraps the Plugin Not Found error in a new type that can be checked by the caller of a function to determine if its contents should be logged or added as an event to the HPA, when this HPA is incompatible. The disregardIncompatibleHPAs is now targetting only the log or addition of the same event. Signed-off-by: Tomás Pinho * Invert if expression to select when we should log CreateNewMetricsCollector errors: don't log when both conditions are true - it's not a PluginNotFoundError and disregardIncompatibleHPAs flag is set to true. This way, if an error is NOT PluginNotFoundError it will always be logged, and when it IS PluginNotFoundError it will only be logged when disregardIncompatibleHPAs is false. Signed-off-by: Tomás Pinho * Remove redundant "whether to" Signed-off-by: Tomás Pinho * Add test case for updating HPAs via HPA Provider while disregarding incompatible HPAs. Signed-off-by: Tomás Pinho --- pkg/collector/collector.go | 10 +++++++- pkg/provider/hpa.go | 44 +++++++++++++++++++-------------- pkg/provider/hpa_test.go | 50 +++++++++++++++++++++++++++++++++++++- pkg/server/start.go | 8 ++++-- 4 files changed, 90 insertions(+), 22 deletions(-) diff --git a/pkg/collector/collector.go b/pkg/collector/collector.go index d6f1473..f6d886c 100644 --- a/pkg/collector/collector.go +++ b/pkg/collector/collector.go @@ -46,6 +46,14 @@ type CollectorPlugin interface { NewCollector(hpa *autoscalingv2.HorizontalPodAutoscaler, config *MetricConfig, interval time.Duration) (Collector, error) } +type PluginNotFoundError struct { + metricTypeName MetricTypeName +} + +func (p *PluginNotFoundError) Error() string { + return fmt.Sprintf("no plugin found for %s", p.metricTypeName) +} + func (c *CollectorFactory) RegisterPodsCollector(metricCollector string, plugin CollectorPlugin) error { if metricCollector == "" { c.podsPlugins.Any = plugin @@ -139,7 +147,7 @@ func (c *CollectorFactory) NewCollector(hpa *autoscalingv2.HorizontalPodAutoscal } } - return nil, fmt.Errorf("no plugin found for %s", config.MetricTypeName) + return nil, &PluginNotFoundError{metricTypeName: config.MetricTypeName} } type MetricTypeName struct { diff --git a/pkg/provider/hpa.go b/pkg/provider/hpa.go index 691beb0..92b4645 100644 --- a/pkg/provider/hpa.go +++ b/pkg/provider/hpa.go @@ -2,6 +2,7 @@ package provider import ( "context" + "errors" "reflect" "sync" "time" @@ -49,16 +50,17 @@ var ( // HPAProvider is a base provider for initializing metric collectors based on // HPA resources. type HPAProvider struct { - client kubernetes.Interface - interval time.Duration - collectorScheduler *CollectorScheduler - collectorInterval time.Duration - metricSink chan metricCollection - hpaCache map[resourceReference]autoscalingv2.HorizontalPodAutoscaler - metricStore *MetricStore - collectorFactory *collector.CollectorFactory - recorder kube_record.EventRecorder - logger *log.Entry + client kubernetes.Interface + interval time.Duration + collectorScheduler *CollectorScheduler + collectorInterval time.Duration + metricSink chan metricCollection + hpaCache map[resourceReference]autoscalingv2.HorizontalPodAutoscaler + metricStore *MetricStore + collectorFactory *collector.CollectorFactory + recorder kube_record.EventRecorder + logger *log.Entry + disregardIncompatibleHPAs bool } // metricCollection is a container for sending collected metrics across a @@ -69,7 +71,7 @@ type metricCollection struct { } // NewHPAProvider initializes a new HPAProvider. -func NewHPAProvider(client kubernetes.Interface, interval, collectorInterval time.Duration, collectorFactory *collector.CollectorFactory) *HPAProvider { +func NewHPAProvider(client kubernetes.Interface, interval, collectorInterval time.Duration, collectorFactory *collector.CollectorFactory, disregardIncompatibleHPAs bool) *HPAProvider { metricsc := make(chan metricCollection) return &HPAProvider{ @@ -80,9 +82,10 @@ func NewHPAProvider(client kubernetes.Interface, interval, collectorInterval tim metricStore: NewMetricStore(func() time.Time { return time.Now().UTC().Add(15 * time.Minute) }), - collectorFactory: collectorFactory, - recorder: recorder.CreateEventRecorder(client), - logger: log.WithFields(log.Fields{"provider": "hpa"}), + collectorFactory: collectorFactory, + recorder: recorder.CreateEventRecorder(client), + logger: log.WithFields(log.Fields{"provider": "hpa"}), + disregardIncompatibleHPAs: disregardIncompatibleHPAs, } } @@ -154,15 +157,20 @@ func (p *HPAProvider) updateHPAs() error { interval = p.collectorInterval } - collector, err := p.collectorFactory.NewCollector(&hpa, config, interval) + c, err := p.collectorFactory.NewCollector(&hpa, config, interval) if err != nil { - p.recorder.Eventf(&hpa, apiv1.EventTypeWarning, "CreateNewMetricsCollector", "Failed to create new metrics collector: %v", err) + + // Only log when it's not a PluginNotFoundError AND flag disregardIncompatibleHPAs is true + if !(errors.Is(err, &collector.PluginNotFoundError{}) && p.disregardIncompatibleHPAs) { + p.recorder.Eventf(&hpa, apiv1.EventTypeWarning, "CreateNewMetricsCollector", "Failed to create new metrics collector: %v", err) + } + cache = false continue } - p.logger.Infof("Adding new metrics collector: %T", collector) - p.collectorScheduler.Add(resourceRef, config.MetricTypeName, collector) + p.logger.Infof("Adding new metrics collector: %T", c) + p.collectorScheduler.Add(resourceRef, config.MetricTypeName, c) } newHPAs++ diff --git a/pkg/provider/hpa_test.go b/pkg/provider/hpa_test.go index 9032721..b3ec38c 100644 --- a/pkg/provider/hpa_test.go +++ b/pkg/provider/hpa_test.go @@ -77,7 +77,7 @@ func TestUpdateHPAs(t *testing.T) { err = collectorFactory.RegisterPodsCollector("", mockCollectorPlugin{}) require.NoError(t, err) - provider := NewHPAProvider(fakeClient, 1*time.Second, 1*time.Second, collectorFactory) + provider := NewHPAProvider(fakeClient, 1*time.Second, 1*time.Second, collectorFactory, false) provider.collectorScheduler = NewCollectorScheduler(context.Background(), provider.metricSink) err = provider.updateHPAs() @@ -94,3 +94,51 @@ func TestUpdateHPAs(t *testing.T) { require.Len(t, provider.collectorScheduler.table, 1) } + +func TestUpdateHPAsDisregardingIncompatibleHPA(t *testing.T) { + // Test HPAProvider with disregardIncompatibleHPAs = true + + value := resource.MustParse("1k") + + hpa := &autoscalingv1.HorizontalPodAutoscaler{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hpa1", + Namespace: "default", + Annotations: map[string]string{}, + }, + Spec: autoscalingv1.HorizontalPodAutoscalerSpec{ + ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{ + Kind: "Deployment", + Name: "app", + APIVersion: "apps/v1", + }, + MinReplicas: &[]int32{1}[0], + MaxReplicas: 10, + Metrics: []autoscalingv1.MetricSpec{ + { + Type: autoscalingv1.ExternalMetricSourceType, + External: &autoscalingv1.ExternalMetricSource{ + MetricName: "some-other-metric", + TargetAverageValue: &value, + }, + }, + }, + }, + } + + fakeClient := fake.NewSimpleClientset() + + var err error + _, err = fakeClient.AutoscalingV2beta1().HorizontalPodAutoscalers("default").Create(hpa) + require.NoError(t, err) + + collectorFactory := collector.NewCollectorFactory() + err = collectorFactory.RegisterPodsCollector("", mockCollectorPlugin{}) + require.NoError(t, err) + + provider := NewHPAProvider(fakeClient, 1*time.Second, 1*time.Second, collectorFactory, true) + provider.collectorScheduler = NewCollectorScheduler(context.Background(), provider.metricSink) + + err = provider.updateHPAs() + require.NoError(t, err) +} diff --git a/pkg/server/start.go b/pkg/server/start.go index ca41013..be081a9 100644 --- a/pkg/server/start.go +++ b/pkg/server/start.go @@ -110,7 +110,8 @@ func NewCommandStartAdapterServer(stopCh <-chan struct{}) *cobra.Command { "whether to enable AWS external metrics") flags.StringSliceVar(&o.AWSRegions, "aws-region", o.AWSRegions, "the AWS regions which should be monitored. eg: eu-central, eu-west-1") flags.StringVar(&o.MetricsAddress, "metrics-address", o.MetricsAddress, "The address where to serve prometheus metrics") - + flags.BoolVar(&o.DisregardIncompatibleHPAs, "disregard-incompatible-hpas", o.DisregardIncompatibleHPAs, ""+ + "disregard failing to create collectors for incompatible HPAs") return cmd } @@ -228,7 +229,7 @@ func (o AdapterServerOptions) RunCustomMetricsAdapterServer(stopCh <-chan struct collectorFactory.RegisterExternalCollector([]string{collector.AWSSQSQueueLengthMetric}, collector.NewAWSCollectorPlugin(awsSessions)) } - hpaProvider := provider.NewHPAProvider(client, 30*time.Second, 1*time.Minute, collectorFactory) + hpaProvider := provider.NewHPAProvider(client, 30*time.Second, 1*time.Minute, collectorFactory, o.DisregardIncompatibleHPAs) go hpaProvider.Run(ctx) @@ -332,4 +333,7 @@ type AdapterServerOptions struct { MetricsAddress string // SkipperBackendWeightAnnotation is the annotation on the ingress indicating the backend weights SkipperBackendWeightAnnotation []string + // Whether to disregard failing to create collectors for incompatible HPAs - such as when using + // kube-metrics-adapter beside another Metrics Provider + DisregardIncompatibleHPAs bool }