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

[controller-manager] Switch Seed controller to controller-runtime #6636

Merged
merged 14 commits into from
Sep 13, 2022
Merged

[controller-manager] Switch Seed controller to controller-runtime #6636

merged 14 commits into from
Sep 13, 2022

Conversation

rfranzke
Copy link
Member

@rfranzke rfranzke commented Sep 7, 2022

How to categorize this PR?

/area dev-productivity scalability
/kind enhancement

What this PR does / why we need it:
Refactor the Seed controller to controller-runtime.

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
  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 labels Sep 7, 2022
@gardener-prow gardener-prow bot added kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 7, 2022
This was referenced Sep 7, 2022
Closed
Merged
@ary1992
Copy link
Contributor

ary1992 commented Sep 8, 2022

/assign

plkokanov
plkokanov requested changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

Looks very good so far. I still have to check the lifecycle and secret controllers integration tests.

docs/concepts/controller-manager.md Outdated Show resolved Hide resolved
pkg/controllermanager/controller/seed/lifecycle/reconciler.go Outdated Show resolved Hide resolved
test/integration/controllermanager/seed/backupbucketscheck/backupbucketscheck_suite_test.go Outdated Show resolved Hide resolved
plkokanov
plkokanov requested changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

Went through the PR one more time and I have a few more comments.

And I have some questions regarding the lifecycle and secrets controllers integration tests. I see that for some objects you hardcoded the names, instead of using generateName. This will make it impossible to run tests in parallel when we use the stress util. Was that intentional?

docs/concepts/controller-manager.md Outdated Show resolved Hide resolved
pkg/controllermanager/controller/seed/backupbucketscheck/add.go Show resolved Hide resolved
pkg/controllermanager/controller/seed/extensionscheck/add.go Show resolved Hide resolved
docs/concepts/controller-manager.md Outdated Show resolved Hide resolved
pkg/controllermanager/controller/seed/extensionscheck/add.go Outdated Show resolved Hide resolved
pkg/controllermanager/controller/seed/secrets/add.go Outdated Show resolved Hide resolved
test/integration/controllermanager/seed/lifecycle/lifecycle_suite_test.go Outdated Show resolved Hide resolved
test/integration/controllermanager/seed/lifecycle/lifecycle_test.go Outdated Show resolved Hide resolved
test/integration/controllermanager/seed/lifecycle/lifecycle_test.go Outdated Show resolved Hide resolved
pkg/controllermanager/controller/seed/lifecycle/reconciler.go Show resolved Hide resolved
@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2022
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2022
@rfranzke
Copy link
Member Author

@plkokanov I have addressed or commented on all your suggestions/questions, thanks for your review! I have also made sure that stress-testing of all integration tests suites for the seed controller is possible now. Please check again :)

@rfranzke rfranzke requested review from plkokanov and removed request for timebertt September 12, 2022 06:23
@ary1992
Copy link
Contributor

ary1992 commented Sep 12, 2022

/unassign

@rfranzke
Use configured syncPeriod instead of hard-coded value
c85f946 
Previously, the `.controllers.seed.syncPeriod` value was defaulted to `30s`, however it was completely unused.
Now, it is defaulted to `10s` and used as part of the lifecycle controller. Earlier, the lifecycle controller was using the hard-coded `10s` value.

Also, the hard-coded `syncPeriodAfterExpiredMonitoringPeriod` value was dropped in favor of `syncPeriod` only (there is no clear benefit to use another sync period here).
Also, the duplicative read of the `Lease` object was dropped (there is no clear benefit to just read the very same Lease object twice in a row, especially since it's always done with the cached client).
@rfranzke
Integration test for lifecycle reconciler
3de2bb2
@rfranzke
Switch lifecycle reconciler to native controller-runtime controller
c278d0f
@rfranzke
Drop no longer needed code
bc82b32
@rfranzke
Address PR review feedback
a02fef8
@rfranzke
Address PR review feedback
eb25915
@rfranzke
Deflake ExtensionsCheck integration test suite
33599ca
@rfranzke
Align BackupBucketsCheck integration test suite
426cfba
@rfranzke
Fix invalid gardenlet Helm chart value config
bc100d0
@gardener-prow
Copy link
Contributor

gardener-prow bot commented Sep 13, 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 bc100d0 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.

@gardener-prow gardener-prow bot added cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. and removed cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Sep 13, 2022
@plkokanov
Copy link
Contributor

Thanks for addressing the comments! I tested it out with the local setup and didn't see any problems.

/lgtm
/approve

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2022
@gardener-prow
Copy link
Contributor

gardener-prow bot commented Sep 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: plkokanov

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 Sep 13, 2022
@rfranzke
Copy link
Member Author

/cla

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Sep 13, 2022

Successfully reached out to cla-assistant.io to initialize recheck of PR #6636

@gardener-prow gardener-prow bot added cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. and removed cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. labels Sep 13, 2022
@gardener-prow gardener-prow bot merged commit 7cab055 into gardener:master Sep 13, 2022
@rfranzke rfranzke deleted the refactor/seed-ctrl branch September 13, 2022 12:43
@rfranzke rfranzke mentioned this pull request Sep 15, 2022
Closed
@rfranzke rfranzke changed the title Switch Seed controller to controller-runtime [controller-manager] Switch Seed controller to controller-runtime Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acumino acumino Awaiting requested review from acumino

@plkokanov plkokanov Awaiting requested review from plkokanov

Assignees

@plkokanov plkokanov

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