Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch controller-manager to logr (1) #5057

Merged
merged 16 commits into from
Dec 6, 2021

Conversation

timebertt
Copy link
Member

How to categorize this PR?

/area logging dev-productivity
/kind enhancement

What this PR does / why we need it:

This is the first PR of a few, that switches the controllers in gardener-controller-manager from logrus to logr.
It tackles:

  • harmonizing the log format (json) of logrus and zap to make logs less confusing during migration period
  • adding zap logr in gcm and gardenlet
  • migrating logrus usages in common packages (shared between gcm and gardenlet)
  • migrating the first few gcm controllers

Which issue(s) this PR fixes:
Part of #4251

Special notes for your reviewer:
/squash

I noticed, that there will be some common questions about different logging aspects. So I plan to add a developer guide answering these questions in one of the following PRs, basically a condensed version of

Release note:

gardener-controller-manager and gardenlet have started switching from logrus to zap. Make sure to use the `json` log format to have harmonized logging during the migration period.

Now, in json both log format look very similar (only key ordering is different).
This is done to make logs less confusing during the migration period.
However, in console/text logs quite differently (not supposed to be used in
production).
it is unused in gcm/gardenlet controllers
This is what the controller-runtime controller implementation does as well.
So, the result will not change when switching from our custom coding to the
upstream implementation later on.
Using gbytes.NewBuffer and gbytes.Say is nicer than mocks
@timebertt timebertt requested a review from a team as a code owner November 23, 2021 14:48
@gardener-robot gardener-robot added area/dev-productivity Developer productivity related (how to improve development) area/logging Logging related kind/enhancement Enhancement, improvement, extension merge/squash size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 23, 2021
@rfranzke
Copy link
Member

/assign

@timebertt
Copy link
Member Author

@stoyanr Thank you for your review.
I addressed your feedback in a new commit, PTAL :)

Copy link
Contributor

@stoyanr stoyanr left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations!

/lgtm

Copy link
Member

@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.

/lgtm

@rfranzke rfranzke merged commit 626ba7c into gardener:master Dec 6, 2021
@timebertt timebertt deleted the refactor/gcm-logr branch December 6, 2021 07:42
rfranzke added a commit to rfranzke/gardener that referenced this pull request Mar 8, 2022
This was forgotten in gardener#5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.
rfranzke added a commit to rfranzke/gardener that referenced this pull request Mar 12, 2022
This was forgotten in gardener#5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.
timebertt pushed a commit that referenced this pull request Mar 14, 2022
…#5539)

* Set runtime logger for gardenlet

This was forgotten in #5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.

* `NewClientSet` should return hash right away

This is to prevent race conditions such as #5285

* Adapt `GardenClientSetFactory`

* Adapt `SeedClientSetFactory`

* Adapt `PlantClientSetFactory`

* Adapt `ShootClientSetFactory`
rfranzke added a commit to rfranzke/gardener that referenced this pull request Mar 14, 2022
This was forgotten in gardener#5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.
rfranzke added a commit to rfranzke/gardener that referenced this pull request Mar 14, 2022
This was forgotten in gardener#5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.
rfranzke added a commit to rfranzke/gardener that referenced this pull request Mar 14, 2022
This was forgotten in gardener#5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.
rfranzke added a commit that referenced this pull request Mar 14, 2022
…ind" errors during shoot reconciliation/deletion (#5568)

* Set runtime logger for gardenlet

This was forgotten in #5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.

* `NewClientSet` should return hash right away

This is to prevent race conditions such as #5285

* Adapt `GardenClientSetFactory`

* Adapt `SeedClientSetFactory`

* Adapt `PlantClientSetFactory`

* Adapt `ShootClientSetFactory`
rfranzke added a commit that referenced this pull request Mar 14, 2022
…ind" errors during shoot reconciliation/deletion (#5569)

* Set runtime logger for gardenlet

This was forgotten in #5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.

* `NewClientSet` should return hash right away

This is to prevent race conditions such as #5285

* Adapt `GardenClientSetFactory`

* Adapt `SeedClientSetFactory`

* Adapt `PlantClientSetFactory`

* Adapt `ShootClientSetFactory`
rfranzke added a commit that referenced this pull request Mar 15, 2022
…ind" errors during shoot reconciliation/deletion (#5567)

* Set runtime logger for gardenlet

This was forgotten in #5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.

* `NewClientSet` should return hash right away

This is to prevent race conditions such as #5285

* Adapt `GardenClientSetFactory`

* Adapt `SeedClientSetFactory`

* Adapt `PlantClientSetFactory`

* Adapt `ShootClientSetFactory`
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
* Harmonize logrus and zap logging formats

Now, in json both log format look very similar (only key ordering is different).
This is done to make logs less confusing during the migration period.
However, in console/text logs quite differently (not supposed to be used in
production).

* Switch NewIDLogger to logr

* Switch ClientMaps to logr, stop passing around loggers

* Construct zap logr in cmd packages

* Log result of automaxprocs with logr

* Remove injection logic in CreateWorker

it is unused in gcm/gardenlet controllers

* Optionally inject logr logger into reconcile ctx

This is what the controller-runtime controller implementation does as well.
So, the result will not change when switching from our custom coding to the
upstream implementation later on.

* Drop logr mock

Using gbytes.NewBuffer and gbytes.Say is nicer than mocks

* Replace several usages of logger.Logger

* Use injected logger in scheduler

* Switch factory to logr

* Switch bastion controller to logr

* Switch cloudprofile controller to logr

* Switch csr-autoapprove controller to logr

* make revendor

* Address review feedback
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
…gardener#5539)

* Set runtime logger for gardenlet

This was forgotten in gardener#5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.

* `NewClientSet` should return hash right away

This is to prevent race conditions such as gardener#5285

* Adapt `GardenClientSetFactory`

* Adapt `SeedClientSetFactory`

* Adapt `PlantClientSetFactory`

* Adapt `ShootClientSetFactory`
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
* Harmonize logrus and zap logging formats

Now, in json both log format look very similar (only key ordering is different).
This is done to make logs less confusing during the migration period.
However, in console/text logs quite differently (not supposed to be used in
production).

* Switch NewIDLogger to logr

* Switch ClientMaps to logr, stop passing around loggers

* Construct zap logr in cmd packages

* Log result of automaxprocs with logr

* Remove injection logic in CreateWorker

it is unused in gcm/gardenlet controllers

* Optionally inject logr logger into reconcile ctx

This is what the controller-runtime controller implementation does as well.
So, the result will not change when switching from our custom coding to the
upstream implementation later on.

* Drop logr mock

Using gbytes.NewBuffer and gbytes.Say is nicer than mocks

* Replace several usages of logger.Logger

* Use injected logger in scheduler

* Switch factory to logr

* Switch bastion controller to logr

* Switch cloudprofile controller to logr

* Switch csr-autoapprove controller to logr

* make revendor

* Address review feedback
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
…gardener#5539)

* Set runtime logger for gardenlet

This was forgotten in gardener#5057 and prevents logs for packages used in gardenlet which were already migrated from `logrus` to `logr`.

* `NewClientSet` should return hash right away

This is to prevent race conditions such as gardener#5285

* Adapt `GardenClientSetFactory`

* Adapt `SeedClientSetFactory`

* Adapt `PlantClientSetFactory`

* Adapt `ShootClientSetFactory`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev-productivity Developer productivity related (how to improve development) area/logging Logging related kind/enhancement Enhancement, improvement, extension size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants