ci-operator: Fix Release All Leases Unreachable Code#4988
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds a lease heartbeat mechanism via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
![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
🧹 Nitpick comments (1)
cmd/ci-operator/main.go (1)
2172-2172: Typo in function name: "Hearthbeating" should be "Heartbeating".The function name
startLeaseHearthbeatingcontains a typo - "Hearth" instead of "Heart".✏️ Proposed fix
-func (o *options) startLeaseHearthbeating() chan struct{} { +func (o *options) startLeaseHeartbeating() chan struct{} {Also update the call site at line 1172:
- stopHeartbeating := o.startLeaseHearthbeating() + stopHeartbeating := o.startLeaseHeartbeating()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ci-operator/main.go` at line 2172, Rename the function startLeaseHearthbeating to startLeaseHeartbeating and update its declaration accordingly, then find and update all call sites (e.g., any invocation like o.startLeaseHearthbeating()) to use o.startLeaseHeartbeating() so the identifier matches; ensure any related comments or documentation are updated to reflect the corrected spelling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ci-operator/main.go`:
- Around line 2172-2189: The ticker created in startLeaseHearthbeating (variable
t) is never stopped causing a resource leak; modify the goroutine in
startLeaseHearthbeating so the ticker is stopped when the goroutine exits
(either by adding defer t.Stop() at the top of the goroutine or by calling
t.Stop() in the case <-stopChan branch before breaking the loop) to ensure the
ticker is cleaned up when stopChan is closed; reference the function
startLeaseHearthbeating, the ticker variable t, and stopChan when making the
change.
---
Nitpick comments:
In `@cmd/ci-operator/main.go`:
- Line 2172: Rename the function startLeaseHearthbeating to
startLeaseHeartbeating and update its declaration accordingly, then find and
update all call sites (e.g., any invocation like o.startLeaseHearthbeating()) to
use o.startLeaseHeartbeating() so the identifier matches; ensure any related
comments or documentation are updated to reflect the corrected spelling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1c3c8bc-c682-45be-a3a6-591d25d3c861
📒 Files selected for processing (1)
cmd/ci-operator/main.go
danilo-gemoli
force-pushed
the
fix/ci-operator/release-all-leases
branch
from
d356acc to
7f826be
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ci-operator/main.go`:
- Around line 1172-1181: The deferred cleanup currently closes the
stopHeartbeating channel and then immediately calls o.leaseClient.ReleaseAll(),
which can race with the heartbeat goroutine; change startLeaseHearthbeating() to
return a stop-and-wait function instead of chan struct{} so callers can signal
shutdown and block until the heartbeat goroutine has fully exited before calling
ReleaseAll(); update the call site (where stopHeartbeating :=
o.startLeaseHearthbeating()) to assign the returned function and invoke it in
the defer (instead of close(stopHeartbeating)), and ensure the heartbeat
goroutine signals completion (e.g., via sync.WaitGroup or a done channel) inside
startLeaseHearthbeating() so ReleaseAll() runs only after Heartbeat() calls have
finished.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2f7b126-fbd7-4b54-aef9-e018de970b6b
📒 Files selected for processing (1)
cmd/ci-operator/main.go
| stopHeartbeating := o.startLeaseHearthbeating() | ||
| defer func() { | ||
| close(stopHeartbeating) | ||
| if l, err := o.leaseClient.ReleaseAll(); err != nil { | ||
| logrus.WithError(err).Errorf("Failed to release leaked leases (%v)", l) | ||
| } else if len(l) != 0 { | ||
| o.metricsAgent.Record(metrics.NewInsightsEvent(metrics.InsightLeaseReleased, metrics.Context{"released_count": len(l)})) | ||
| logrus.Warnf("Would leak leases: %v", l) | ||
| } | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd cmd/ci-operator && rg -n 'startLeaseHearthbeating' main.goRepository: openshift/ci-tools
Length of output: 182
🏁 Script executed:
cd cmd/ci-operator && rg -n -B5 -A20 'func.*startLeaseHearthbeating' main.goRepository: openshift/ci-tools
Length of output: 782
🏁 Script executed:
cd cmd/ci-operator && sed -n '1172,1181p' main.goRepository: openshift/ci-tools
Length of output: 506
Heartbeat goroutine can overlap with ReleaseAll() due to missing completion synchronization.
The cleanup defers a call that closes stopHeartbeating and immediately invokes ReleaseAll(), but closing the stop channel only signals the heartbeat goroutine to exit—it does not wait for completion. The goroutine may still be executing a Heartbeat() call when ReleaseAll() runs, creating non-deterministic lease cleanup behavior.
The startLeaseHearthbeating() function (lines 2172-2190) returns a channel, and its goroutine exits only after stopping the ticker and breaking the loop, both of which happen asynchronously after the channel is closed. Change the return type from chan struct{} to a function that both signals and waits for goroutine completion before allowing ReleaseAll() to proceed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ci-operator/main.go` around lines 1172 - 1181, The deferred cleanup
currently closes the stopHeartbeating channel and then immediately calls
o.leaseClient.ReleaseAll(), which can race with the heartbeat goroutine; change
startLeaseHearthbeating() to return a stop-and-wait function instead of chan
struct{} so callers can signal shutdown and block until the heartbeat goroutine
has fully exited before calling ReleaseAll(); update the call site (where
stopHeartbeating := o.startLeaseHearthbeating()) to assign the returned function
and invoke it in the defer (instead of close(stopHeartbeating)), and ensure the
heartbeat goroutine signals completion (e.g., via sync.WaitGroup or a done
channel) inside startLeaseHearthbeating() so ReleaseAll() runs only after
Heartbeat() calls have finished.
There was a problem hiding this comment.
ReleaseAll and Heartbeat are two functions synchronized by the same mutex. As far as I understand, these two scenario might happen:
Case 1:
ReleaseAll gets executed before Heartbeat does. In this case the client releases all the leases, therefore Heartbeat does nothing.
Case 2:
Heartbeat gets executed before ReleaseAll does. In this case the client updates all the leases, that will then be released by ReleaseAll.
I guess both of them are safe. Am I correct?
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the lease.Client implementation to verify the mutex synchronization.
🏁 Script executed:
#!/bin/bash
# Find lease client implementation
rg -n -A 30 'func.*ReleaseAll' pkg/lease/
Length of output: 1557
🏁 Script executed:
#!/bin/bash
# Find Heartbeat implementation
rg -n -A 20 'func.*Heartbeat' pkg/lease/
Length of output: 3324
@danilo-gemoli, you are correct. After checking the implementation in pkg/lease/client.go:
Heartbeat()(Line 159–179) acquiresc.Lock()and holds it for its entire execution.ReleaseAll()(Line 191–205) does the same — acquiresc.Lock()and holds it throughout.
Since both methods are fully protected by the same mutex for their complete duration, they are mutually exclusive by construction. Your two scenarios are both safe:
| Scenario | What happens |
|---|---|
ReleaseAll runs first |
Empties c.leases; when Heartbeat later acquires the lock it iterates an empty map and is a no-op. |
Heartbeat runs first |
Updates all leases; ReleaseAll then acquires the lock and cleanly releases them all. |
My original concern about a race was wrong — I had missed that the mutex serializes these two calls inside the client itself. Apologies for the noise, the current implementation is correct.
😄
(\_/)
(•_•)
/>🐇 you were right all along!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
/test e2e |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/integration Failures are not related to the PR |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/integration DetailsIn response to this:
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-sigs/prow repository. |
|
/test images |
|
@danilo-gemoli: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit 71d74df
into
openshift:main
3 participants
The
ReleaseAll()safety net was, indeed, unreachable:that is an infinity loop that never ends, therefore
o.leaseClient.ReleaseAll()won't be ever executed.This is an attempt to fix it by:
Summary by CodeRabbit