| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Improves operator shutdown behavior in leader-election scenarios by always installing a JVM shutdown hook and preventing System.exit(1) from being invoked during graceful shutdown, with a regression test covering the deadlock scenario.
Changes:
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| operator-framework-core/src/test/java/io/javaoperatorsdk/operator/LeaderElectionManagerTest.java | Adds a regression test for shutdown-hook/leader-election interaction to avoid JVM termination. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java | Changes shutdown hook installation behavior and improves shutdown documentation; deprecates old overload. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/LeaderElectionManager.java | Adds graceful-shutdown coordination to avoid System.exit during JVM shutdown; minor permission-check change. |
Sorry, something went wrong.
| @Test | ||
| void stopLeadingDoesNotInvokeSystemExitWhenStopWasCalledFirst() { | ||
| // When stop() is called before the onStopLeading callback fires (which is what happens when | ||
| // stop()'s future cancellation triggers the callback), stopLeading() must skip | ||
| // System.exit(1). Otherwise calling stop() from inside a JVM shutdown hook deadlocks against | ||
| // the java.lang.Shutdown class lock. If this regression is ever reintroduced, this test | ||
| // method would terminate the JUnit JVM via System.exit(1) instead of failing cleanly. | ||
| final var leaderElectionManager = leaderElectionManager(null); | ||
| leaderElectionManager.stop(); | ||
| leaderElectionManager.stopLeading(); | ||
| } |
| * <p>Internally this class wraps a Fabric8 {@link LeaderElector} that coordinates via a Kubernetes | ||
| * {@code Lease} resource (group {@value #COORDINATION_GROUP}, resource {@value #LEASES_RESOURCE}). | ||
| * When this pod acquires the lease, {@link #startLeading()} starts event processing on the | ||
| * controller manager. When the lease is lost or the leader-election future is cancelled, {@link | ||
| * #stopLeading()} is invoked. |
There was a problem hiding this comment.
I don't think we need that level of information, especially because this touches the internal implementation, which might change over time, and addresses concepts that have not been defined in this scope.
Sorry, something went wrong.
There was a problem hiding this comment.
As part of the last commit I removed the JavaDoc for this class and also the ones made as part of the installShutdownHook methods. Please let me know if you want to stick to the current installShutdownHook method JavaDocs.
Sorry, something went wrong.
There was a problem hiding this comment.
I didn't mean to remove all the javadoc 😉
I just thought that the internal details were just a bit much but the overall description of the purpose and behavior was OK.
Sorry, something went wrong.
There was a problem hiding this comment.
Got it, thanks for the clarification! I've restored the class-level JavaDoc on LeaderElectionManager along with the JavaDocs on Operator#stop() and Operator#installShutdownHook(). The paragraph that I read as the "internal details" bit (the one naming the Fabric8 LeaderElector and the Lease group/resource constants) is the only piece I kept out. Let me know if that's the right trim or if there's more to pull out.
Sorry, something went wrong.
|
Leader loses lead (onStopLeading fires without prior stop()) was thinking about this scenario, if that could still lead to a deadlock. So if leader looses lead, there is always a shutdown hook installed, but also calls System.exit(). Wouldn't this lead to a deadlock? |
Sorry, something went wrong.
In the lost-lead scenario we will have this flow:
So, Shutdown.lock is acquired exactly once (by Thread-LE, never recursively), so no deadlock. If you to increase the test coverage for this I can add a second regression test that simulates the lost-lead-then-shutdown sequence on top of the existing one. |
Sorry, something went wrong.
…tor#installShutdownHook(Duration) Signed-off-by: Dennis-Mircea Ciupitu <dennis.mircea.ciupitu@gmail.com>
Signed-off-by: Dennis-Mircea Ciupitu <dennis.mircea.ciupitu@gmail.com>
There was a problem hiding this comment.
Awesome, LGTM! thx!
Sorry, something went wrong.
### What changes were proposed in this pull request? This PR aims to upgrade `java-operator-sdk` to 5.3.5. ### Why are the changes needed? To bring the latest bug fixes and improvements from `java-operator-sdk` 5.3.5: - https://github.com/operator-framework/java-operator-sdk/releases/tag/v5.3.5 - operator-framework/java-operator-sdk#3383 - operator-framework/java-operator-sdk#3414 - operator-framework/java-operator-sdk#3365 - operator-framework/java-operator-sdk#3382 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 4.8 Closes #712 from dongjoon-hyun/SPARK-57508. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request? This PR aims to upgrade `java-operator-sdk` to 5.3.5. ### Why are the changes needed? To bring the latest bug fixes and improvements from `java-operator-sdk` 5.3.5: - https://github.com/operator-framework/java-operator-sdk/releases/tag/v5.3.5 - operator-framework/java-operator-sdk#3383 - operator-framework/java-operator-sdk#3414 - operator-framework/java-operator-sdk#3365 - operator-framework/java-operator-sdk#3382 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 4.8 Closes apache#712 from dongjoon-hyun/SPARK-57508. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
| Back | FazBrowse Home | New Git URL |
Summary
Resolves all three concerns raised in #3376:
What changed
Fix the underlying deadlock (LeaderElectionManager)
The deadlock from #1614 was very specific: Operator#stop() called from inside a JVM shutdown hook would cancel the leader-election future, which fired the onStopLeading callback, which called System.exit(1). That System.exit then blocked indefinitely on the java.lang.Shutdown class lock that the shutdown hook thread itself was already holding.
This PR breaks the recursion. LeaderElectionManager now carries a stoppingGracefully AtomicBoolean, set in stop() before the future is cancelled and checked at the top of stopLeading(). When the flag is set, stopLeading() returns immediately instead of invoking System.exit. The "restart on lost lead" behavior (when the leader-election library detects a real lost lease without a prior stop()) is preserved because that path runs without the flag ever being set.
Re-enable the shutdown hook unconditionally (Operator)
With the deadlock fixed at its source, the conditional in Operator#installShutdownHook() is no longer required. The hook is now registered regardless of leader-election state, and the "Leader election is on, shutdown hook will not be installed." warn log line is removed. A leader pod receiving SIGTERM will run the hook, which calls Operator#stop(), which now cleanly cancels the leader-election future and releases the lease.
Deprecate installShutdownHook(Duration) (Operator)
The Duration argument has been dead since #2479. This PR adds a new no-arg installShutdownHook() overload (the recommended replacement) whose JavaDoc points users at ConfigurationServiceOverrider#withReconciliationTerminationTimeout(Duration) as the real configuration knob. The existing installShutdownHook(Duration) is marked @Deprecated(forRemoval = true) and delegates to the no-arg overload. The Duration parameter is kept only for source and binary compatibility.
Documentation additions
Regression test
LeaderElectionManagerTest#stopLeadingDoesNotInvokeSystemExitWhenStopWasCalledFirst calls stop() and then stopLeading() directly. If the graceful-shutdown short-circuit is ever reintroduced as System.exit(1), this test method would terminate the JUnit JVM rather than failing cleanly, making the regression impossible to miss in CI. LeaderElectionManager#stopLeading was lowered from private to protected to enable the test (and to allow subclasses to extend the behavior).
Behavior summary by scenario
Test plan