fix: avoid reconciling ended leases#846
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe lease controller now filters out already-ended leases at event admission time and adds tests for ended-label detection, predicate behavior, and label restoration during reconciliation. ChangesEnded Lease Skip
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@controller/internal/controller/lease_controller.go`:
- Around line 82-84: The early return in reconcile for lease status ended is
preventing the ended-label backfill from ever retrying when metadata update
fails. In the LeaseReconciler flow, move the `lease.Status.Ended` short-circuit
in `Reconcile` so it runs only after the ended label sync path completes, or
gate it on `isLeaseEnded(&lease)` being true as well. Make sure the label write
still happens for leases whose status is ended but that do not yet have
`jumpstarter.dev/lease-ended`, so downstream `MatchingActiveLeases` no longer
treats them as active.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f80c2bd0-dbc5-4c2d-bc06-2fc80a4f30e2
📒 Files selected for processing (1)
controller/internal/controller/lease_controller.go
bennyz
force-pushed
the
ended-lease-invalid
branch
2 times, most recently
from
62c3ae1 to
35002c5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controller/internal/controller/lease_controller_test.go (1)
1712-1769: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese specs don’t actually prove the new skip behavior.
Both cases call
reconcileLease()directly, which bypassesSetupWithManagerentirely, so the newskipEndedpredicate is never exercised. Also, theRequeueAfter == 0assertion does not distinguish the early-return path from any normal reconcile that already converges on an ended lease. Please make one test observe a skipped operation (for example via a spy/fake client) or add a focused predicate-level test.🤖 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 `@controller/internal/controller/lease_controller_test.go` around lines 1712 - 1769, The current lease tests in lease_controller_test.go do not exercise the new skipEnded behavior because they call reconcileLease directly instead of going through the manager/predicate path. Update these specs to verify the skip path itself, either by adding a focused test for skipEnded in SetupWithManager or by using a spy/fake client to observe that reconciliation is skipped. Keep the existing end-of-lease cases tied to reconcileLease only for state changes, and add an assertion that specifically proves the predicate prevents a reconcile from running rather than relying on RequeueAfter.
🤖 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.
Nitpick comments:
In `@controller/internal/controller/lease_controller_test.go`:
- Around line 1712-1769: The current lease tests in lease_controller_test.go do
not exercise the new skipEnded behavior because they call reconcileLease
directly instead of going through the manager/predicate path. Update these specs
to verify the skip path itself, either by adding a focused test for skipEnded in
SetupWithManager or by using a spy/fake client to observe that reconciliation is
skipped. Keep the existing end-of-lease cases tied to reconcileLease only for
state changes, and add an assertion that specifically proves the predicate
prevents a reconcile from running rather than relying on RequeueAfter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccd2b42a-ee83-466b-b2a1-7f66ddc46c26
📒 Files selected for processing (2)
controller/internal/controller/lease_controller.gocontroller/internal/controller/lease_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/internal/controller/lease_controller.go
bennyz
force-pushed
the
ended-lease-invalid
branch
from
35002c5 to
2e06178
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controller/internal/controller/lease_controller_test.go (1)
1808-1831: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the short-circuit assertion.
RequeueAfter == 0doesn't prove the second reconcile skipped the expensive path; a full reconcile can still return zero. Since this PR is specifically about avoiding work for ended leases, it would be better to assert that the second reconcile performs no writes (for example, unchangedresourceVersion/generation) or use a client spy to verifyStatus().Update/Updateare not reached.🤖 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 `@controller/internal/controller/lease_controller_test.go` around lines 1808 - 1831, The current short-circuit check in reconcileLease only asserts RequeueAfter is zero, which does not prove the ended-lease path skipped reconciliation work. Strengthen the test by verifying no writes happen on the second call to reconcileLease, such as asserting the lease’s resourceVersion/generation stays unchanged after the second reconcile or by using a spy/mock to confirm Update and Status().Update are not invoked; use the existing reconcileLease, getLease, and k8sClient setup to locate and adjust the test.
🤖 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 `@controller/internal/controller/lease_controller_test.go`:
- Around line 1059-1124: The skipEnded test is duplicating the production
predicate instead of validating the real filter used by SetupWithManager, so it
can diverge unnoticed. Extract the predicate logic into a shared helper (or
reuse the actual predicate from SetupWithManager) and update the tests to call
that shared symbol rather than rebuilding predicate.Funcs locally. Also replace
the current “transition into ended” case with the important unlabeled-ended
update path, using a Lease whose Status.Ended is true but whose ended label is
still absent, so the filter behavior matches the reconciler backfill flow.
---
Nitpick comments:
In `@controller/internal/controller/lease_controller_test.go`:
- Around line 1808-1831: The current short-circuit check in reconcileLease only
asserts RequeueAfter is zero, which does not prove the ended-lease path skipped
reconciliation work. Strengthen the test by verifying no writes happen on the
second call to reconcileLease, such as asserting the lease’s
resourceVersion/generation stays unchanged after the second reconcile or by
using a spy/mock to confirm Update and Status().Update are not invoked; use the
existing reconcileLease, getLease, and k8sClient setup to locate and adjust the
test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd51b178-e689-4c9d-b006-7780a39df465
📒 Files selected for processing (2)
controller/internal/controller/lease_controller.gocontroller/internal/controller/lease_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/internal/controller/lease_controller.go
bennyz
force-pushed
the
ended-lease-invalid
branch
from
2e06178 to
c73f1c9
Compare
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assited-by: claude-opus-4.6
bennyz
force-pushed
the
ended-lease-invalid
branch
from
c73f1c9 to
edc795e
Compare
|
looks good from my side as well. |
4 participants
Otherwise this will spam the logs and can check the server if there are invalid leases