ZOOKEEPER-4712: Fix partially shutdown of ZooKeeperServer and its processors #2154

jonmv · 2024-04-04T11:34:46Z

This PR fixes the shutdown errors that were added in #157, and also avoids a common NPE during ZK shutdown from a learner, when the leader shuts down (first commit).
Together with #2111 and #2152, this should cover all the fixes in #1925.

We've had the forked ZK from #1925 running embedded in hundreds, if not thousands, of ZK clusters, with rolling restarts most days, and we've had zero cases of inconsistent data since we patched—one or a few cases per week before that.
(We still sometimes see ephemeral nodes remain after the leader is brutally taken down, i.e., with Runtime.halt(), but this looks different; it seems clearing out client sessions, and their ephemeral nodes, simply isn't done when death is too sudden.)

AlphaCanisMajoris · 2024-04-10T13:24:49Z

LGTM.

Together with #2111 and #2152, this should cover all the fixes in #1925.

Yes I believe so.

BTW, could u please recommit this pr, since it wasn't built successfully.

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java

tsuna · 2024-06-13T12:37:12Z

Will this fix be backported in the 3.8 train? We just hit this bug on one of our clusters, it's a shame we've had various fixes up for review for over 20 months, I would appreciate you guys pushing this through the finish line so this critical issue can be closed. Thanks!

changruill · 2024-06-14T05:59:42Z

One more thing, the resources (Threads/Processors...) created in startupWithServerState(State.INITIAL) won't be released in shutdown, cause of canShutdown does not contain the condition state == State.INITIAL. This LEAK would occur before ZooKeeperServer.state changes to State.RUNNING (follower read a UPTODATE packet).

tsuna · 2024-06-19T21:06:15Z

This is a pretty small, targeted fix now, is there anything controversial about it or would it be possible to merge it and cut a release soon?
The change needs to be rebased.

… down from Learner

…ldren

…rtial shutdown

jonmv · 2024-08-12T12:48:24Z

I reworked the Learner-socket-close code to be a bit less prone to misuse, while handling a conflict with the first commit, just now.

jonmv · 2024-08-28T09:07:23Z

d2ee4dd fixes the last comment by @changruill.

tsuna · 2024-09-18T11:23:56Z

Hello, would it be possible to move forward with this PR and merge it and cut a release? Are there any outstanding concerns or objections?

anmolnar

lgtm.

anmolnar · 2024-09-18T16:55:41Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java

+            if (sock == null) { // Closing before establishing the connection is a noop
+                return;
+            }


nit: Why have you moved the null check inside the lock?

Close could be called from different threads, and sockBeingClosed ensures memory visibility for sock, as it's set after sock is assigned in connectToLeader.
The only other thread I can see that closes the learner, right now, is the sync processor, which is initialised after sock is assigned, so it works as-was, but I still prefer to be explicit about this.

anmolnar · 2024-09-18T18:30:46Z

@jonmv @tsuna Please close any outstanding PR/Jira ticket that you think is already superseded by something else.
Also please follow the e-mail thread on the dev list which tries to wrap up what's needed for the release. Thanks.

kezhuw

+1 in general.

I left inline one comment about the overriding method.

kezhuw · 2024-09-19T13:26:09Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java

@@ -46,7 +48,7 @@ public void processRequest(Request si) {
                learner.writePacket(qp, false);
            } catch (IOException e) {
                LOG.warn("Closing connection to leader, exception during packet send", e);
-                learner.closeSockSync();
+                learner.closeSocket();


+1 on this.

This is the leftover of ZOOKEEPER-4409. We should respect zookeeper.learner.closeSocketAsync here.

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java

…wn in ZooKeeperServer::shutdown

kezhuw · 2024-09-19T14:06:50Z

Hi @jonmv, I added a test case for this pr in jonmv#1. Could you please take a look ?

ZOOKEEPER-4712: Add test case to assert request processors got shutdown in ZooKeeperServer::shutdown

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java

jonmv · 2024-09-20T08:26:02Z

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/WatchLeakTest.java

@@ -97,7 +97,7 @@ public Boolean answer(InvocationOnMock invocation) throws Throwable {
            }
        });

-        ZKDatabase database = new ZKDatabase(null);
+        ZKDatabase database = new ZKDatabase(mock(FileTxnSnapLog.class));


Otherwise: NPE during fast-forward during shutdown.

