GoogleCloudPlatform / k8s-stackdriver

Apache License 2.0
389 stars 211 forks source link

Empty external metrics list will fail the client-go CachedDiscoveryClient caching and make helm upgrade resending api call #729

Open jacobycwang opened 1 month ago

jacobycwang commented 1 month ago

The custom-metrics-stackdriver-adapter returns an empty list in ListAllExternalMetrics (e.g., kubectl get --raw '/apis/external.metrics.k8s.io/v1beta1'). Although this allows the HPA to function, it causes side effects in other Kubernetes ecosystem tools, such as Helm. https://github.com/GoogleCloudPlatform/k8s-stackdriver/blob/d0e6643438e2ea83bc448603668ca1dd7b7e0edf/custom-metrics-stackdriver-adapter/pkg/adapter/provider/provider.go#L354-L358

We encountered an issue where helm upgrade sends over 400 API requests. This is because Helm relies on the client-go CachedDiscoveryClient for validation. CachedDiscoveryClient caches responses and saves them to a file if the response is not empty. When the custom-metrics-stackdriver-adapter returns an empty list for external.metrics.k8s.io/v1beta1, it is never cached, causing the CachedDiscoveryClient to continuously refresh all API resources (not just external.metrics.k8s.io/v1beta1). This results in a high number of API requests being sent. https://github.com/kubernetes/client-go/blob/c7396197f39ed43fb0369c054360fcb989d65c88/discovery/cached/disk/cached_discovery.go#L82C35-L82C65 https://github.com/kubernetes/client-go/blob/c7396197f39ed43fb0369c054360fcb989d65c88/discovery/cached/memory/memcache.go#L104

CatherineF-dev commented 5 days ago

From https://github.com/grafana/tanka/issues/1076, it might be related to https://github.com/GoogleCloudPlatform/k8s-stackdriver/pull/690 which bumped client-go version to fix vulnerabilities.

CatherineF-dev commented 5 days ago

Could you try this image: gcr.io/gke-release/custom-metrics-stackdriver-adapter:v0.14.2-gke.0 ?

raywainman commented 5 days ago

Helm recently upgraded from client-go v0.29 to v0.30 in https://github.com/helm/helm/pull/13021.