refactor(promClientController): split it into two prom client controllers

[ingvagabund]

Description

The current prometheus client reconciler is serving two purposes which are not cleanly similar. E.g. secret based reconciliation is async, in cluster reconciliation sync. Also, secret reconciler requires more validation and more complex initialization. By dividing it into two both the testing and the overall interpretation of it gets simplified.

Checklist

Please ensure your pull request meets the following criteria before submitting
for review, these items will be used by reviewers to assess the quality and
completeness of your changes:

• Code Readability: Is the code easy to understand, well-structured, and consistent with project conventions?
• Naming Conventions: Are variable, function, and structs descriptive and consistent?
• Code Duplication: Is there any repeated code that should be refactored?
• Function/Method Size: Are functions/methods short and focused on a single task?
• Comments & Documentation: Are comments clear, useful, and not excessive? Were comments updated where necessary?
• Error Handling: Are errors handled appropriately ?
• Testing: Are there sufficient unit/integration tests?
• Performance: Are there any obvious performance issues or unnecessary computations?
• Dependencies: Are new dependencies justified ?
• Logging & Monitoring: Is logging used appropriately (not too verbose, not too silent)?
• Backward Compatibility: Does this change break any existing functionality or APIs?
• Resource Management: Are resources (files, connections, memory) managed and released properly?
• PR Description: Is the PR description clear, providing enough context and explaining the motivation for the change?
• Documentation & Changelog: Are README and docs updated if necessary?

[Copilot]

Pull request overview

Refactors Prometheus client handling in the descheduler by splitting the previous single controller into two dedicated controllers (in-cluster SA token vs. secret-based token), and updates the test suite to reflect the new wiring.

Changes:

• Introduces inClusterPromClientController and secretBasedPromClientController and updates descheduler bootstrap/run logic to use the appropriate controller.
• Adds helper functions (getPrometheusConfig, configureSecretPromClientReconciler) to decide which Prometheus mode to configure and when to start namespaced informers/reconcilers.
• Updates descheduler_test.go to use the new controllers and adds secret informer propagation waiting for secret-based scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
pkg/descheduler/descheduler.go	Splits Prometheus client controller logic into in-cluster vs secret-based controllers and updates bootstrap/run paths accordingly.
pkg/descheduler/descheduler_test.go	Updates tests for the split controllers; adds secret propagation waiting helper for informer-backed secret reads.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

configureSecretPromClientReconciler only checks AuthToken != nil, but the caller immediately dereferences prometheusConfig.AuthToken.SecretReference.Namespace. If AuthToken is set but SecretReference is nil/empty (possible in unit tests or if validation is bypassed), this will panic. Consider making configureSecretPromClientReconciler also require a non-nil SecretReference with a non-empty Namespace (and likely Name), or guard the dereference here and return a clear error instead.

Suggested change

	if prometheusConfig != nil && configureSecretPromClientReconciler(prometheusConfig) {
	namespacedSharedInformerFactory = informers.NewSharedInformerFactoryWithOptions(rs.Client, 0, informers.WithTransform(trimManagedFields), informers.WithNamespace(prometheusConfig.AuthToken.SecretReference.Namespace))
	if prometheusConfig != nil &&
	configureSecretPromClientReconciler(prometheusConfig) &&
	prometheusConfig.AuthToken != nil &&
	prometheusConfig.AuthToken.SecretReference != nil &&
	prometheusConfig.AuthToken.SecretReference.Namespace != "" {
	namespacedSharedInformerFactory = informers.NewSharedInformerFactoryWithOptions(
	rs.Client,
	0,
	informers.WithTransform(trimManagedFields),
	informers.WithNamespace(prometheusConfig.AuthToken.SecretReference.Namespace),
	)

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

With the current branching, any non-nil Prometheus config that has URL=="" will fall into the in-cluster controller path, changing prior behavior (previously no reconciliation happened unless URL was set) and potentially causing reconcileInClusterSAToken() to attempt client creation with an empty URL. Consider gating both controller creations on prometheusConfig.URL != "" (and returning a validation error if Prometheus is configured but URL is empty).

Suggested change

	if prometheusConfig != nil {
	if prometheusConfig != nil {
	if prometheusConfig.URL == "" {
	return nil, fmt.Errorf("prometheus metrics provider is configured but URL is empty")
	}

[Copilot]

waitForSecretsPropagation logs at Info level on every 100ms poll iteration; on slow CI this can generate a lot of noisy output. Consider reducing verbosity (e.g., klog.V(n)) and/or only logging on state transition (first miss / first hit) instead of every retry.

Suggested change

	return wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (bool, error) {
	for _, obj := range objects {
	secret, ok := obj.(*v1.Secret)
	if !ok {
	continue
	}
	_, err := secretsLister.Get(secret.Name)
	if err != nil {
	if apierrors.IsNotFound(err) {
	klog.Infof("Secret %s/%s has not propagated to the indexer", secret.Namespace, secret.Name)
	return false, nil
	}
	return false, err
	}
	klog.Infof("Secret %s/%s has propagated to the indexer", secret.Namespace, secret.Name)
	// Track logging state per secret so we only log on state transitions (first miss / first hit).
	firstMissLogged := make(map[string]bool)
	propagatedLogged := make(map[string]bool)
	return wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (bool, error) {
	for _, obj := range objects {
	secret, ok := obj.(*v1.Secret)
	if !ok {
	continue
	}
	key := secret.Namespace + "/" + secret.Name
	_, err := secretsLister.Get(secret.Name)
	if err != nil {
	if apierrors.IsNotFound(err) {
	if !firstMissLogged[key] {
	firstMissLogged[key] = true
	klog.V(3).Infof("Secret %s has not propagated to the indexer", key)
	}
	return false, nil
	}
	return false, err
	}
	if !propagatedLogged[key] {
	propagatedLogged[key] = true
	klog.V(3).Infof("Secret %s has propagated to the indexer", key)
	}

[Copilot]

setupPromClientControllerTest returns an error but also calls t.Fatalf inside the helper when secret propagation times out. Mixing fatal test exits with error returns makes the helper harder to reuse and can surprise callers; prefer returning the propagation error and let the caller decide whether to fail the test.

Suggested change

	t.Fatalf("secrets did not propagate to the indexer: %v", err)
	return nil, fmt.Errorf("secrets did not propagate to the indexer: %w", err)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

setupPromClientControllerTest returns an error but also calls t.Fatalf when secrets fail to propagate. This makes the error return misleading/unusable for callers and can terminate tests unexpectedly. Prefer returning the propagation error (and let callers decide whether to Fatal) instead of calling t.Fatalf inside this helper.

Suggested change

	t.Fatalf("secrets did not propagate to the indexer: %v", err)
	}
	}
	return &promClientControllerTestSetup{
	fakeClient: fakeClient,
	namespacedInformerFactory: namespacedInformerFactory,
	return nil, fmt.Errorf("secrets did not propagate to the indexer: %w", err)
	}
	}
	return &promClientControllerTestSetup{
	fakeClient: fakeClient,
	namespacedSharedInformerFactory: namespacedInformerFactory,

[Copilot]

Comment has an unmatched parenthesis: "Override the fields needed for this test (no need for a mutex". Please close the parenthesis to avoid confusing readers.

Suggested change

	// Override the fields needed for this test (no need for a mutex
	// since reconcileInClusterSAToken gets to run later)
	// Override the fields needed for this test (no need for a mutex since
	// reconcileInClusterSAToken gets to run later).