jonmv · 2024-09-20T11:33:18Z

A seemingly unrelated unit test failed in the PR jenkins job: it sleeps for a while, while waiting for some state that wasn't reached within timeout; it doesn't depend on shutdown logic, and the test also runs fine locally.

kezhuw · 2024-09-20T12:07:53Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java

+                //    cleared anyway before loading the snapshot
+                try {
+                    // This will fast-forward the database to the last recorded transaction
+                    zkDb.fastForwardDataBase();


I saw its access to metrics after unregisterMetrics. It probably be a good to order it before shutdownComponents, that is assuming it require full functional server components.

The metrics used by the FileTxnSnapLog are not "owned" by the ZooKeeperServer, or its children, and not unregistered here, so that shouldn't be a problem.
On the other hand, as the parent ZooKeeperServer is a dependency of its child classes, I would generally shut down all their components before the parent, and I consider the zkDb to be owned by the parent in this case, which suggests the current order of shutdown is correct. That said, "shutdown is hard" 😂

The metrics used by the FileTxnSnapLog are not "owned" by the ZooKeeperServer, or its children, and not unregistered here, so that shouldn't be a problem.

It is true that running multiple ZooKeeper instances in one JVM probably be hard. It is false that it unregisters metrics during shutdown.

I am ok to keep it unchanged.

kezhuw

LGTM

I left a comments about the order of zkDb.fastForwardDataBase() with shutdownComponents.

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java

…cessors Reviewers: anmolnar, kezhuw, kezhuw, kezhuw Author: jonmv Closes #2154 from jonmv/jonmv/ZOOKEEPER-4541-take-2 (cherry picked from commit bc9afbf) Signed-off-by: Andor Molnar <andor@apache.org>

anmolnar · 2024-09-20T19:10:54Z

Merged. Thanks @jonmv !

anmolnar · 2024-09-20T19:11:55Z

@jonmv What is your Jira id?

jonmv · 2024-09-21T16:04:58Z

@jonmv What is your Jira id?

It's also "jonmv".

anmolnar · 2024-09-23T22:22:46Z

@jonmv What is your Jira id?

It's also "jonmv".

All done. Thank you!

bbarnes52 · 2024-10-28T23:34:59Z

Hey @jonmv thanks for the fix. Can https://issues.apache.org/jira/browse/ZOOKEEPER-4502 (referenced in #1925) be closed now that this is merged?

kezhuw · 2024-10-29T02:11:03Z

I have closed ZOOKEEPER-4502.

@bbarnes52 Thank you for your information!

jonmv mentioned this pull request Apr 4, 2024

ZOOKEEPER-4394: Apply only committed requests in sync with leader before NEWLEADER ACK #2152

Merged

jonmv commented Apr 10, 2024

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java Outdated Show resolved Hide resolved

tsuna mentioned this pull request Jun 13, 2024

ZOOKEEPER-4541 Ephemeral znode owned by closed session visible in 1 of 3 servers #1925

Closed

jonmv added 5 commits August 12, 2024 14:46

Avoid NPE when closing SendAckRequestProcessor after other failure

c4e7e42

Fix shutdown method overrides, so, e.g., SyncRequestProcessor is shut…

8d055cc

… down from Learner

Remove shutdown(boolean) from OZKS, which was identical to that of LZKS

d82213e

Correctly propagate ZKDB.clear during shutdown of not-running ZKS chi…

13001bf

…ldren

Shut down sync processor before parent ZKS; it uses TXN-log during pa…

5d7aa33

…rtial shutdown

jonmv force-pushed the jonmv/ZOOKEEPER-4541-take-2 branch from 8085a35 to 5d7aa33 Compare August 12, 2024 12:47

kezhuw self-requested a review August 12, 2024 13:36

anmolnar approved these changes Sep 18, 2024

View reviewed changes

kezhuw changed the title ~~Jonmv/zookeeper 4541 take 2~~ ZOOKEEPER-4712: Fix partially shutdown of ZooKeeperServer and its processors Sep 19, 2024

kezhuw approved these changes Sep 19, 2024

View reviewed changes

ZOOKEEPER-4712: Add test case to assert request processors got shutdo…

41e6e95

…wn in ZooKeeperServer::shutdown

kezhuw mentioned this pull request Sep 19, 2024

ZOOKEEPER-4712: Add test case to assert request processors got shutdown in ZooKeeperServer::shutdown jonmv/zookeeper#1

Merged

jonmv and others added 2 commits September 20, 2024 08:18

Merge pull request #1 from kezhuw/ZOOKEEPER-4712-test-case

56e959d

ZOOKEEPER-4712: Add test case to assert request processors got shutdown in ZooKeeperServer::shutdown

Refactor shutdown to avoid requiring repeated logic in children

c0682d7

jonmv commented Sep 20, 2024

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java Show resolved Hide resolved

jonmv commented Sep 20, 2024

View reviewed changes

kezhuw reviewed Sep 20, 2024

View reviewed changes

kezhuw approved these changes Sep 20, 2024

View reviewed changes

kezhuw reviewed Sep 20, 2024

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderZooKeeperServer.java Outdated Show resolved Hide resolved

Fix spotbugs failure

3d6f619

kezhuw approved these changes Sep 20, 2024

View reviewed changes

anmolnar merged commit bc9afbf into apache:master Sep 20, 2024
14 checks passed

This was referenced Sep 21, 2024

ZOOKEEPER-4713: ObserverZooKeeperServer.shutdown() is redundant #2029

Closed

ZOOKEEPER-4712: Follower.shutdown() and Observer.shutdown() do not correctly shutdown the syncProcessor, which may lead to data inconsistency #2032

Closed

jonmv deleted the jonmv/ZOOKEEPER-4541-take-2 branch September 23, 2024 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-4712: Fix partially shutdown of ZooKeeperServer and its processors #2154

ZOOKEEPER-4712: Fix partially shutdown of ZooKeeperServer and its processors #2154

jonmv commented Apr 4, 2024

AlphaCanisMajoris commented Apr 10, 2024

tsuna commented Jun 13, 2024

changruill commented Jun 14, 2024

tsuna commented Jun 19, 2024 •

edited

Loading

jonmv commented Aug 12, 2024

jonmv commented Aug 28, 2024

tsuna commented Sep 18, 2024

anmolnar left a comment

anmolnar Sep 18, 2024

jonmv Sep 19, 2024

anmolnar commented Sep 18, 2024

kezhuw left a comment

kezhuw Sep 19, 2024

kezhuw commented Sep 19, 2024

jonmv Sep 20, 2024

jonmv commented Sep 20, 2024

kezhuw Sep 20, 2024

jonmv Sep 20, 2024

kezhuw Sep 20, 2024

kezhuw left a comment

anmolnar commented Sep 20, 2024

anmolnar commented Sep 20, 2024

jonmv commented Sep 21, 2024

anmolnar commented Sep 23, 2024

bbarnes52 commented Oct 28, 2024

kezhuw commented Oct 29, 2024

ZOOKEEPER-4712: Fix partially shutdown of ZooKeeperServer and its processors #2154

ZOOKEEPER-4712: Fix partially shutdown of ZooKeeperServer and its processors #2154

Conversation

jonmv commented Apr 4, 2024

AlphaCanisMajoris commented Apr 10, 2024

tsuna commented Jun 13, 2024

changruill commented Jun 14, 2024

tsuna commented Jun 19, 2024 • edited Loading

jonmv commented Aug 12, 2024

jonmv commented Aug 28, 2024

tsuna commented Sep 18, 2024

anmolnar left a comment

Choose a reason for hiding this comment

anmolnar Sep 18, 2024

Choose a reason for hiding this comment

jonmv Sep 19, 2024

Choose a reason for hiding this comment

anmolnar commented Sep 18, 2024

kezhuw left a comment

Choose a reason for hiding this comment

kezhuw Sep 19, 2024

Choose a reason for hiding this comment

kezhuw commented Sep 19, 2024

jonmv Sep 20, 2024

Choose a reason for hiding this comment

jonmv commented Sep 20, 2024

kezhuw Sep 20, 2024

Choose a reason for hiding this comment

jonmv Sep 20, 2024

Choose a reason for hiding this comment

kezhuw Sep 20, 2024

Choose a reason for hiding this comment

kezhuw left a comment

Choose a reason for hiding this comment

anmolnar commented Sep 20, 2024

anmolnar commented Sep 20, 2024

jonmv commented Sep 21, 2024

anmolnar commented Sep 23, 2024

bbarnes52 commented Oct 28, 2024

kezhuw commented Oct 29, 2024

tsuna commented Jun 19, 2024 •

edited

Loading