[GEP-28] Allow to configure `hostNetwork`, `replicas` and `tolerations` for controller installations. #11527

ScheererJ · 2025-02-26T12:40:10Z

How to categorize this PR?

/area ipcei
/kind enhancement

What this PR does / why we need it:

Allow to configure hostNetwork, replicas, tolerations and usable ports for controller installations.

In the autonomous shoot cluster scenario, some controller installations may need to run in the host network. They may also need to be scaled down in a single node scenario or may need to tolerate additional taints during bootstrap. Running multiple controller installations in the host network also requires managing the usable ports so that two controllers
do not clash in their port usage.
This change adds the one flag to the controller installation reconciler that will trigger the corresponding changes.
Notably, the handling of the usable ports is focussed on the gardenadm scenario and is explicitly not supposed to be run as a controller.

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

Release note:

Allow to configure bootstrapping control plane nodes with controller installations by setting `hostNetwork`, `replicas`, `tolerations` and usable ports.

ScheererJ · 2025-02-26T12:40:19Z

/cc @rfranzke

timebertt

Thanks for the PR :)
/lgtm
Though I'm wondering if we should add unit tests for the changes. I know, I also haven't any when working on the file before 😄

gardener-prow · 2025-02-27T09:05:18Z

LGTM label has been added.

Git tree hash: ef42449e6dc00754e1a8573be8c9490de47b1883

ScheererJ · 2025-02-27T09:40:06Z

Thanks for the PR :) /lgtm Though I'm wondering if we should add unit tests for the changes. I know, I also haven't any when working on the file before 😄

I had an offline discussion with @rfranzke concerning this topic. He wanted to take a look and decide then.
We currently do not have a unit test for this reconciler, but there is an integration test. The integration test creates the reconciler in BeforeSuite(...) , though. Hence, some options are (no particular order):

Follow the tradition of this reconciler ;-) , i.e. no additional tests for mutations in the pods
Create a unit test for the reconciler
Adapt the integration test to use the new options (without changing the current structure)
Adapt the integration test and move the reconciler creation from BeforeSuite(...) to BeforeEach(...) so that there are tests with and without the new options

Feel free to share your thoughts what you think is the best one in this case.

timebertt · 2025-02-27T10:58:22Z

Ok, got it. My personal preference would be to extract the to-be-tested logic into a dedicated function that can be covered by unit tests.
Doing a controller setup in BeforeEach in integration tests works, but always comes with additional burdens. Also, I think testing all configuration cases of controllers should not be in the scope of integration tests but should rather be covered by unit tests

rfranzke · 2025-02-27T11:54:34Z

Looking at the changes of this PR, I agree with @timebertt. A unit test seems more reasonable for this scenario - it's a lightweight mutation, and re-instantiating a new manager + controller in an integration test seems a bit extreme (I'm not strictly against it, though).

ScheererJ · 2025-02-28T10:16:16Z

/hold

I will add a corresponding unit test.

ScheererJ · 2025-03-03T14:45:27Z

After offline discussion with @rfranzke, I updated the pull request including a unit test.

PTAL.

/unhold

timebertt

Thanks for reworking this PR :)

pkg/gardenlet/controller/controllerinstallation/controllerinstallation/reconciler.go

pkg/gardenlet/controller/controllerinstallation/controllerinstallation/reconciler_test.go

timebertt

Thanks for addressing my concerns. The PR generally looks good now.
I only left some testing nits.

pkg/gardenlet/controller/controllerinstallation/controllerinstallation/reconciler_test.go

timebertt

Lovely, this looks great now :)

gardener-prow · 2025-03-05T15:12:29Z

LGTM label has been added.

Git tree hash: 65ac7b80fb5ff2465a6be20fef86b53103d834d2

gardener-prow · 2025-03-05T15:12:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timebertt

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~~ [timebertt]

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

rfranzke · 2025-03-05T15:21:23Z

/hold

until #11593 is merged 😇

rfranzke

Sorry for joining the party only late, but since this has to be rebased after #11593 got merged (I guess 🥲), here a few nit suggestions 😇

pkg/gardenlet/controller/controllerinstallation/controllerinstallation/reconciler.go

ports for controller installations. In the autonomous shoot cluster scenario, some controller installations may need to run in the host network. They may also need to be scaled down in a single node scenario or may need to tolerate additional taints during bootstrap. Running multiple controller installations in the host network also requires managing the usable ports so that two controllers do not clash in their port usage. This change adds the one flag to the controller installation reconciler that will trigger the corresponding changes. Notably, the handling of the usable ports is focussed on the `gardenadm` scenario and is explicitly not supposed to be run as a controller.

Removed state from reconciler and reused `utilsnet.SuggestPort(...)` instead.

ScheererJ · 2025-03-06T12:32:53Z

/unhold

as #11593 has been merged.

rfranzke

/lgtm

gardener-prow · 2025-03-06T15:08:00Z

LGTM label has been added.

Git tree hash: 58e20e48320245af3421da4e28fa61ee3930f873

gardener-prow bot added area/ipcei IPCEI (Important Project of Common European Interest) kind/enhancement Enhancement, improvement, extension labels Feb 26, 2025

gardener-prow bot requested a review from oliver-goetz February 26, 2025 12:40

gardener-prow bot added the cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. label Feb 26, 2025

gardener-prow bot requested review from tobschli and rfranzke February 26, 2025 12:40

gardener-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 26, 2025

ScheererJ mentioned this pull request Feb 26, 2025

☂️ [GEP-28] Autonomous Shoot Clusters #2906

Open

timebertt reviewed Feb 27, 2025

View reviewed changes

gardener-prow bot assigned timebertt Feb 27, 2025

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2025

gardener-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2025

ScheererJ force-pushed the gep-28/controllerinstallation-with-hostnetwork-replicas-tolerations branch from 46a5666 to 14cd422 Compare March 3, 2025 14:41

gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2025

gardener-prow bot requested a review from timebertt March 3, 2025 14:41

gardener-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 3, 2025

gardener-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2025

timebertt reviewed Mar 3, 2025

View reviewed changes

timebertt reviewed Mar 5, 2025

View reviewed changes

timebertt approved these changes Mar 5, 2025

View reviewed changes

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2025

gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2025

gardener-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2025

rfranzke reviewed Mar 5, 2025

View reviewed changes

gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2025

ScheererJ added 7 commits March 6, 2025 13:25

Refactor SuggestPort function to be able to reuse it in gardenadm

eac8a23

Addressed review feedback.

9a2bb71

Removed state from reconciler and reused `utilsnet.SuggestPort(...)` instead.

Fix import alias

978a13d

Adapt documentation to include autonomous shoot cluster case

c427799

Address review feedback

3b82f17

Address review feedback

be8bdbd

ScheererJ force-pushed the gep-28/controllerinstallation-with-hostnetwork-replicas-tolerations branch from e68b656 to be8bdbd Compare March 6, 2025 12:31

gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2025

gardener-prow bot requested a review from timebertt March 6, 2025 12:31

gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2025

gardener-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2025

Fix format

2646f7c

rfranzke reviewed Mar 6, 2025

View reviewed changes

gardener-prow bot assigned rfranzke Mar 6, 2025

gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2025

gardener-prow bot merged commit 9511de2 into gardener:master Mar 6, 2025
19 checks passed

[GEP-28] Allow to configure hostNetwork, replicas and tolerations for controller installations. #11527

[GEP-28] Allow to configure hostNetwork, replicas and tolerations for controller installations. #11527

Uh oh!

Conversation

ScheererJ commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ScheererJ commented Feb 26, 2025

Uh oh!

timebertt left a comment

Choose a reason for hiding this comment

Uh oh!

gardener-prow bot commented Feb 27, 2025

Uh oh!

ScheererJ commented Feb 27, 2025

Uh oh!

timebertt commented Feb 27, 2025

Uh oh!

rfranzke commented Feb 27, 2025

Uh oh!

ScheererJ commented Feb 28, 2025

Uh oh!

ScheererJ commented Mar 3, 2025

Uh oh!

timebertt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timebertt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timebertt left a comment

Choose a reason for hiding this comment

Uh oh!

gardener-prow bot commented Mar 5, 2025

Uh oh!

gardener-prow bot commented Mar 5, 2025

Uh oh!

rfranzke commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfranzke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ScheererJ commented Mar 6, 2025

Uh oh!

rfranzke left a comment

Choose a reason for hiding this comment

Uh oh!

gardener-prow bot commented Mar 6, 2025

Uh oh!

Uh oh!

Uh oh!

[GEP-28] Allow to configure `hostNetwork`, `replicas` and `tolerations` for controller installations. #11527

[GEP-28] Allow to configure `hostNetwork`, `replicas` and `tolerations` for controller installations. #11527

ScheererJ commented Feb 26, 2025 •

edited

Loading

rfranzke commented Mar 5, 2025 •

edited

Loading