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.

Sign up for GitHub

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

Jump to bottom

[scheduler] Introduce controller-runtime manager and switch Shoot controller #4320

Merged
merged 19 commits into from
Aug 10, 2021
Merged

[scheduler] Introduce controller-runtime manager and switch Shoot controller #4320

merged 19 commits into from
Aug 10, 2021

Conversation

xrstf
Copy link
Contributor

@xrstf xrstf commented Jul 6, 2021 •
edited by timuthy
Loading

How to categorize this PR?

/area dev-productivity
/kind technical-debt

What this PR does / why we need it:
This PR replaces the home grown controller management and leader election in the gardener-scheduler with the controller-runtime Manager and its cohorts.

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

Special notes for your reviewer:

Release note:

The following changes have been made incompatibly to the `GardenerSchedulerConfiguration`:
- The configuration key `server` has been refined into `healthProbes` and `metrics`. Note that both cannot be listening on the same port.
- The `CachedRuntimeClients` feature gate has been removed, objects are now always cached.
- `lockObjectName` was removed in favor of `resourceName`.
- `lockObjectNamespace` was removed in favor of `resourceNamespace`.

If you deploy Gardener with the provided Helm charts, note that the metrics endpoint for the Gardener-Scheduler is now exposed via a service on port `9090`.

@gardener-robot gardener-robot added area/dev-productivity Developer productivity related (how to improve development) kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly labels Jul 6, 2021
@gardener-robot
Copy link

@xrstf Thank you for your contribution.

@gardener-robot gardener-robot added needs/review size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2021
@xrstf xrstf force-pushed the scheduler-ctrlruntime-manager branch from 5fc4cc4 to 599e8cd Compare July 6, 2021 13:17
@gardener-robot gardener-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2021
@xrstf xrstf force-pushed the scheduler-ctrlruntime-manager branch from f674a65 to 7e28c65 Compare July 6, 2021 20:06
@xrstf xrstf force-pushed the scheduler-ctrlruntime-manager branch from 7e28c65 to a54adf3 Compare July 12, 2021 13:46
rfranzke
rfranzke reviewed Jul 14, 2021
View reviewed changes
pkg/logger/types.go Outdated Show resolved Hide resolved
pkg/logger/types.go Outdated Show resolved Hide resolved
pkg/logger/zap_test.go Outdated Show resolved Hide resolved
pkg/logger/zap.go Outdated Show resolved Hide resolved
pkg/logger/logrus_test.go Outdated Show resolved Hide resolved
pkg/scheduler/apis/config/v1alpha1/types.go Outdated Show resolved Hide resolved
example/20-componentconfig-gardener-scheduler.yaml Outdated Show resolved Hide resolved
@rfranzke
Copy link
Member

/needs rebase
/invite @timebertt

@gardener-robot gardener-robot requested a review from timebertt July 14, 2021 12:25
@timuthy timuthy removed their assignment Aug 9, 2021
@timuthy
Copy link
Member

timuthy commented Aug 10, 2021

@timebertt thanks a lot for your review. I addressed all your comments in the latest commit. Additionally, I added a health check which was missing before, so that the /healthz endpoint didn't work.

timebertt
timebertt reviewed Aug 10, 2021
View reviewed changes
Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Awesome, this looks great now! 🥳
Two easy-to-fix comments left ;)

pkg/scheduler/apis/config/validation/validation.go Show resolved Hide resolved
pkg/logger/zap.go Outdated Show resolved Hide resolved
timebertt
timebertt approved these changes Aug 10, 2021
View reviewed changes
Copy link
Member

@timebertt timebertt 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, thank you for continuing the PR and addressing all of my suggestions! :)
/lgtm

@timuthy timuthy added this to the v1.29 milestone Aug 10, 2021
@timuthy timuthy merged commit c8f7398 into gardener:master Aug 10, 2021
@timebertt timebertt mentioned this pull request Aug 18, 2021
Closed
12 tasks
@timebertt timebertt mentioned this pull request Sep 11, 2021
Merged
@timebertt timebertt mentioned this pull request Oct 21, 2021
Closed
76 tasks
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
@xrstf @timuthy
use ctrlruntime Manager to manage shoot scheduler controller (gardene…
a0e0a54 
…r#4320)

* use ctrlruntime Manager to manage shoot scheduler controller

* remove dead code

* make zap logger/logr code reusable

* nicer error trace

* deprecated SchedulerConfig.Server in favor of .HealthServer and .MetricsServer

* PR feedback

* remove metrics.go

* revendor

* PR feedback

* fix PR feedback

* update controlplane chart

* introduce pkg/logger.Format constants

* PR feedback

* Add default log format for Gardenlet

* Unify leader election options for GSched

* Use predicates for GSched

* Fix verify

* Address review comments

* Remove skip caller from logger

Co-authored-by: Tim Usner <tim.usner@sap.com>
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
@xrstf @timuthy
use ctrlruntime Manager to manage shoot scheduler controller (gardene…
ddaa0aa 
…r#4320)

* use ctrlruntime Manager to manage shoot scheduler controller

* remove dead code

* make zap logger/logr code reusable

* nicer error trace

* deprecated SchedulerConfig.Server in favor of .HealthServer and .MetricsServer

* PR feedback

* remove metrics.go

* revendor

* PR feedback

* fix PR feedback

* update controlplane chart

* introduce pkg/logger.Format constants

* PR feedback

* Add default log format for Gardenlet

* Unify leader election options for GSched

* Use predicates for GSched

* Fix verify

* Address review comments

* Remove skip caller from logger

Co-authored-by: Tim Usner <tim.usner@sap.com>
@rfranzke rfranzke changed the title use ctrlruntime Manager to manage shoot scheduler controller Introduce controller-runtime manager in gardener-scheduler and switch Shoot controller Jul 19, 2022
@gardener-prow gardener-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2022
@rfranzke rfranzke changed the title Introduce controller-runtime manager in gardener-scheduler and switch Shoot controller [scheduler] Introduce controller-runtime manager and switch Shoot controller Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timuthy timuthy timuthy left review comments

@timebertt timebertt timebertt approved these changes

@rfranzke rfranzke rfranzke left review comments

Assignees

@timebertt timebertt

Labels
area/dev-productivity Developer productivity related (how to improve development) kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.
9 participants
@xrstf @gardener-robot @rfranzke @timebertt @timuthy @AndreasBurger @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3