Addressing reviev comments

Author: ingvagabund

README.md

@@ -353,10 +353,10 @@ profiles:
           "cpu" : 50
           "memory": 50
           "pods": 50
-        metricsUtilization:
-          source: KubernetesMetrics
-          # prometheus:
-          #   query: instance:node_cpu:rate:sum
+        # metricsUtilization:
+        #   source: Prometheus
+        #   prometheus:
+        #     query: instance:node_cpu:rate:sum
         evictionLimits:
           node: 5
     plugins:

pkg/api/types.go

@@ -142,7 +142,8 @@ type ReferencedResourceList = map[v1.ResourceName]*resource.Quantity
 type Prometheus struct {
 	URL string
 	// authToken used for authentication with the prometheus server.
-	// If not set the in cluster authentication token from the container's file system is read.
+	// If not set the in cluster authentication token for the descheduler service
+	// account is read from the container's file system is read.
 	AuthToken *AuthToken
 }
 

pkg/descheduler/descheduler.go

@@ -145,6 +145,14 @@ func (ir *informerResources) CopyTo(fakeClient *fakeclientset.Clientset, newFact
 	return nil
 }
 
+func metricsProviderListToMap(providersList []api.MetricsProvider) map[api.MetricsSource]*api.MetricsProvider {
+	providersMap := make(map[api.MetricsSource]*api.MetricsProvider)
+	for _, provider := range providersList {
+		providersMap[provider.Source] = &provider
+	}
+	return providersMap
+}
+
 func newDescheduler(ctx context.Context, rs *options.DeschedulerServer, deschedulerPolicy *api.DeschedulerPolicy, evictionPolicyGroupVersion string, eventRecorder events.EventRecorder, sharedInformerFactory, namespacedSharedInformerFactory informers.SharedInformerFactory) (*descheduler, error) {
 	podInformer := sharedInformerFactory.Core().V1().Pods().Informer()
 
@@ -195,11 +203,7 @@ func newDescheduler(ctx context.Context, rs *options.DeschedulerServer, deschedu
 		podEvictionReactionFnc: podEvictionReactionFnc,
 		prometheusClient:       rs.PrometheusClient,
 		queue:                  workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{Name: "descheduler"}),
-		metricsProviders:       make(map[api.MetricsSource]*api.MetricsProvider),
-	}
-
-	for _, provider := range deschedulerPolicy.MetricsProviders {
-		desch.metricsProviders[provider.Source] = &provider
+		metricsProviders:       metricsProviderListToMap(deschedulerPolicy.MetricsProviders),
 	}
 
 	if rs.MetricsClient != nil {
@@ -215,12 +219,16 @@ func newDescheduler(ctx context.Context, rs *options.DeschedulerServer, deschedu
 	}
 
 	prometheusProvider := desch.metricsProviders[api.PrometheusMetrics]
-	if namespacedSharedInformerFactory != nil && prometheusProvider != nil {
-		if prometheusProvider.Prometheus == nil || prometheusProvider.Prometheus.AuthToken == nil || prometheusProvider.Prometheus.AuthToken.SecretReference == nil {
+	if prometheusProvider != nil && prometheusProvider.Prometheus != nil && prometheusProvider.Prometheus.AuthToken != nil {
+		authTokenSecret := prometheusProvider.Prometheus.AuthToken.SecretReference
+		if authTokenSecret == nil || authTokenSecret.Namespace == "" {
 			return nil, fmt.Errorf("prometheus metrics source configuration is missing authentication token secret")
 		}
+		if namespacedSharedInformerFactory == nil {
+			return nil, fmt.Errorf("namespacedSharedInformerFactory not configured")
+		}
 		namespacedSharedInformerFactory.Core().V1().Secrets().Informer().AddEventHandler(desch.eventHandler())
-		desch.namespacedSecretsLister = namespacedSharedInformerFactory.Core().V1().Secrets().Lister().Secrets(desch.metricsProviders[api.PrometheusMetrics].Prometheus.AuthToken.SecretReference.Namespace)
+		desch.namespacedSecretsLister = namespacedSharedInformerFactory.Core().V1().Secrets().Lister().Secrets(authTokenSecret.Namespace)
 	}
 
 	return desch, nil
@@ -299,6 +307,10 @@ func (d *descheduler) sync() error {
 		// clear the token if the secret is not found
 		if apierrors.IsNotFound(err) {
 			d.currentPrometheusAuthToken = ""
+			if d.previousPrometheusClientTransport != nil {
+				d.previousPrometheusClientTransport.CloseIdleConnections()
+			}
+			d.previousPrometheusClientTransport = nil
 		}
 		return fmt.Errorf("unable to get %v/%v secret", ns, name)
 	}
@@ -474,14 +486,7 @@ func Run(ctx context.Context, rs *options.DeschedulerServer) error {
 		return err
 	}
 
-	hasKubernetesMetricsProvider := false
-	for _, provider := range deschedulerPolicy.MetricsProviders {
-		if provider.Source == api.KubernetesMetrics {
-			hasKubernetesMetricsProvider = true
-			break
-		}
-	}
-	if (deschedulerPolicy.MetricsCollector != nil && deschedulerPolicy.MetricsCollector.Enabled) || hasKubernetesMetricsProvider {
+	if (deschedulerPolicy.MetricsCollector != nil && deschedulerPolicy.MetricsCollector.Enabled) || metricsProviderListToMap(deschedulerPolicy.MetricsProviders)[api.KubernetesMetrics] != nil {
 		metricsClient, err := client.CreateMetricsClient(clientConnection, "descheduler")
 		if err != nil {
 			return err
@@ -594,23 +599,17 @@ func RunDeschedulerStrategies(ctx context.Context, rs *options.DeschedulerServer
 	defer eventBroadcaster.Shutdown()
 
 	var namespacedSharedInformerFactory informers.SharedInformerFactory
-	saTokenReconciliation := noReconciliation
-
-	var prometheusConfiguration *api.Prometheus
-	for _, provider := range deschedulerPolicy.MetricsProviders {
-		if provider.Source == api.PrometheusMetrics {
-			prometheusConfiguration = provider.Prometheus
-		}
-	}
+	metricProviderTokenReconciliation := noReconciliation
 
-	if prometheusConfiguration != nil && prometheusConfiguration.URL != "" {
-		if prometheusConfiguration.AuthToken != nil {
+	prometheusProvider := metricsProviderListToMap(deschedulerPolicy.MetricsProviders)[api.PrometheusMetrics]
+	if prometheusProvider != nil && prometheusProvider.Prometheus != nil && prometheusProvider.Prometheus.URL != "" {
+		if prometheusProvider.Prometheus.AuthToken != nil {
 			// Will get reconciled
-			namespacedSharedInformerFactory = informers.NewSharedInformerFactoryWithOptions(rs.Client, 0, informers.WithTransform(trimManagedFields), informers.WithNamespace(prometheusConfiguration.AuthToken.SecretReference.Namespace))
-			saTokenReconciliation = secretReconciliation
+			namespacedSharedInformerFactory = informers.NewSharedInformerFactoryWithOptions(rs.Client, 0, informers.WithTransform(trimManagedFields), informers.WithNamespace(prometheusProvider.Prometheus.AuthToken.SecretReference.Namespace))
+			metricProviderTokenReconciliation = secretReconciliation
 		} else {
 			// Use the sa token and assume it has the sufficient permissions to authenticate
-			saTokenReconciliation = inClusterReconciliation
+			metricProviderTokenReconciliation = inClusterReconciliation
 		}
 	}
 
@@ -623,13 +622,13 @@ func RunDeschedulerStrategies(ctx context.Context, rs *options.DeschedulerServer
 	defer cancel()
 
 	sharedInformerFactory.Start(ctx.Done())
-	if saTokenReconciliation == secretReconciliation {
+	if metricProviderTokenReconciliation == secretReconciliation {
 		namespacedSharedInformerFactory.Start(ctx.Done())
 	}
 
 	sharedInformerFactory.WaitForCacheSync(ctx.Done())
 	descheduler.podEvictor.WaitForEventHandlersSync(ctx)
-	if saTokenReconciliation == secretReconciliation {
+	if metricProviderTokenReconciliation == secretReconciliation {
 		namespacedSharedInformerFactory.WaitForCacheSync(ctx.Done())
 	}
 
@@ -647,12 +646,12 @@ func RunDeschedulerStrategies(ctx context.Context, rs *options.DeschedulerServer
 		}
 	}
 
-	if saTokenReconciliation == secretReconciliation {
+	if metricProviderTokenReconciliation == secretReconciliation {
 		go descheduler.runAuthenticationSecretReconciler(ctx)
 	}
 
 	wait.NonSlidingUntil(func() {
-		if saTokenReconciliation == inClusterReconciliation {
+		if metricProviderTokenReconciliation == inClusterReconciliation {
 			// Read the sa token and assume it has the sufficient permissions to authenticate
 			if err := descheduler.reconcileInClusterSAToken(); err != nil {
 				klog.ErrorS(err, "unable to reconcile an in cluster SA token")

pkg/framework/plugins/nodeutilization/lownodeutilization.go

@@ -107,19 +107,20 @@ func NewLowNodeUtilization(args runtime.Object, handle frameworktypes.Handle) (f
 	var usageClient usageClient
 	// MetricsServer is deprecated, removed once dropped
 	if metricsUtilization != nil {
-		if metricsUtilization.MetricsServer || metricsUtilization.Source == api.KubernetesMetrics {
+		switch {
+		case metricsUtilization.MetricsServer, metricsUtilization.Source == api.KubernetesMetrics:
 			if handle.MetricsCollector() == nil {
 				return nil, fmt.Errorf("metrics client not initialized")
 			}
 			usageClient = newActualUsageClient(resourceNames, handle.GetPodsAssignedToNodeFunc(), handle.MetricsCollector())
-		} else if metricsUtilization.Source == api.PrometheusMetrics {
+		case metricsUtilization.Source == api.PrometheusMetrics:
 			if handle.PrometheusClient() == nil {
 				return nil, fmt.Errorf("prometheus client not initialized")
 			}
 			usageClient = newPrometheusUsageClient(handle.GetPodsAssignedToNodeFunc(), handle.PrometheusClient(), metricsUtilization.Prometheus.Query)
-		} else if metricsUtilization.Source != "" {
+		case metricsUtilization.Source != "":
 			return nil, fmt.Errorf("unrecognized metrics source")
-		} else {
+		default:
 			return nil, fmt.Errorf("metrics source is empty")
 		}
 	} else {