feat(controllers): skip reconciliation for terminals on hibernated shoots#511
feat(controllers): skip reconciliation for terminals on hibernated shoots#511petersutter wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe PR adds Shoot hibernation awareness to the Terminal controller. A new ChangesTerminal Shoot Hibernation Support
Sequence DiagramsequenceDiagram
participant Shoot as Shoot (Gardener API)
participant shootWakeUpPredicate
participant mapShootToTerminals
participant TerminalReconciler
participant isAnyReferencedShootHibernated
Note over Shoot: IsHibernated transitions true→false
Shoot->>shootWakeUpPredicate: Update event
shootWakeUpPredicate-->>mapShootToTerminals: event accepted
mapShootToTerminals->>TerminalReconciler: list Terminals via TerminalShootRef index
mapShootToTerminals-->>TerminalReconciler: enqueue reconcile requests
TerminalReconciler->>isAnyReferencedShootHibernated: check host/target ShootRefs
isAnyReferencedShootHibernated-->>TerminalReconciler: no hibernated Shoot found
TerminalReconciler->>TerminalReconciler: proceed with normal reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/terminal_hibernation_test.go (1)
727-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet the Shoot wake-up watch trigger reconciliation in this integration test.
The manual annotation update requeues the Terminal directly, so this test does not prove that the new Shoot watch and
mapShootToTerminalspath works afterIsHibernatedchanges tofalse.Proposed direction
patch := client.MergeFrom(shoot.DeepCopy()) shoot.Status.IsHibernated = false Expect(e.K8sClient.Status().Patch(ctx, shoot, patch)).To(Succeed()) - By("Triggering a reconcile to confirm hibernation check no longer blocks") - Eventually(func(g Gomega) { - t := &dashboardv1alpha1.Terminal{} - g.Expect(e.K8sClient.Get(ctx, terminalKey, t)).To(Succeed()) - - if t.Annotations == nil { - t.Annotations = map[string]string{} - } - - t.Annotations["test-wake-trigger"] = "true" - g.Expect(e.K8sClient.Update(ctx, t)).To(Succeed()) - }, timeout, interval).Should(Succeed()) - - By("Waiting for the terminal to progress past hibernation check (LastOperation transitions away from Succeeded)") + By("Waiting for the Shoot wake-up watch to re-enqueue the Terminal")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/terminal_hibernation_test.go` around lines 727 - 744, The test is still forcing reconciliation by updating the Terminal annotation, so it does not validate the Shoot wake-up watch path. In terminal_hibernation_test.go, after clearing Shoot.Status.IsHibernated in the wake-up scenario, remove the manual Terminal update/requeue and instead wait for the Shoot-to-Terminal watch flow to trigger reconciliation via the mapShootToTerminals path, using the existing e.K8sClient, shoot, and terminalKey references to assert the Terminal changes are observed after the Shoot becomes unhibernated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/terminal_controller.go`:
- Around line 303-308: The hibernation check in isAnyReferencedShootHibernated
should not swallow Shoot read errors other than NotFound; right now the caller
in the Terminal reconciliation path can continue with an unknown state. Update
the error handling so that only a NotFound result is treated as non-blocking,
while transient/RBAC/cache read failures are returned from
isAnyReferencedShootHibernated and propagated by the reconcile flow in
terminal_controller.go to trigger a requeue. Keep the existing normal event/log
behavior only for the confirmed hibernated case and use the Terminal
reconciliation logic to stop processing when the referenced Shoot cannot be read
reliably.
In `@controllers/terminal_hibernation_test.go`:
- Around line 718-724: The hibernation stability assertion in the Consistently
check should not treat a K8sClient.Get failure as success, because that allows
the test to pass if the Terminal is deleted. Update the check around
e.K8sClient.Get and terminalKey so that a missing Terminal makes the condition
fail, while still returning true only when t.Status.AttachServiceAccountName and
t.Status.PodName are both nil. Keep the logic localized to the consistency
predicate in terminal_hibernation_test.go.
---
Nitpick comments:
In `@controllers/terminal_hibernation_test.go`:
- Around line 727-744: The test is still forcing reconciliation by updating the
Terminal annotation, so it does not validate the Shoot wake-up watch path. In
terminal_hibernation_test.go, after clearing Shoot.Status.IsHibernated in the
wake-up scenario, remove the manual Terminal update/requeue and instead wait for
the Shoot-to-Terminal watch flow to trigger reconciliation via the
mapShootToTerminals path, using the existing e.K8sClient, shoot, and terminalKey
references to assert the Terminal changes are observed after the Shoot becomes
unhibernated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5ff6638-6e6f-4bec-83f4-0e8306e3c42b
📒 Files selected for processing (6)
api/v1alpha1/terminal_types.gocontrollers/controller_suite_test.gocontrollers/terminal_controller.gocontrollers/terminal_hibernation_test.gomain.gotest/common.go
…oots Watch Shoot resources and re-enqueue referencing Terminals on wake-up (IsHibernated: true → false). While hibernated, emit a Hibernated event and return early. Strips cached Shoots to metadata + hibernation status to reduce memory.
7f519e2 to
67f2054
Compare
|
Superseded by #512. Old branch name |
/area robustness
/kind enhancement
What this PR does / why we need it:
When a Shoot referenced by a Terminal's host or target credentials is hibernated, the terminal controller now skips reconciliation and emits a
Hibernatedevent. A Shoot watch with a wake-up predicate (IsHibernated: true → false) automatically re-enqueues affected Terminals when the Shoot comes back.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note:
Summary by CodeRabbit
New Features
Bug Fixes