Always generate CRD metrics

When the VPA metrics were (re-)added, code was added to not try to
watch VPAs unless the CRDs had been created, to avoid spurious errors.
But it appears that ksm has since been fixed to not need this; it
watches for CRD creation/deletion itself, and ignores the VPA metric
definitions if the VPA CRDs don't exist.

Signed-off-by: Dan Winship <[email protected]>

Author: danwinship

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Note: This CHANGELOG is only for the monitoring team to track all monitoring related changes. Please see OpenShift release notes for official changes.
 
+## 4.20
+
+- [#2595](https://github.com/openshift/cluster-monitoring-operator/pull/2595) Multi-tenant support for KSM's CRS feature-set downstream.
+
 ## 4.18
 
 - [#2503](https://github.com/openshift/cluster-monitoring-operator/issues/2503) Expose `scrapeInterval` setting for UWM Prometheus.

assets/kube-state-metrics/deployment.yaml

@@ -36,6 +36,7 @@ spec:
         - --port=8081
         - --telemetry-host=127.0.0.1
         - --telemetry-port=8082
+        - --custom-resource-state-config-file=/etc/kube-state-metrics/custom-resource-state-configmap.yaml
         - |
           --metric-denylist=
           ^kube_secret_labels$,

jsonnet/components/kube-state-metrics.libsonnet

@@ -173,8 +173,8 @@ function(params)
     kubeRbacProxySecret: generateSecret.staticAuthSecret(cfg.namespace, cfg.commonLabels, 'kube-state-metrics-kube-rbac-proxy-config'),
 
     // This removes the upstream addon-resizer and all resource requests and
-    // limits. Additionally configures the kube-rbac-proxies to use the serving
-    // cert configured on the `Service` above.
+    // limits. Additionally, it configures the kube-rbac-proxies to use the serving
+    // cert configured on the `Service` above, and enables CRD metrics.
     //
     // The upstream kube-state-metrics Dockerfile defines a `VOLUME` directive
     // in `/tmp`. Although this is unused it will take some time for it to get
@@ -237,6 +237,7 @@ function(params)
                   else
                     c {
                       args+: [
+                        '--custom-resource-state-config-file=/etc/kube-state-metrics/custom-resource-state-configmap.yaml',
                         |||
                           --metric-denylist=
                           ^kube_secret_labels$,
@@ -259,7 +260,6 @@ function(params)
                           name: tmpVolumeName,
                           readOnly: false,
                         },
-                        // The custom resource state configmap is always mounted in the kube-state-metrics container and only when the VPA CRD is installed, CMO will add `--custom-resource-state-config-file` to the container arguments list.
                         {
                           mountPath: '/etc/kube-state-metrics',
                           name: crsVolumeName,

pkg/client/client.go

@@ -65,14 +65,12 @@ import (
 	"k8s.io/klog/v2"
 	apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
 	aggregatorclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset"
-	"k8s.io/utils/ptr"
 )
 
 const (
-	deleteTimeout                        = 10 * time.Minute
-	metadataPrefix                       = "monitoring.openshift.io/"
-	clusterConsole                       = "cluster"
-	VerticalPodAutoscalerCRDMetadataName = "verticalpodautoscalers.autoscaling.k8s.io"
+	deleteTimeout  = 10 * time.Minute
+	metadataPrefix = "monitoring.openshift.io/"
+	clusterConsole = "cluster"
 )
 
 type Client struct {
@@ -276,10 +274,6 @@ func (c *Client) KubernetesInterface() kubernetes.Interface {
 	return c.kclient
 }
 
-func (c *Client) ApiExtensionsInterface() apiextensionsclient.Interface {
-	return c.eclient
-}
-
 func (c *Client) EventRecorder() events.Recorder {
 	return c.eventRecorder
 }
@@ -423,21 +417,6 @@ func (c *Client) ClusterOperatorListWatch(ctx context.Context, name string) *cac
 	}
 }
 
-func (c *Client) VerticalPodAutoscalerCRDListWatch(ctx context.Context) *cache.ListWatch {
-	return &cache.ListWatch{
-		ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {
-			return c.eclient.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{
-				FieldSelector: fields.OneTermEqualSelector("metadata.name", VerticalPodAutoscalerCRDMetadataName).String(),
-			})
-		},
-		WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
-			return c.eclient.ApiextensionsV1().CustomResourceDefinitions().Watch(ctx, metav1.ListOptions{
-				FieldSelector: fields.OneTermEqualSelector("metadata.name", VerticalPodAutoscalerCRDMetadataName).String(),
-			})
-		},
-	}
-}
-
 func (c *Client) HasRouteCapability(ctx context.Context) (bool, error) {
 	_, err := c.oscclient.ConfigV1().ClusterOperators().Get(ctx, "ingress", metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
@@ -1850,29 +1829,6 @@ func (c *Client) RegisterConsolePlugin(ctx context.Context, name string) error {
 	return nil
 }
 
-// VPACustomResourceDefinitionPresent checks if VerticalPodAutoscaler CRD is present in the cluster.
-func (c *Client) VPACustomResourceDefinitionPresent(ctx context.Context, lastKnownVPACustomResourceDefinitionPresent *bool) (*bool, error) {
-	_, err := c.ApiExtensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(ctx, VerticalPodAutoscalerCRDMetadataName, metav1.GetOptions{})
-	if err != nil {
-		// VPA CRD is absent.
-		if apierrors.IsNotFound(err) {
-			return ptr.To(false), nil
-		}
-
-		// See if we have an idea of the state of the CRD's presence before the transient error occurred.
-		// If we do, resort to that since we do not want this to cause unnecessary reconciles.
-		if lastKnownVPACustomResourceDefinitionPresent != nil {
-			return lastKnownVPACustomResourceDefinitionPresent, nil
-		}
-
-		// If we don't, throw.
-		return nil, fmt.Errorf("failed to get %s CRD: %w", VerticalPodAutoscalerCRDMetadataName, err)
-	}
-
-	// VPA CRD is present.
-	return ptr.To(true), nil
-}
-
 // mergeMetadata merges labels and annotations from `existing` map into `required` one where `required` has precedence
 // over `existing` keys and values. Additionally, function performs filtering of labels and annotations from `exiting` map
 // where keys starting from string defined in `metadataPrefix` are deleted. This prevents issues with preserving stale

pkg/manifests/manifests.go

@@ -316,9 +316,6 @@ var (
 	KubeRbacProxyTLSCipherSuitesFlag                     = "--tls-cipher-suites="
 	KubeRbacProxyMinTLSVersionFlag                       = "--tls-min-version="
 
-	kubeStateMetricsCustomResourceStateConfigFileFlag = "--custom-resource-state-config-file="
-	kubeStateMetricsCustomResourceStateConfigFile     = "/etc/kube-state-metrics/custom-resource-state-configmap.yaml"
-
 	AuthProxyExternalURLFlag  = "-external-url="
 	AuthProxyCookieDomainFlag = "-cookie-domain="
 	AuthProxyRedirectURLFlag  = "-redirect-url="
@@ -748,8 +745,7 @@ func (f *Factory) KubeStateMetricsMinimalServiceMonitor() (*monv1.ServiceMonitor
 	return f.NewServiceMonitor(f.assets.MustNewAssetSlice(KubeStateMetricsMinimalServiceMonitor))
 }
 
-func (f *Factory) KubeStateMetricsDeployment(enableCRSMetrics bool) (*appsv1.Deployment, error) {
-	flagCRSConfigFile := kubeStateMetricsCustomResourceStateConfigFileFlag + kubeStateMetricsCustomResourceStateConfigFile
+func (f *Factory) KubeStateMetricsDeployment() (*appsv1.Deployment, error) {
 	d, err := f.NewDeployment(f.assets.MustNewAssetSlice(KubeStateMetricsDeployment))
 	if err != nil {
 		return nil, err
@@ -764,9 +760,6 @@ func (f *Factory) KubeStateMetricsDeployment(enableCRSMetrics bool) (*appsv1.Dep
 			if f.config.ClusterMonitoringConfiguration.KubeStateMetricsConfig.Resources != nil {
 				d.Spec.Template.Spec.Containers[i].Resources = *f.config.ClusterMonitoringConfiguration.KubeStateMetricsConfig.Resources
 			}
-			if enableCRSMetrics {
-				d.Spec.Template.Spec.Containers[i].Args = append(container.Args, flagCRSConfigFile)
-			}
 		}
 	}
 

pkg/manifests/manifests_test.go

@@ -272,7 +272,7 @@ func TestUnconfiguredManifests(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	_, err = f.KubeStateMetricsDeployment(false)
+	_, err = f.KubeStateMetricsDeployment()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -3694,14 +3694,13 @@ func TestKubeStateMetrics(t *testing.T) {
 
 	f := NewFactory("openshift-monitoring", "openshift-user-workload-monitoring", c, defaultInfrastructureReader(), &fakeProxyReader{}, NewAssets(assetsPath), &APIServerConfig{}, &configv1.Console{})
 
-	d, err := f.KubeStateMetricsDeployment(true)
+	d, err := f.KubeStateMetricsDeployment()
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	kubeRbacProxyTLSCipherSuitesArg := ""
 	kubeRbacProxyMinTLSVersionArg := ""
-	gotKubeStateMetricsCustomResourceStateConfigFile := ""
 	for _, container := range d.Spec.Template.Spec.Containers {
 		switch container.Name {
 		case "kube-state-metrics":
@@ -3712,7 +3711,6 @@ func TestKubeStateMetrics(t *testing.T) {
 				t.Fatal("kube-state-metrics resources incorrectly configured")
 			}
 
-			gotKubeStateMetricsCustomResourceStateConfigFile = getContainerArgValue(d.Spec.Template.Spec.Containers, kubeStateMetricsCustomResourceStateConfigFileFlag, container.Name)
 		case "kube-rbac-proxy-self", "kube-rbac-proxy-main":
 			if container.Image != "docker.io/openshift/origin-kube-rbac-proxy:latest" {
 				t.Fatalf("%s image incorrectly configured", container.Name)
@@ -3745,11 +3743,7 @@ func TestKubeStateMetrics(t *testing.T) {
 		t.Fatal("kube-state-metrics topology spread constraints WhenUnsatisfiable not configured correctly")
 	}
 
-	if gotKubeStateMetricsCustomResourceStateConfigFile != kubeStateMetricsCustomResourceStateConfigFileFlag+kubeStateMetricsCustomResourceStateConfigFile {
-		t.Fatal("expected kube-state-metrics to enable custom-resource-state metrics")
-	}
-
-	d2, err := f.KubeStateMetricsDeployment(true)
+	d2, err := f.KubeStateMetricsDeployment()
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/operator/operator.go

@@ -33,7 +33,6 @@ import (
 	"github.com/openshift/library-go/pkg/operator/events"
 	certapiv1 "k8s.io/api/certificates/v1"
 	v1 "k8s.io/api/core/v1"
-	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
@@ -205,27 +204,6 @@ type Operator struct {
 
 	ruleController    *alert.RuleController
 	relabelController *alert.RelabelConfigController
-
-	// lastKnownVPACustomResourceDefinitionPresent is a boolean pointer that
-	// remembers the presence of the VPA CRD in the cluster between
-	// kube-state-metrics task reconciliations. It is used to determine
-	// whether to enable kube-state-metrics custom-resource-state-based
-	// metrics for VPA CRs, even in cases where the Kube API may emit a
-	// transient error (errors excluding `IsNotFound`) on the VPA CRD `GET`
-	// requests (to determine its presence).
-	// * `true` indicates that the VPA CRD is already present in the
-	// cluster, and the custom-resource-state-based metrics can be safely
-	// enabled.
-	// * `false` indicates that the VPA CRD is not present in the cluster,
-	// and enabling the custom-resource-state-based metrics will cause
-	// kube-state-metrics to error (affecting `KubeStateMetricsListErrors`).
-	// * `nil` indicates that the presence of the VPA CRD is unknown, and
-	// the operator will attempt to determine the presence of the VPA CRD in
-	// the current reconciliation cycle, and remember its state in the next
-	// one. In the case where the VPA CRD is added or removed between
-	// reconciliations, the variable "forgets" it, and is set to `nil`,
-	// triggering a check on the next cycle.
-	lastKnownVPACustomResourceDefinitionPresent *bool
 }
 
 func New(
@@ -446,20 +424,6 @@ func New(
 	}
 	o.informers = append(o.informers, informer)
 
-	informer = cache.NewSharedIndexInformer(
-		o.client.VerticalPodAutoscalerCRDListWatch(ctx),
-		&apiextv1.CustomResourceDefinition{}, resyncPeriod, cache.Indexers{},
-	)
-	// Only trigger reconciliation on the addition or removal of VPA CRDs.
-	_, err = informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
-		AddFunc:    o.handleEvent,
-		DeleteFunc: o.handleEvent,
-	})
-	if err != nil {
-		return nil, err
-	}
-	o.informers = append(o.informers, informer)
-
 	kubeInformersOperatorNS := informers.NewSharedInformerFactoryWithOptions(
 		c.KubernetesInterface(),
 		resyncPeriod,
@@ -667,10 +631,7 @@ func (o *Operator) handleEvent(obj interface{}) {
 		*configv1.APIServer,
 		*configv1.Console,
 		*configv1.ClusterOperator,
-		*configv1.ClusterVersion,
-		// Currently, the CRDs that trigger reconciliation are:
-		// * verticalpodautoscalers.autoscaling.k8s.io
-		*apiextv1.CustomResourceDefinition:
+		*configv1.ClusterVersion:
 		// Log GroupKind and Name of the obj
 		rtObj := obj.(k8sruntime.Object)
 		gk := rtObj.GetObjectKind().GroupVersionKind().GroupKind()
@@ -819,18 +780,6 @@ func (o *Operator) sync(ctx context.Context, key string) error {
 		klog.Warningf("Fail to load ConsoleConfig, AlertManager's externalURL may be outdated")
 	}
 
-	// Enable kube-state-metrics' custom-resource-state-based metrics if VPA CRD is installed within the cluster.
-	o.lastKnownVPACustomResourceDefinitionPresent, err = o.client.VPACustomResourceDefinitionPresent(ctx, o.lastKnownVPACustomResourceDefinitionPresent)
-	if err != nil {
-		// Throw on all transient errors.
-		return fmt.Errorf("unable to guess the desired state for kube-state-metrics' custom-resource-state metrics enablement: %w", err)
-	} else {
-		// If we didn't get an error, we can safely assume that the CRD is deterministically either present or absent.
-		if *o.lastKnownVPACustomResourceDefinitionPresent {
-			klog.Infof("%s CRD found, enabling kube-state-metrics' custom-resource-state-based metrics", client.VerticalPodAutoscalerCRDMetadataName)
-		}
-	}
-
 	factory := manifests.NewFactory(
 		o.namespace,
 		o.namespaceUserWorkload,
@@ -859,7 +808,7 @@ func (o *Operator) sync(ctx context.Context, key string) error {
 				newTaskSpec("Prometheus", tasks.NewPrometheusTask(o.client, factory, config)),
 				newTaskSpec("Alertmanager", tasks.NewAlertmanagerTask(o.client, factory, config)),
 				newTaskSpec("NodeExporter", tasks.NewNodeExporterTask(o.client, factory)),
-				newTaskSpec("KubeStateMetrics", tasks.NewKubeStateMetricsTask(o.client, factory, *o.lastKnownVPACustomResourceDefinitionPresent)),
+				newTaskSpec("KubeStateMetrics", tasks.NewKubeStateMetricsTask(o.client, factory)),
 				newTaskSpec("OpenshiftStateMetrics", tasks.NewOpenShiftStateMetricsTask(o.client, factory)),
 				newTaskSpec("MetricsServer", tasks.NewMetricsServerTask(ctx, o.namespace, o.client, factory, config)),
 				newTaskSpec("TelemeterClient", tasks.NewTelemeterClientTask(o.client, factory, config)),

pkg/tasks/kubestatemetrics.go

@@ -23,16 +23,14 @@ import (
 )
 
 type KubeStateMetricsTask struct {
-	client           *client.Client
-	factory          *manifests.Factory
-	enableCRSMetrics bool
+	client  *client.Client
+	factory *manifests.Factory
 }
 
-func NewKubeStateMetricsTask(client *client.Client, factory *manifests.Factory, enableCRSMetrics bool) *KubeStateMetricsTask {
+func NewKubeStateMetricsTask(client *client.Client, factory *manifests.Factory) *KubeStateMetricsTask {
 	return &KubeStateMetricsTask{
-		client:           client,
-		factory:          factory,
-		enableCRSMetrics: enableCRSMetrics,
+		client:  client,
+		factory: factory,
 	}
 }
 
@@ -97,7 +95,7 @@ func (t *KubeStateMetricsTask) Run(ctx context.Context) error {
 		return fmt.Errorf("reconciling %s/%s ConfigMap failed: %w", cm.Namespace, cm.Name, err)
 	}
 
-	dep, err := t.factory.KubeStateMetricsDeployment(t.enableCRSMetrics)
+	dep, err := t.factory.KubeStateMetricsDeployment()
 	if err != nil {
 		return fmt.Errorf("initializing kube-state-metrics Deployment failed: %w", err)
 	}