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

[gardenlet] Switch ManagedSeed controller to controller-runtime #7108

Merged
merged 18 commits into from
Dec 16, 2022

Conversation

ary1992
Copy link
Contributor

@ary1992 ary1992 commented Nov 30, 2022

How to categorize this PR?

/area dev-productivity scalability
/kind enhancement

What this PR does / why we need it:
Refactor the ManagedSeed 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 (if not already present)
Switch controller to controller-runtime

  • PR is in draft as the delete part of integration test is not yet complete and need some opinion on that.

Release note:

NONE

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Nov 30, 2022

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

@gardener-prow gardener-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 30, 2022
@gardener-prow gardener-prow bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 30, 2022
@shafeeqes shafeeqes self-assigned this Nov 30, 2022
@ary1992 ary1992 changed the title Refactor/managedseed ctrl [gardenlet] Switch ManagedSeed controller to controller-runtime Nov 30, 2022
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2022
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 PR, had a quick look and left some comments.

test/integration/gardenlet/managedseed/managedseed_test.go Outdated Show resolved Hide resolved
test/integration/gardenlet/managedseed/managedseed_test.go Outdated Show resolved Hide resolved
test/integration/gardenlet/managedseed/managedseed_test.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add_test.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add_test.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add_test.go Outdated Show resolved Hide resolved
test/integration/gardenlet/managedseed/managedseed_test.go Outdated Show resolved Hide resolved
@ary1992
Copy link
Contributor Author

ary1992 commented Dec 2, 2022

/test all

@ary1992
Copy link
Contributor Author

ary1992 commented Dec 2, 2022

/test all

@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2022
With this changes garden namespace validation can be disabled by disabling the ManagedSeed plugin for integrations test.
In the next commit namespace will be passed as an argument to controller.
This is done for assigning a unique garden namespace in case if integration test.
- enqueue with jitter delay logic from managed seed controller is moved to controllerutils.
- in the later commit jitter delay logic is removed from controller and instead EnqueueWithJitterDelay function is used.
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add_test.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add_test.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
test/integration/gardenlet/managedseed/managedseed_test.go Outdated Show resolved Hide resolved
@rfranzke rfranzke changed the title [gardenlet] Switch ManagedSeed controller to controller-runtime [gardenlet] Switch ManagedSeed controller to controller-runtime Dec 14, 2022
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.

Nice stuff. Just a few minor things

pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
pkg/gardenlet/controller/managedseed/add.go Outdated Show resolved Hide resolved
- Added `ShootClientMap` and `ImageVector` to the Reconciler struct.
- Removed `RateLimiter` from Reconciler struct as it is not needed here.
- Renamed `SeedOfManagedSeedFilterPredicate` to `SeedOfManagedSeedPredicate`
- Renamed `ManagedSeedFilterPredicate` to `ManagedSeedPredicate`
- GardenletConfiguration is passed in Reconciler struct instead of `ManagedSeedControllerConfiguration`
@ary1992
Copy link
Contributor Author

ary1992 commented Dec 15, 2022

/test pull-gardener-unit

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Dec 15, 2022

@ary1992: 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-apidiff 02ca98c link false /test pull-gardener-apidiff

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.

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

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

gardener-prow bot commented Dec 15, 2022

LGTM label has been added.

Git tree hash: 260ebba6aa7c9f623a71dbb18f3f84ee80131289

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.

/lgtm

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Dec 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shafeeqes

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 Dec 16, 2022
@gardener-prow gardener-prow bot merged commit e3dde16 into gardener:master Dec 16, 2022
seshachalam-yv pushed a commit to seshachalam-yv/gardener that referenced this pull request Dec 26, 2022
…ardener#7108)

* Move garden namespace validation of ManagedSeed to ManagedSeed plugins

With this changes garden namespace validation can be disabled by disabling the ManagedSeed plugin for integrations test.
In the next commit namespace will be passed as an argument to controller.

* Pass garden namespace and chart path as an argument to controller

This is done for assigning a unique garden namespace in case if integration test.

* Adapt configurable garden namespace in gardenlet charts

* Integration test

* Move EnqueueWithJitterDelay function to controllerutils

- enqueue with jitter delay logic from managed seed controller is moved to controllerutils.
- in the later commit jitter delay logic is removed from controller and instead EnqueueWithJitterDelay function is used.

* Switch ManagedSeed controller to native controller-runtime

* Adapt unit and integration tests

* Improve integration test [Delete case]

* Address PR review feedback

* Address PR review feedback

* Fix e2e test

* Introduce garden namespace for garden and garden namespace for shoot in the controller

- this is done to allow passing created namespace for shoot and garden instead of using hardcoded garden namespace for integration test.

* Move EnqueueWithJitterDelay function to controller as it is specific to controller and not a generic function

* Fix integration test

* Remove ControlledResourceEventHandler as it no longer used anywhere

* Address PR review feedback

* Address PR review feedback

- Added `ShootClientMap` and `ImageVector` to the Reconciler struct.
- Removed `RateLimiter` from Reconciler struct as it is not needed here.
- Renamed `SeedOfManagedSeedFilterPredicate` to `SeedOfManagedSeedPredicate`
- Renamed `ManagedSeedFilterPredicate` to `ManagedSeedPredicate`
- GardenletConfiguration is passed in Reconciler struct instead of `ManagedSeedControllerConfiguration`

* Rename file and minor Nit
@ary1992 ary1992 deleted the refactor/managedseed-ctrl branch February 16, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants