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

Refactor Gardener components to native controller-runtime components #4251

Closed
76 tasks done
rfranzke opened this issue Jun 23, 2021 · 13 comments
Closed
76 tasks done

Refactor Gardener components to native controller-runtime components #4251

rfranzke opened this issue Jun 23, 2021 · 13 comments
Assignees
Labels
area/dev-productivity Developer productivity related (how to improve development) area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/scalability Scalability related kind/cleanup Something that is not needed anymore and can be cleaned up kind/enhancement Enhancement, improvement, extension kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly priority/3 Priority (lower number equals higher priority) roadmap/internal Roadmap for our team-internal goals, e.g. drive up seed utilization

Comments

@rfranzke
Copy link
Member

rfranzke commented Jun 23, 2021

How to categorize this issue?

/area open-source dev-productivity scalability
/kind cleanup technical-debt

What would you like to be added:

  • gardener-scheduler, gardener-controller-manager and gardenlet should be refactored such that they become native controller-runtime components.
  • gardener-resource-manager, gardener-admission-controller, gardener-seed-admission-controller are already native controller-runtime components.
  • gardener-apiserver is special anyways because it's neither a controller nor a webhook server.

Why is this needed:

  • Less code
  • Less duplication/hand-crafted logic
  • Easier maintenance
  • Metrics out-of-the-box
  • Streamlined logging

Steps

@rfranzke rfranzke added the kind/enhancement Enhancement, improvement, extension label Jun 23, 2021
@gardener-robot gardener-robot added area/dev-productivity Developer productivity related (how to improve development) area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/scalability Scalability related kind/cleanup Something that is not needed anymore and can be cleaned up kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly priority/3 Priority (lower number equals higher priority) labels Jun 23, 2021
@timebertt
Copy link
Member

All controllers should implement the reconcile.Reconciler interface (already done for GSCHED, GCM)

I'm currently working on this for the remaining gardenlet controllers as part of #2822

@timebertt
Copy link
Member

All controllers should implement the reconcile.Reconciler interface (already done for GSCHED, GCM)

I'm currently working on this for the remaining gardenlet controllers as part of #2822

It's mostly done with #4503.
Though, the shoot controller should be further refactored into individual reconcile.Reconcilers instead of using reconcile.Funcs.

@rfranzke
Copy link
Member Author

/assign @timebertt
/roadmap internal
/milestone 2021-Q4

@gardener-robot gardener-robot added this to the 2021-Q4 milestone Sep 29, 2021
@gardener-robot gardener-robot added the roadmap/internal Roadmap for our team-internal goals, e.g. drive up seed utilization label Sep 29, 2021
@timebertt
Copy link
Member

/in-progress

@timebertt
Copy link
Member

timebertt commented Oct 25, 2021

refactor remaining controllers to reconcile.Reconciler interface (extensions controller: controllerinstallation/shootstate)

I tried to accomplish this, but I faced a few difficulties with the current implementation/architecture of the package:

  • the controller actually contains 2 controllers: a controllerinstallation controller and a shootstate controller, but both controllers are initialized and built from the shared artifacts map
  • the controllerinstallation reconcilers share a common state (kindToRequiredTypes) which is not so easy to do if they are split into dedicated structs

Given, that this controller package anyways needs significant restructuring work (actually it almost needs to be rebuilt from scratch) in order to make it work with native c-r constructs, I decided to not refactor this right now to reconcile.Reconciler. This provides only little to no value right now over reconcile.Func, as it cannot be simply added as a native controller to a c-r manager, so it needs to be touched again anyways.
Once we are able to leverage all the c-r goodies to get rid of all our custom coding, we can rebuild it in a clean way.

So this will be done in a dedicated PR as part of step

add native c-r controllers to manager including predicates, event handlers, etc. as needed (can be done step by step)

@timebertt
Copy link
Member

check whether we can remove active worker counting in our controllers (dummy graceful termination)

I look into this and played around with it a little bit. The result:

  • current implementation doesn't prevent the process from exiting, if items are left in the queue or workers have not finished yet
  • too much effort to fix current implementation
  • could remove it now in favor of graceful termination feature, that we will get with native c-r controllers
  • but it also doesn't hurt, so let's keep it for now and simply remove it together in individual refactorings of controllers

Hence, I marked the step as completed for now.
cc @rfranzke

@acumino acumino removed their assignment Oct 13, 2022
@shafeeqes shafeeqes removed their assignment Jan 5, 2023
@rfranzke rfranzke unpinned this issue Jan 25, 2023
@gardener-ci-robot
Copy link
Contributor

The Gardener project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close

/lifecycle stale

@gardener-prow gardener-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 22, 2023
@rfranzke
Copy link
Member Author

/remove-lifecycle stale

@gardener-prow gardener-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 22, 2023
@timebertt
Copy link
Member

Now that #7147 is merged, we can close this issue, correct? :)

@shafeeqes
Copy link
Contributor

Yes 🚀
/close

@gardener-prow gardener-prow bot closed this as completed Mar 29, 2023
@gardener-prow
Copy link
Contributor

gardener-prow bot commented Mar 29, 2023

@shafeeqes: Closing this issue.

In response to this:

Yes 🚀
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev-productivity Developer productivity related (how to improve development) area/open-source Open Source (community, enablement, contributions, conferences, CNCF, etc.) related area/scalability Scalability related kind/cleanup Something that is not needed anymore and can be cleaned up kind/enhancement Enhancement, improvement, extension kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly priority/3 Priority (lower number equals higher priority) roadmap/internal Roadmap for our team-internal goals, e.g. drive up seed utilization
Projects
None yet
Development

No branches or pull requests

10 participants