[controller-manager] Switch `ManagedSeedSet` controller to `controller-runtime` #6742

ary1992 · 2022-09-26T11:32:48Z

How to categorize this PR?

/area dev-productivity scalability
/kind enhancement

What this PR does / why we need it:
Refactor the ManagedSeedSet 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:

Add documentation
Add integration test based on envtest
Switch controller to controller-runtime

Release note:

NONE

gardener-prow · 2022-09-26T11:32:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ary1992 · 2022-09-26T11:33:32Z

/test all

ary1992 · 2022-09-27T02:17:10Z

/test all

ary1992 · 2022-09-29T05:27:07Z

/test pull-gardener-e2e-kind

plkokanov

Thanks for the PR!
I had a very rough view on the PR. Will continue with it next week.

docs/concepts/controller-manager.md

pkg/controllermanager/controller/managedseedset/add.go

docs/concepts/controller-manager.md

rfranzke · 2022-10-04T07:38:43Z

/assign

pkg/controllermanager/controller/managedseedset/managedseedset_predicates.go

pkg/controllermanager/controller/managedseedset/add.go

pkg/controllermanager/controller/managedseedset/add_test.go

test/integration/controllermanager/managedseedset/managedseedset_suite_test.go

test/integration/controllermanager/managedseedset/managedseedset_test.go

pkg/controllermanager/controller/managedseedset/managedseedset_predicates.go

rfranzke

Can you please also

delete https://github.com/gardener/gardener/blob/master/pkg/utils/kubernetes/eventhandler.go and the test now (it's no longer used)?

enable

gardener/cmd/gardener-controller-manager/app/app.go

Lines 139 to 140 in 3f57fd0

    
           // TODO: enable this once we have refactored all controllers and added them to this manager 
        
           // LeaderElectionReleaseOnCancel: true,

and move up to the other leader election settings

docs/concepts/controller-manager.md

test/integration/controllermanager/managedseedset/managedseedset_suite_test.go

pkg/controllermanager/controller/managedseedset/managedseedset_predicates.go

pkg/controllermanager/controller/managedseedset/add.go

pkg/controllermanager/controller/managedseedset/managedseedset_predicates.go

ary1992 · 2022-10-08T05:33:53Z

Can you please also

delete https://github.com/gardener/gardener/blob/master/pkg/utils/kubernetes/eventhandler.go and the test now (it's no longer used)?

No it cannot be deleted as managedseed controller in gardenlet still uses it.

gardener/pkg/gardenlet/controller/managedseed/managedseed.go

Line 150 in e57e63d

ControllerPredicateFactory: kutil.ControllerPredicateFactoryFunc(c.filterSeed),

…ler(1)

…ler(2) Refactor shoot informer.

…ler(3) Refactor ManagedSeed informer

…ler(4) Refactor Seed informer

…ler(5) - Adapting integration test - Removing legacy controller implementation

- Renamed `set` to `managedSeedSet`. - Move predicates to add.go as it is done for other controllers. - Add `generationChange` predicate to `ManagedSeedSet` object.

rfranzke

Joint review with @plkokanov:

Thanks for addressing the previous comments, it looks quite promising now. Please also drop the managedseedset_ prefix from files in managedseedset package (except for the managedseedset_suite_test.go).

cmd/gardener-controller-manager/app/app.go

docs/concepts/controller-manager.md

pkg/controllermanager/controller/managedseedset/add.go

docs/concepts/controller-manager.md

test/integration/controllermanager/managedseedset/managedseedset_test.go

pkg/controllermanager/controller/managedseedset/add.go

gardener-prow · 2022-10-19T10:53:50Z

@ary1992: The following tests 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-dependency-watchdog-verify-image-build	`66422f9`	link	true	`/test pull-dependency-watchdog-verify-image-build`
pull-gardener-check-vulnerabilities	`5d43127`	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.

rfranzke · 2022-10-19T11:14:34Z

/test pull-gardener-e2e-kind-migration

rfranzke · 2022-10-19T11:33:49Z

It looks good to me now, thanks for addressing all the suggestions.
/approve
Waiting for final review and /lgtm of @plkokanov (and milestone assignment)

gardener-prow · 2022-10-19T11:34:04Z

[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:

~~OWNERS~~ [rfranzke]

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

plkokanov · 2022-10-19T11:58:05Z

Looks good to me as well.
/lgtm
/milestone v1.58

gardener-prow bot requested review from rfranzke and shafeeqes September 26, 2022 11:32

gardener-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 26, 2022

gardener-prow bot added cla: no Indicates the PR's author has not signed the cla-assistant.io CLA. 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 26, 2022

ary1992 force-pushed the refactor/managedseedset-ctrl branch from af60f5f to 9acdb34 Compare September 27, 2022 02:15

ary1992 marked this pull request as ready for review September 27, 2022 03:55

gardener-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2022

gardener-prow bot requested review from acumino and oliver-goetz September 27, 2022 03:55

ary1992 mentioned this pull request Sep 29, 2022

Refactor Gardener components to native controller-runtime components #4251

Closed

76 tasks

plkokanov requested changes Sep 30, 2022

View reviewed changes

gardener-prow bot assigned plkokanov Sep 30, 2022

acumino self-assigned this Oct 3, 2022

gardener-prow bot assigned rfranzke Oct 4, 2022

plkokanov requested changes Oct 4, 2022

View reviewed changes

rfranzke requested changes Oct 4, 2022

View reviewed changes

ary1992 force-pushed the refactor/managedseedset-ctrl branch from cff6528 to 547d277 Compare October 10, 2022 04:28

ary1992 requested a review from rfranzke October 10, 2022 05:22

ary1992 added 11 commits October 17, 2022 15:22

Add integration test for ManagedSeedSet controller

a18c69a

Switch ManagedSeedSet reconciler to native controller-runtime control…

8eb3cff

…ler(1)

Switch ManagedSeedSet reconciler to native controller-runtime control…

8333823

…ler(2) Refactor shoot informer.

Switch ManagedSeedSet reconciler to native controller-runtime control…

cef5d7d

…ler(3) Refactor ManagedSeed informer

Switch ManagedSeedSet reconciler to native controller-runtime control…

c69b12c

…ler(4) Refactor Seed informer

Switch ManagedSeedSet reconciler to native controller-runtime control…

138be8a

…ler(5) - Adapting integration test - Removing legacy controller implementation

Cleanup and Streamline controller

ba3f597

Rename set to managedSeedSet

3fb3c47

Address PR review feedback

8034866

- Renamed `set` to `managedSeedSet`. - Move predicates to add.go as it is done for other controllers. - Add `generationChange` predicate to `ManagedSeedSet` object.

Address PR review feedback

1377fd5

Fix flaky integration test

5d43127

ary1992 force-pushed the refactor/managedseedset-ctrl branch from 66422f9 to 5d43127 Compare October 18, 2022 05:09

ary1992 requested a review from plkokanov October 18, 2022 05:11

rfranzke requested changes Oct 18, 2022

View reviewed changes

ary1992 force-pushed the refactor/managedseedset-ctrl branch 2 times, most recently from 70ea03c to 43c2a19 Compare October 19, 2022 05:38

rfranzke requested changes Oct 19, 2022

View reviewed changes

Address PR review feedback

67993a8

ary1992 force-pushed the refactor/managedseedset-ctrl branch from 43c2a19 to 67993a8 Compare October 19, 2022 10:03

Remove managedseedset_ prefix from file names

7e0f916

gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2022

gardener-prow bot added this to the v1.58 milestone Oct 19, 2022

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2022

gardener-prow bot merged commit 70d9a0b into gardener:master Oct 19, 2022

ary1992 deleted the refactor/managedseedset-ctrl branch February 16, 2023 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller-manager] Switch `ManagedSeedSet` controller to `controller-runtime` #6742

[controller-manager] Switch `ManagedSeedSet` controller to `controller-runtime` #6742

ary1992 commented Sep 26, 2022

gardener-prow bot commented Sep 26, 2022

ary1992 commented Sep 26, 2022

ary1992 commented Sep 27, 2022

ary1992 commented Sep 29, 2022

plkokanov left a comment •

edited

Loading

rfranzke commented Oct 4, 2022

rfranzke left a comment

ary1992 commented Oct 8, 2022 •

edited

Loading

rfranzke left a comment

gardener-prow bot commented Oct 19, 2022 •

edited

Loading

rfranzke commented Oct 19, 2022

rfranzke commented Oct 19, 2022

gardener-prow bot commented Oct 19, 2022

plkokanov commented Oct 19, 2022

	// TODO: enable this once we have refactored all controllers and added them to this manager
	// LeaderElectionReleaseOnCancel: true,

[controller-manager] Switch ManagedSeedSet controller to controller-runtime #6742

[controller-manager] Switch ManagedSeedSet controller to controller-runtime #6742

Conversation

ary1992 commented Sep 26, 2022

gardener-prow bot commented Sep 26, 2022

ary1992 commented Sep 26, 2022

ary1992 commented Sep 27, 2022

ary1992 commented Sep 29, 2022

plkokanov left a comment • edited Loading

Choose a reason for hiding this comment

rfranzke commented Oct 4, 2022

rfranzke left a comment

Choose a reason for hiding this comment

ary1992 commented Oct 8, 2022 • edited Loading

rfranzke left a comment

Choose a reason for hiding this comment

gardener-prow bot commented Oct 19, 2022 • edited Loading

rfranzke commented Oct 19, 2022

rfranzke commented Oct 19, 2022

gardener-prow bot commented Oct 19, 2022

plkokanov commented Oct 19, 2022

[controller-manager] Switch `ManagedSeedSet` controller to `controller-runtime` #6742

[controller-manager] Switch `ManagedSeedSet` controller to `controller-runtime` #6742

plkokanov left a comment •

edited

Loading

ary1992 commented Oct 8, 2022 •

edited

Loading

gardener-prow bot commented Oct 19, 2022 •

edited

Loading