Code review comments from @ialidzhikov

[ialidzhikov]

First, you could address the findings from Oliver's review against the main branch by creating a PR that addresses his comments.

---

Mid

• Switch to prow for head-update and pull-requests jobs. For new repos we already use prow as it gives many possibilities such as having e2e test, defining checks in much better format than CI/CD black magics, etc.
• No developer docs/instructions how to run it locally (aka local setup docs).
• Switch to project to push to AR. GCR is being replaced by AR and C. Cwienk was adapting all repos to publish to AR. Looks like he missed this repo: Move from GCR to artifact registry #10
• We drop vendoring from the repos. Nowadays, gardener/gardener and most of the extension repos don't have a vendor dir. As a new Project you could drop vendor dir as well: Drop vendoring #13
• You could add developer docs for the existing controllers and other important units in the code base: Example: https://github.com/gardener/gardener/blob/master/docs/concepts/gardenlet.md#controllers
• Can we use the latest version of the sigs.k8s.io/custom-metrics-apiserver dependency (v1.28.0) https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/go.mod#L21 This should also transitively update the K8s dependencies to the latest versions. I recall the you mentioned that you have problems with the dependencies: Upgrade k8s.io/* to v0.28, sigs.k8s.io/controller-runtime to v0.16 #14
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/api/generated/openapi/openapi.go#L3: How this file is being generated? The Makefile says "Code generation is currently not implemented". Set up proper code generation: Upgrade k8s.io/* to v0.28, sigs.k8s.io/controller-runtime to v0.16 #14
• Don't use testIsolation approach to fake/mock TimeNow. Use k8s.io/utils/clock/.RealClock/k8s.io/utils/clock/testing.FakeClock instead. Example: https://github.com/gardener/gardener/blob/dff43d99d2128c99f6d09116145ee48aebfd97f4/pkg/controllermanager/controller/event/reconciler.go#L38. Usages that need adaptation:
  • https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/metrics_provider/metrics_provider.go#L54
  • https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/input_data_registry/input_data_registry.go#L168
  • https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/metrics_scraper/pacemaker.go#L51
  • https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/metrics_scraper/scrape_queue.go#L76
  • https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/metrics_scraper/scraper.go#L295-L296
  • https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/ha/ha_service.go#L29
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/util/gardener/util.go#L13: This is not true. Some quite old Shoot control plane namespaces are in the format shoot-<project-name>-<shoot-name>. See https://github.com/gardener/gardener/blob/76704c377f34cdbdf1b0d3986b243c8b67c66909/pkg/component/kubeapiserverexposure/kube_apiserver_service.go#L237-L239 and https://github.com/gardener/gardener/blob/76704c377f34cdbdf1b0d3986b243c8b67c66909/pkg/utils/gardener/shoot.go#L696-L708. You should rather adapt the check to be "has prefix shoot-": Apply review comments. Add debug support #7
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/util/gardener/util.go#L13 - you should also think about the garden runtime cluster. As far as I understand, we would like to use the same approach to scale the virtual-kube-apiserver, maybe even the gardener-apiserver. Definitely this check won't work for the virtual-kube-apiserver or gardener-apiserver.
• You could add skaffold based local setup.
• You could add e2e tests.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/controller/secret/actuator.go#L33: I am not sure reusing the prometheus Shoot access Secret is a good thing. Maybe we should rather have an own Shoot access Secret that gets created by gardenlet?: Apply review comments. Add debug support #7
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/controller/reconciler_test.go#L31: I hope you could use the fake client from "sigs.k8s.io/controller-runtime/pkg/client/fake".
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/app/common.go#L10-L17: I agree with Oliver's comment in Proposed #3 (comment). No need to introduce our semantics. We can use the K8s sematics described in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md#what-method-to-use. Or alternatively use the log level semantics described in https://github.com/gardener/gardener/blob/4507f53471f8f982e1351af499a5ed2804ee65d4/docs/development/logging.md#log-levels.
• No readiness and liveness probe defined in the example Deployment in example/custom-metrics-deployment.yaml

Minor

