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

Add cache to target client #40

Merged
merged 1 commit into from
Apr 2, 2020
Merged

Add cache to target client #40

merged 1 commit into from
Apr 2, 2020

Conversation

timebertt
Copy link
Contributor

What this PR does / why we need it:
This PR adds a cache to the client for the target cluster. That way, grm's health controller will not retrieve all objects of the manged resource from the API server (by default every minute), but rather use the cache instead. Additionally it only retrieves the resource, if it is of a Kind, that will actually be checked for its health.

Also, grm now does not set .status.resources[].namespace to default for cluster scoped resources (e.g. CustomResourceDefinitions). The previous behaviour seemed to cause problems, when retrieving such resources from the cached client, so I changed it along the way.

Which issue(s) this PR fixes:
Part of gardener/gardener#1953

I did some rudimentary before/after comparison via prometheus in my local setup with the following results:

  • Received bytes for

    • grm in garden namespace down by ~65% (~15.5kB/s -> ~5.5kB/s)
    • grm in each shoot namespace down by ~30% (~16.9kB/s -> ~11.8kB/s) (should not really contribute much to external traffic, as most of the managed resources here target the Shoot's API server (in-cluster))
  • Sent bytes for

    • grm in garden namespace down by ~41% (~2.81kB/s -> ~1.65kB/s)
    • grm in each shoot namespace down by ~14% (~7.31kB/s -> ~6.31kB/s) (again, should not really contribute much to external traffic, as most of the managed resources here target the Shoot's API server (in-cluster))

Special notes for your reviewer:

Release note:

`gardener-resource-manager` now makes use of a caching client for talking to the targeted API server, which reduces its network traffic.

@timebertt timebertt requested a review from a team as a code owner March 27, 2020 11:37
@gardener-robot-ci-3 gardener-robot-ci-3 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 Mar 27, 2020
Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Minor nits.

pkg/controller/managedresources/health/health_checker.go Outdated Show resolved Hide resolved
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has 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 Mar 30, 2020
@timebertt
Copy link
Contributor Author

/hold
I have one more optimisation I want to make.

@timebertt
Copy link
Contributor Author

/hold cancel
The optimisation I wanted to make isn't really relevant.

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.

Very nice :) I just have one suggestion.

cmd/gardener-resource-manager/app/app.go Show resolved Hide resolved
@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 Apr 1, 2020
@timebertt
Copy link
Contributor Author

timebertt commented Apr 1, 2020

@timuthy and I reworked app.go to not use os.Exit() and return an error instead, and to use a new context and properly stop the manager or the cache when either of them errors.
I also rebased onto master.
PTAL :)

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

@rfranzke rfranzke added reviewed/ok-to-test Has 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 Apr 2, 2020
@timebertt
Copy link
Contributor Author

Rebased to resolve conflicts.
@rfranzke do you also want to take another look, or can we get this in?

@gardener-robot-ci-3 gardener-robot-ci-3 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 Apr 2, 2020
Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Had another rough sketch, but I'm anyways certain that you guys have done it very thoroughly.
/lgtm

@rfranzke rfranzke merged commit 495c8b1 into master Apr 2, 2020
@rfranzke rfranzke deleted the enh/target-cache branch April 2, 2020 07:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants