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] Refactor handling of Shoot main controller special cases #7189

Merged

Conversation

timebertt
Copy link
Member

How to categorize this PR?

/area dev-productivity quality
/kind technical-debt

What this PR does / why we need it:

Previously, special cases for confined and jittered shoot reconciliations (see docs) were handled by a few lines of boolean variable combinations, and the reconciliation due tracker.
We faced some bugs and regressions in this logic because it was not covered by tests at all, was not readable, and was not documented at all.
However, this logic is quite important for the stability of large-scale gardener installations.

This PR refactors this logic to make it more readable and well-documented and adds full unit test coverage.
Additionally, we can now use the helpers in both the shoot main reconciler and its event handler.
This allows us to use AddAfter in the event handler, instead of performing a reconciliation that will exit early with RequeueAfter (and allows us to get rid of the reconciliation due tracker).

Co-Authored-By @rfranzke

Which issue(s) this PR fixes:
Follow-up on #7082
Part of #4251

Special notes for your reviewer:

Release note:

NONE

@gardener-prow gardener-prow bot added area/dev-productivity Developer productivity related (how to improve development) area/quality Output qualification (tests, checks, scans, automation in general, etc.) related kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly labels Dec 12, 2022
@gardener-prow gardener-prow bot added 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 Dec 12, 2022
@rfranzke
Copy link
Member

/assign

@timuthy
Copy link
Member

timuthy commented Dec 12, 2022

/assign

@timebertt
Copy link
Member Author

This PR should go into the v1.62.0 release. Otherwise, the reconcileInMaintenanceOnly=false case is partially broken.
cc @oliver-goetz
/milestone v1.62

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Dec 12, 2022

@timebertt: You must be a member of the gardener/gardener-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

In response to this:

This PR should go into the v1.62.0 release. Otherwise, the reconcileInMaintenanceOnly=false case is partially broken.
cc @oliver-goetz
/milestone v1.62

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.

@timebertt timebertt added this to the v1.62 milestone Dec 12, 2022
@rfranzke
Copy link
Member

/assign @ary1992

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.

Thanks, we did a joint review with @ary1992

pkg/gardenlet/controller/shoot/shoot/helper/infos.go Outdated Show resolved Hide resolved
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.

Thank you @timebertt for the joint work on this, enjoyed it.

/lgtm
/approve

@timebertt
Copy link
Member Author

Thanks for the fast review, PTAL :)

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

gardener-prow bot commented Dec 13, 2022

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

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

/retest-required

@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2022
@timebertt timebertt force-pushed the refactor/shoot-ctrl-special-cases branch from 28d1e7c to 78026ac Compare December 13, 2022 12:24
@gardener-prow gardener-prow bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 13, 2022
@timuthy
Copy link
Member

timuthy commented Dec 13, 2022

/unassign
for now since there are enough reviewers and I won't make it in time.

@timebertt
Copy link
Member Author

Sorry folks, something went wrong during the rebase. It should be fixed now.

Copy link
Member

@acumino acumino left a comment

Choose a reason for hiding this comment

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

/lgtm

@acumino acumino added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022
@timebertt timebertt force-pushed the refactor/shoot-ctrl-special-cases branch from a24c415 to 75833b7 Compare December 13, 2022 14:28
@gardener-prow gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022
@gardener-prow
Copy link
Contributor

gardener-prow bot commented Dec 13, 2022

@timebertt: 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 75833b7 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 13, 2022
@gardener-prow gardener-prow bot merged commit 1d77e6b into gardener:master Dec 13, 2022
@timebertt timebertt deleted the refactor/shoot-ctrl-special-cases branch December 13, 2022 15:58
@rfranzke rfranzke changed the title Refactor handling of shoot controller special cases [gardenlet] Refactor handling of Shoot main controller special cases Dec 14, 2022
@timuthy
Copy link
Member

timuthy commented Dec 14, 2022

Kudos @timebertt and @rfranzke for touching this spaceship 👨‍🚀 Nice PR!

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/quality Output qualification (tests, checks, scans, automation in general, etc.) related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly 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

5 participants