Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Target client improvements #95

Merged
merged 3 commits into from
Dec 4, 2020

Conversation

timebertt
Copy link
Contributor

@timebertt timebertt commented Dec 2, 2020

How to categorize this PR?

/area scalability robustness quality
/kind enhancement
/priority normal

What this PR does / why we need it:

This PR improves the following aspects of the client for the target cluster:

  • The cache can be disabled via the newly added command line flag --target-disable-cache. In this case, grm won't cache any objects of the target cluster and always talk to the target API server directly (fixes Add option to disable target cache #92).
  • Switches to a DynamicRESTMapper, which will ensure that resources are rediscovered on NoMatchErrors, but discovery calls are not executed more than once every 10 minutes (fixes Switch to DynamicRESTMapper for target client #93).

I took the chance to refactor the contents of app.go, as I found it to be very overloaded and hard to comprehend.
Now

  • both the health and secret controller are placed in their own controller package
  • controllers and the corresponding options are setup in the respective controller packages in add.go
  • the health endpoint is moved into its own dedicated package
  • the ClassFilter logic is moved into its own dedicated package
  • all remaining general options (i.e. command line flags) are handled in dedicated files under cmd
  • app.go only instruments those options and AddToManager functions to put everything together. This is very similar to how we construct manager, options and controllers in our extensions.

Also, I switched to our gcr copies of the base images.

Special notes for your reviewer:
Similar to g/g, grm now also vendors our c-r fork in order to get the fix from kubernetes-sigs/controller-runtime#1151:
sigs.k8s.io/controller-runtime => github.com/gardener/controller-runtime v0.6.3-gardener.1

We can drop it, once we have upgraded to [email protected].

Release note:

The cache of the kubernetes client for the target cluster can now be disabled via the `--target-disable-cache` flag.
gardener-resource-manager now uses a `DynamicRESTMapper`, which will reduce the amount of explicit discovery calls and faster reconciliation loops and some cases.

@timebertt timebertt requested a review from a team as a code owner December 2, 2020 19:31
@gardener-robot gardener-robot added area/quality Output qualification (tests, checks, scans, automation in general, etc.) related area/robustness Robustness, reliability, resilience related area/scalability Scalability related kind/enhancement Enhancement, improvement, extension priority/normal needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Dec 2, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 2, 2020
@timebertt
Copy link
Contributor Author

Also tested this PR in my local gardener setup, shoot creation/reconciliation/hibernation/wakeup/deletion works fine.
Also did some load testing with large secrets like described in #92 and grm was completely stable and at a constant level of ~30MiB of Memory 💰

@timuthy
Copy link
Contributor

timuthy commented Dec 3, 2020

/assign

Copy link
Contributor

@timuthy timuthy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR. Now it looks very similar to what we do in the Gardener extension which will further simplify maintenance, thanks for that. I only found one nit for now.

pkg/cmd/manager.go Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Dec 3, 2020
@timebertt timebertt requested a review from timuthy December 4, 2020 07:44
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 4, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 4, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 4, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 4, 2020
Copy link
Contributor

@timuthy timuthy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Dec 4, 2020
@gardener-robot gardener-robot added needs/changes Needs (more) changes and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else reviewed/lgtm Has approval for merging labels Dec 4, 2020
@timebertt timebertt merged commit 2da0208 into gardener-attic:master Dec 4, 2020
@timebertt timebertt deleted the enh/target-client branch December 4, 2020 15:01
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/quality Output qualification (tests, checks, scans, automation in general, etc.) related area/robustness Robustness, reliability, resilience related area/scalability Scalability related kind/enhancement Enhancement, improvement, extension needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to DynamicRESTMapper for target client Add option to disable target cache
5 participants