• Drop the .docforge dir and switch to the central manifest. After Use central and new manifest format gardener/documentation#431, the repos do no longer needs to define a .docforge dir and the manifests are maintained centrally. See the linked issue for more details. Additionally, as a consequence https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/Makefile#L104-L106 has to be dropped. Drop also the make check-docforge target. It should be no longer needed.: Fix make verify #11
• make check is reporting golangci-lint findings. You could fix them.: Fix make verify #11
• make format is failing that there is no test/ dir.: Fix make verify #11
• make generateis not implemented (https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/Makefile#L112-L116): Does the project need code generation at all? If not, let's remove it.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/Dockerfile#L9 - +1, let's use distroless instead of alpine. It is also part of the component checklist - the component should not run as a root user, if possible.: Fix make verify #11
• The pkg/version pkg - we usually don't define such pkg in other repos and rather reuse the k8s.io/component-base/version/verflag pkg. You should be already familiar with it as in Add support for a --version command line flag gardener/gardener-extension-runtime-gvisor#38 you used this pkg and eliminated a custom version pkg in the runtime-gvisor extension: Move from GCR to artifact registry #10
• All source files are missing a REUSE license header: Switch to use REUSE license format  #12
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/metrics_scraper/metrics_client.go#L44: Instead of having metricsClientTestIsolation you could directly have a field that is rest.HTTPClient. When you instantiate in non-test code, you pass real client to a constuctor func such as NewMetricsClient(httpClient). When you instantiate in test code, you pass fake/mock client.
• You could move the copied code from gardener/gardener from ./pkg/util/gardener to /third_party/. Example: gardener/gardener@a1eb2fb: Drop vendoring #13
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/cmd/gardener-custom-metrics/main.go#L85-L88: A ClientSet is created but then it is not used. What is the rational behind it? I asssume we can drop it.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/util/errutil/errutil.go#L13-L19: Why we need such utility func and why it is not possible to use fmt.Errorf natively as in every other place in the gardener code-base.

Nits (really, really, really minor)

• rm LICENSE.md. The licenses are already defined under LICENSES/ and there is the symlink LICENSE in the root of the repo: Switch to use REUSE license format  #12
• Add a REUSE badge to the README.md. Example: Do not use a fake REUSE badge gardener/gardener-extension-shoot-rsyslog-relp#62: Apply review comments. Add debug support #7
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/cmd/gardener-custom-metrics/main.go#L16: In gardener/gardener and other repos we don't use import alias for "sigs.k8s.io/controller-runtime/pkg/manager". I assume you use import alias to have a variable named manager. In gardener/gardener we avoid this problem by naming the var mgr. You could follow also the same pattern: Upgrade k8s.io/* to v0.28, sigs.k8s.io/controller-runtime to v0.16 #14
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/Makefile#L1-L13: REUSE license header should be used instead: Switch to use REUSE license format  #12
• There is hack/test-e2e-local.sh but it seems not used. Let's drop it for now and introduce it if needed in the future.: Drop vendoring #13
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/Dockerfile#L16: The ".exe" naming is odd, we don't do it in any other Dockerfile in the gardener org. Let's remove it.: Apply review comments. Add debug support #7
• You could remove commented code/statements such as https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/Dockerfile#L7 and https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/Dockerfile#L17: Apply review comments. Add debug support #7
• VERSION file should rather contain the following version v0.1.0-dev: Apply review comments. Add debug support #7
• go.mod: We usually use only 2 require blocks. One for the direct dependencies, and the other one managed by the goo tool for the indirect/transitive ones. You could merge the first and the second require blocks.: Apply review comments. Add debug support #7
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/go.mod#L12-L21: There are many comments which are not very useful for the reader. I rather suggest to drop them as they seem leftover from testing: Fix make verify #11
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/go.mod#L115: You should be able to drop this replace and use the semver version (v0.23.7) in https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/go.mod#L15. I assume you had to add the replace because of gardener/gardener and all modules that vendor gardener/gardener require k8s.io/[email protected]+incompatible gardener/gardener#6807. But I see that this repo no longer vendors github.com/gardener/gardener and in the latest versions of gardener this issue is fixed.: Drop the sigs.k8s.io/metrics-server dependency #8
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/api/openapi-gen-dependency.go#L4-L9: In gardnener/gardener and in other repos we use to tool dependency trick https://github.com/gardener/gardener/blob/6a00a8027aa2829a00e3294bace261babd2770bb/hack/tools.go#L30. You could move the corresponding import to hack/tools.go to make it clear that this is a tool dependency: Upgrade k8s.io/* to v0.28, sigs.k8s.io/controller-runtime to v0.16 #14
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/app/cli_options.go#L104-L105: doc strings should be written above the field, not next to the field.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/metrics_provider/metrics_provider.go#L18: the package naming convensions in golang (https://go.dev/blog/package-names): Good package names are short and clear. They are lower case, with no under_scores or mixedCaps.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/metrics_provider/metrics_provider.go#L81: No need to use %s/%s, the String() method of a types.NamespacedName already does this.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/input_data_registry/consumer_interface.go#L13-L19: Move them above the field, not next to the field.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/input_data_registry/consumer_interface.go#L12: I don't think kapi is an abreviation that get's used somewhere in gardener/gardener code-base. Consider renaming to kubeAPIServer.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/metrics_scraper/metrics_client.go#L96: Typo responce -> response: Apply review comments. Add debug support #7
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/metrics_scraper/metrics_client.go#L71: You could remove the metrics client: prefix from the error messages from this func. It should be rather added by the caller.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/controller/pod/actuator.go#L116-L120: Dependency injection funcs like InjectClient are removed in the newer versions of sigs.k8s.io/controller-runtime. This should be adapted after upgrading the dependencies to the latest versions: Upgrade k8s.io/* to v0.28, sigs.k8s.io/controller-runtime to v0.16 #14
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/controller/pod/predicate.go#L31: You could give a better name to the predicate, for example NewKubeAPIServerPodPredicate.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/controller/constants.go#L3-L10: they seem unused.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/metrics_scraper/scrape_queue.go#L161: You could use dueAtTime.Sub(q.scrapePeriod).
• Add support for a --version flag.

Questions:

• Why the repo is private?: The repo has been made public.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/controller/pod/actuator.go#L75: What happens when pod.Status.PodIP is empty. According to the doc string of the field, pod.Status.PodIP will be empty if not yet allocated.
  • [andrerun-new]: See the log entry below. It's a bit ugly - a project outsider may have a hard time figuring out what's going on. @ialidzhikov, a pod gets stuck without an IP address every now and then, right? It's not an extremely rare event? If so, I think I should add special handling for this case and log a nicer message. [under-discussion]

    ERROR	gardener-custom-metrics.input.scraper	Kapi metrics retrieval failed	{"op": "scrape", "namespace": "shoot--local--local", "pod": "kube-apiserver-5588c58789-crm72", "error": "metrics client: making http request: Get \"https:///metrics\": http: no Host in request URL"}
    

• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/input_data_registry/input_data_registry.go#L24: Why we store the whole MetricsUrl? Instead you could only store the Pod IP and construct the metrics URL when fetching the metrics.
• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/input/input_data_registry/input_data_registry.go#L2
  • [andrerun-new]: The design reason is I want to keep the decision 'where to scrape', outside of the scraper. There's also a minor runtime concern - I prefer less object creation/GC churn.

3: Storing the same Pod labels would be a lot waste of memory. I see that you need the Pod labels to allow selecting metrics by object labelSelector. Maybe the whole model has to be adapted. We can for example accept that Pod labels are immutable and store them only once and not for every new metric value. [under-discussion]

• https://github.com/gardener/gardener-custom-metrics/blob/392b48aab8e985dfbbea4a9cffdf753fcc315cb3/pkg/ha/ha_service.go#L47: IIUC, the benefit of running 2 replicas is only that the 2nd Pod waits in "stand by" mode and on issues with the leader replica, the "stand by" can take over faster. By faster - we don't to wait a new Pod to be scheduled and started. Updating the Endpoint manually to influence the traffic to go to the leader replica looks hacky. We were running metrics-server for Shoots and ManagedSeeds for years with a single replica and I don't recall us having issues related to it. https://github.com/kubernetes-sigs/metrics-server/tree/master?tab=readme-ov-file#high-availability: metrics-server seems to have a real HA mode where 2 of the replicas are serving (?). We can check what they do and how. And I agree with Proposed #3 (comment) - this approach is error-prone a lot.
  • [andrerun]: The main benefit I see in the second replica is that it ties compute resources in another AZ, so it guards against AZ resource shortage disrupting failover. Overall, I have my reservations regarding the need for a second replica, considering the intended use of the component, but that was a hard requirement introduced by the GEP review process. I'll elaborate offline. [under-discussion]
• I didn't manage to test the component in local setup at all (due to missing docs/instructions) but I wanted to ask how it behaves on restarts and whether the HPA acting on the custom metric is fine with it. I assume on Pod restart the leader will change and the newly elected replica won't report any metrics (or will report 0-ed metrics value). Is HPA able to deal with unavailability of the gardener-custom-metrics component?

---

Final notes. I didn't deep dive into non-trivial packages like ./pkg/input/metrics_scraper.