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

[gardenlet] Switch ControllerInstallation controller to controller-runtime #6733

Merged
merged 16 commits into from
Oct 7, 2022
Merged

[gardenlet] Switch ControllerInstallation controller to controller-runtime #6733

merged 16 commits into from
Oct 7, 2022

Conversation

rfranzke
Copy link
Member

How to categorize this PR?

/area dev-productivity scalability
/kind enhancement

What this PR does / why we need it:
Refactor the ControllerInstallation controller to controller-runtime. It consists of several reconcilers, so there are some empty separator commits to allow easy navigation through the PR.

Also, we are now filtering the cache at manager/cluster level already. This way, we ensure that the gardenlet only sees ControllerInstallations which are related to the Seed it is responsible for. This allows us to drop the custom filtering in the controllers event handlers. It also reduces memory usage of the gardenlet.

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

Special notes for your reviewer:
Generally, we want to follow this cookbook while refactoring existing controllers:

  1. Add documentation
  2. Add integration test based on envtest (if not already present)
  3. Switch controller to controller-runtime

Release note:

NONE

@gardener-prow gardener-prow bot added area/dev-productivity Developer productivity related (how to improve development) area/scalability Scalability related kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Sep 23, 2022
@gardener-prow gardener-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 23, 2022
@rfranzke rfranzke mentioned this pull request Sep 23, 2022
Closed
76 tasks
@rfranzke
Copy link
Member Author

/test pull-gardener-e2e-kind

shafeeqes
shafeeqes reviewed Sep 26, 2022
View reviewed changes
Copy link
Contributor

@shafeeqes shafeeqes 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 well structured PR.
Generally looks good.
/needs-second-opinion

pkg/gardenlet/controller/controllerinstallation/care/reconciler.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/controllerinstallation/care/reconciler_test.go Show resolved Hide resolved
@ary1992
Copy link
Contributor

ary1992 commented Sep 28, 2022

/assign

@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2022
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2022
@rfranzke
Copy link
Member Author

rfranzke commented Oct 4, 2022

@shafeeqes @ary1992 The PR is rebased and the feedback is addressed, please check it out again.

shafeeqes
shafeeqes reviewed Oct 6, 2022
View reviewed changes
Copy link
Contributor

@shafeeqes shafeeqes left a comment

Choose a reason for hiding this comment

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

I went through the PR again, found only one minor nit.
Otherwise, lgtm.

pkg/gardenlet/controller/controllerinstallation/controllerinstallation/add.go Outdated Show resolved Hide resolved
@shafeeqes
Copy link
Contributor

/lgtm

@gardener-prow gardener-prow bot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 6, 2022
@gardener-prow gardener-prow bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 6, 2022
ary1992
ary1992 reviewed Oct 7, 2022
View reviewed changes
Copy link
Contributor

@ary1992 ary1992 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
Just minor nits.

pkg/registry/core/controllerinstallation/strategy.go Outdated Show resolved Hide resolved
docs/concepts/gardenlet.md Outdated Show resolved Hide resolved
pkg/gardenlet/controller/controllerinstallation/care/reconciler.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/controllerinstallation/care/reconciler.go Outdated Show resolved Hide resolved
@rfranzke
Use spec.seedRef.name field selector for ControllerInstallation c…
173b202 
…ache

This way, we don't have to filter unrelated `ControllerInstallation`s at
the controller level (e.g., with predicates). The underlying
controller-runtime cache already makes sure that we only see objects
which are related to our seed now.
@rfranzke
Properly register indexed fields in registry
96538d8 
Without this, the field selector watch for the gardenlet does not work.
@rfranzke
-------- empty separator commit --------
90a4942
@rfranzke
Add documentation for care reconciler
213fedc
@rfranzke
Switch care reconciler to native controller-runtime controller
76979a8
@rfranzke
Limit exponential backoff to sync period
451a815 
Similar to other refactored care controllers.
Now we can properly/cleanly handle and return errors occuring during reconciliation.
@rfranzke
Watch ManagedResources and map to ControllerInstallations
dd340af 
The main `ControllerInstallation` controller now adds a label to the created `ManagedResource` in the seed so that the care controller can map the `ManagedResource` to the owning `ControllerInstallation`.
Also, the predicate about changed conditions from the seed's `ExtensionsCheck` controller was moved to the general predicate utils so that it can be reused in the care controller now.
@rfranzke
-------- empty separator commit --------
cf7bd35
@rfranzke
Add documentation for main reconciler
3c9c27f
@rfranzke
Add integration test for main ControllerInstallation controller
fe35a3c
@rfranzke
Switch main reconciler to native controller-runtime controller
03bdc17
@rfranzke
Replace Reconciler.isResponsible function with a predicate
6ff02fe
@rfranzke
Drop no longer required code
45a03b8
@rfranzke
Address PR review feedback
42be4f5
@rfranzke
Address PR review feedback
d978eb2
@rfranzke rfranzke requested a review from ary1992 October 7, 2022 06:24
@rfranzke
Copy link
Member Author

rfranzke commented Oct 7, 2022

ref #6642 (comment)

/test pull-gardener-integration

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Oct 7, 2022

@rfranzke: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-gardener-check-vulnerabilities ca2b1f0 link false /test pull-gardener-check-vulnerabilities

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@shafeeqes
Copy link
Contributor

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2022
@rfranzke
Copy link
Member Author

rfranzke commented Oct 7, 2022

/approve

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Oct 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rfranzke

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2022
@gardener-prow gardener-prow bot merged commit f0a6992 into gardener:master Oct 7, 2022
@rfranzke rfranzke deleted the refactor/ctrlinst branch October 7, 2022 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ialidzhikov ialidzhikov Awaiting requested review from ialidzhikov

@plkokanov plkokanov Awaiting requested review from plkokanov

@ary1992 ary1992 Awaiting requested review from ary1992

@shafeeqes shafeeqes Awaiting requested review from shafeeqes

Assignees

@ary1992 ary1992

@shafeeqes shafeeqes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dev-productivity Developer productivity related (how to improve development) area/scalability Scalability related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rfranzke @ary1992 @shafeeqes