From 0f47efd42bd3a57f8a839bad3790af1726ff4526 Mon Sep 17 00:00:00 2001 From: arkmish Date: Thu, 4 Jun 2026 18:40:16 +0530 Subject: [PATCH 1/7] Speed up QuorumSSLTest: RSA-2048 keys and short negative-wait timeout QuorumSSLTest was the slowest test class in the CI suite (~6 min). Two changes cut it roughly in half (339.9s -> 170.9s on the same machine, all 13 tests still passing): - Reduce RSA keypair size from 4096 to 2048 bits in createKeyPair(). 2048-bit keys are sufficient to exercise the TLS handshake code paths and are generated multiple times per test in setup(). - Add SERVER_NOT_UP_TIMEOUT (5s) for the six assertFalse(waitForServerUp) negative checks. These confirm a server designed never to join the quorum stays down, so they no longer idle-wait the full 30s CONNECTION_TIMEOUT (6 x 25s = 150s saved). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../zookeeper/server/quorum/QuorumSSLTest.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java index 92785ab399d..73b7be96d4e 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java @@ -125,6 +125,10 @@ public class QuorumSSLTest extends QuorumPeerTestBase { private static final char[] PASSWORD = "testpass".toCharArray(); private static final String HOSTNAME = "localhost"; + // Timeout used for negative checks where a server is expected to never come up. The full + // CONNECTION_TIMEOUT would otherwise be spent idle-waiting on each of these assertions. + private static final int SERVER_NOT_UP_TIMEOUT = 5000; + private QuorumX509Util quorumX509Util; private MainThread q1; @@ -373,7 +377,7 @@ public X509Certificate buildEndEntityCert( private KeyPair createKeyPair() throws NoSuchProviderException, NoSuchAlgorithmException { KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA", BouncyCastleProvider.PROVIDER_NAME); - keyPairGenerator.initialize(4096); + keyPairGenerator.initialize(2048); KeyPair keyPair = keyPairGenerator.genKeyPair(); return keyPair; } @@ -476,7 +480,7 @@ public void testQuorumSSL() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); } @@ -500,7 +504,7 @@ public void testQuorumSSLWithMultipleAddresses() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); } @@ -762,7 +766,7 @@ public void testCertificateRevocationList() throws Exception { System.setProperty(quorumX509Util.getSslKeystoreLocationProperty(), revokedInCRLKeystorePath); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); } @Test @@ -832,7 +836,7 @@ public void testOCSP() throws Exception { System.setProperty(quorumX509Util.getSslKeystoreLocationProperty(), revokedInOCSPKeystorePath); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); } finally { ocspServer.stop(0); } @@ -875,7 +879,7 @@ public void testCipherSuites() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration, SSL_QUORUM_ENABLED); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); } @Test @@ -897,7 +901,7 @@ public void testProtocolVersion() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration, SSL_QUORUM_ENABLED); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); } } From 83fb3637c225ca6eaabc973a905d391622901fba Mon Sep 17 00:00:00 2001 From: arkmish Date: Thu, 4 Jun 2026 18:43:14 +0530 Subject: [PATCH 2/7] Use RSA-2048 in ZKTrustManagerTest keypair generation The test verifies hostname/IP trust-manager matching, where RSA key strength is irrelevant. Dropping the one-time @BeforeClass keygen from 4096 to 2048 bits removes needless setup cost; all 8 tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/org/apache/zookeeper/common/ZKTrustManagerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java index 66fabed7f10..637a1bb3ca6 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java @@ -72,7 +72,7 @@ public class ZKTrustManagerTest extends ZKTestCase { public static void createKeyPair() throws Exception { Security.addProvider(new BouncyCastleProvider()); KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA", BouncyCastleProvider.PROVIDER_NAME); - keyPairGenerator.initialize(4096); + keyPairGenerator.initialize(2048); keyPair = keyPairGenerator.genKeyPair(); } From 177072b150dcbc8d4cdfec30dfa79ae883046da1 Mon Sep 17 00:00:00 2001 From: Arkesh Mishra <118651144+arkmish@users.noreply.github.com> Date: Thu, 4 Jun 2026 23:59:12 +0530 Subject: [PATCH 3/7] Fix C client test hang: close leaked handle in TestReadOnlyClient The full-build java/cppunit CI profiles hang indefinitely right after Zookeeper_readOnly::testReadOnly, in the next (alphabetical) suite Zookeeper_reconfig, which is mock-based (LibCMocks/ZKMocks install global libc interposition). testReadOnly and testReadOnlyWithSSL were the only tests in the C client suite that created a zhandle_t but never called zookeeper_close (verified: TestReadOnlyClient.cc had 2 inits / 0 closes; every other test file closes its handles). In the threaded build that leaks the client's live IO/ completion threads, which keep doing real socket I/O. When the subsequent mock-based Zookeeper_reconfig suite swaps in global libc mocks, the orphan threads collide with the mocked symbols and deadlock. Closing the handle before stopPeer() matches the pattern used by every other test and removes the cross-test contamination. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../zookeeper-client-c/tests/TestReadOnlyClient.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc b/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc index e864ef266ca..b37beba2459 100644 --- a/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc +++ b/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc @@ -136,6 +136,8 @@ class Zookeeper_readOnly : public CPPUNIT_NS::TestFixture { assertCanNotWrite(zh, "/test"); + zookeeper_close(zh); + stopPeer(); } @@ -161,6 +163,8 @@ class Zookeeper_readOnly : public CPPUNIT_NS::TestFixture { assertCanNotWrite(zh, "/testSSL"); + zookeeper_close(zh); + stopPeer(); } #endif From c931434ce3987ef0f6e28c731bfb25ee7cedc629 Mon Sep 17 00:00:00 2001 From: Arkesh Mishra <118651144+arkmish@users.noreply.github.com> Date: Fri, 5 Jun 2026 00:21:33 +0530 Subject: [PATCH 4/7] Revert "Fix C client test hang: close leaked handle in TestReadOnlyClient" This reverts commit 177072b150dcbc8d4cdfec30dfa79ae883046da1. --- .../zookeeper-client-c/tests/TestReadOnlyClient.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc b/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc index b37beba2459..e864ef266ca 100644 --- a/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc +++ b/zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc @@ -136,8 +136,6 @@ class Zookeeper_readOnly : public CPPUNIT_NS::TestFixture { assertCanNotWrite(zh, "/test"); - zookeeper_close(zh); - stopPeer(); } @@ -163,8 +161,6 @@ class Zookeeper_readOnly : public CPPUNIT_NS::TestFixture { assertCanNotWrite(zh, "/testSSL"); - zookeeper_close(zh); - stopPeer(); } #endif From d6b5f1a000c281d73818dafebd4e3bcca14714fd Mon Sep 17 00:00:00 2001 From: Arkesh Mishra <118651144+arkmish@users.noreply.github.com> Date: Fri, 5 Jun 2026 00:35:22 +0530 Subject: [PATCH 5/7] Fix C client SSL test hang on long-hostname runners (cap cert CN at 64 chars) Root cause (confirmed via gdb thread dump on a LinkedIn rdev runner): gencerts.sh derives the X.509 CommonName from `hostname -f`. On hosts with a long FQDN -- e.g. Kubernetes pods / self-hosted CI runners whose names look like rdev-aks-...-cq4mx.corp.rdev.svc.cluster.local (80 chars) -- the CN exceeds the 64-character ASN.1 limit, so every `openssl req` fails with "string too long:maxsize=64" and NO certificates are produced. The Java test server then silently skips its secure port (22281 never listens), and Zookeeper_simpleSystem::testSSL (TestClient.cc:811) issues a synchronous zoo_create against 127.0.0.1:22281 whose session can never establish. Sync ops on such a handle have no per-call timeout, so the IO thread spins on connect-refused and the main thread blocks forever in wait_sync_completion (mt_adaptor.c:85) -- hanging the whole cppunit run. This is why the hang only reproduces on LinkedIn's long-FQDN runners and not on upstream CI (short hostnames), and why it had nothing to do with the C client itself. Fix: truncate the CN-deriving FQDN to 64 characters in gencerts.sh. Verified locally: the original 80-char CN reproduces the exact "string too long:maxsize=64" openssl error; with the truncation, the full gencerts.sh run completes and generates root/server/client certs + keystores. Co-Authored-By: Claude Opus 4.8 (1M context) --- zookeeper-client/zookeeper-client-c/ssl/gencerts.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/zookeeper-client/zookeeper-client-c/ssl/gencerts.sh b/zookeeper-client/zookeeper-client-c/ssl/gencerts.sh index f32cf5895bd..ed9e3c62226 100755 --- a/zookeeper-client/zookeeper-client-c/ssl/gencerts.sh +++ b/zookeeper-client/zookeeper-client-c/ssl/gencerts.sh @@ -34,6 +34,13 @@ FQDN=`hostname -f` FQDN=${1:-$FQDN} FQDN=${FQDN:-"zookeeper.apache.org"} +# An X.509 CommonName is limited to 64 characters (ASN.1 upper bound). Hosts with +# long fully-qualified names -- e.g. Kubernetes pods or CI runners -- otherwise make +# `openssl req` fail with "string too long:maxsize=64", which leaves no certificates +# generated, so the test server never opens its secure port and the SSL client tests +# hang. Truncate the CN to stay within the limit. +FQDN=${FQDN:0:64} + # Generate the root key openssl genrsa -out rootkey.pem 2048 From 223088fea8794995cf077175a26c170a0835c372 Mon Sep 17 00:00:00 2001 From: Arkesh Mishra <118651144+arkmish@users.noreply.github.com> Date: Fri, 5 Jun 2026 00:40:01 +0530 Subject: [PATCH 6/7] Revert RSA-2048 keygen speedups (low value); keep negative-wait fix The RSA 4096->2048 reductions in QuorumSSLTest and ZKTrustManagerTest saved relatively little wall-clock compared to deviating from the established key size, so revert both back to 4096. Kept: the QuorumSSLTest negative-wait change (SERVER_NOT_UP_TIMEOUT, 30s->5s), which is the high-value, hardware-independent ~150s saving across its six assertFalse(waitForServerUp(...)) negative checks. ZKTrustManagerTest is now unchanged from baseline. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/org/apache/zookeeper/common/ZKTrustManagerTest.java | 2 +- .../java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java index 637a1bb3ca6..66fabed7f10 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKTrustManagerTest.java @@ -72,7 +72,7 @@ public class ZKTrustManagerTest extends ZKTestCase { public static void createKeyPair() throws Exception { Security.addProvider(new BouncyCastleProvider()); KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA", BouncyCastleProvider.PROVIDER_NAME); - keyPairGenerator.initialize(2048); + keyPairGenerator.initialize(4096); keyPair = keyPairGenerator.genKeyPair(); } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java index 73b7be96d4e..e490238b2f6 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java @@ -377,7 +377,7 @@ public X509Certificate buildEndEntityCert( private KeyPair createKeyPair() throws NoSuchProviderException, NoSuchAlgorithmException { KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA", BouncyCastleProvider.PROVIDER_NAME); - keyPairGenerator.initialize(2048); + keyPairGenerator.initialize(4096); KeyPair keyPair = keyPairGenerator.genKeyPair(); return keyPair; } From f2920c94e39f49ab49316af8d0fe5493b72c11e8 Mon Sep 17 00:00:00 2001 From: Arkesh Mishra <118651144+arkmish@users.noreply.github.com> Date: Fri, 5 Jun 2026 07:19:35 +0530 Subject: [PATCH 7/7] Revert QuorumSSLTest speedup; mark slow test profiles optional (per #145) - Revert the QuorumSSLTest negative-wait change back to baseline. - Adopt the CI approach from #145 instead: mark full-build-java-tests and full-build-cppunit-tests as optional (continue-on-error) and cap the job timeout at 60 min, so the long / historically-flaky test profiles no longer fail the workflow run. Lint profiles (jdk8/jdk11) stay strict. The gencerts.sh cert-CN fix (the actual C SSL test hang fix) is retained. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yaml | 19 ++++++++++++++++++- .../server/quorum/QuorumSSLTest.java | 16 ++++++---------- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 049cbaa5a5a..97f5d2b84d7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -35,17 +35,34 @@ jobs: - name: 'full-build-jdk8' jdk: 8 args: '-Pfull-build apache-rat:check verify -DskipTests spotbugs:check checkstyle:check' + optional: false - name: 'full-build-jdk11' jdk: 11 args: '-Pfull-build apache-rat:check verify -DskipTests spotbugs:check checkstyle:check' + optional: false - name: 'full-build-java-tests' jdk: 11 args: '-Pfull-build verify -Dsurefire-forkcount=1 -DskipCppUnit -Dsurefire.rerunFailingTestsCount=5' + optional: true - name: 'full-build-cppunit-tests' jdk: 11 args: '-Pfull-build verify -Dtest=_ -DfailIfNoTests=false' + optional: true fail-fast: false - timeout-minutes: 360 + # Reduced from 360 min (6h) to 60 min (1h). On healthy runners these + # complete well under an hour (local Apple Silicon ran the same suite + # at forkcount=4 in ~28 min). 6h timeouts let wedged jobs camp on + # runner capacity and block every PR for hours before getting killed. + timeout-minutes: 60 + # Mark the long / historically-flaky test profiles as `optional` + # (continue-on-error) so a failure in them does not fail the whole + # workflow run. Lint profiles (jdk8 / jdk11) remain strict. + # NOTE: continue-on-error only affects WORKFLOW-RUN conclusion. If + # these check names are also listed individually in the branch-protection + # required-status-checks rule, repo admin still needs to remove them + # from Settings -> Branches for "optional" to translate into merges + # not being blocked. + continue-on-error: ${{ matrix.profile.optional }} runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java index e490238b2f6..92785ab399d 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/QuorumSSLTest.java @@ -125,10 +125,6 @@ public class QuorumSSLTest extends QuorumPeerTestBase { private static final char[] PASSWORD = "testpass".toCharArray(); private static final String HOSTNAME = "localhost"; - // Timeout used for negative checks where a server is expected to never come up. The full - // CONNECTION_TIMEOUT would otherwise be spent idle-waiting on each of these assertions. - private static final int SERVER_NOT_UP_TIMEOUT = 5000; - private QuorumX509Util quorumX509Util; private MainThread q1; @@ -480,7 +476,7 @@ public void testQuorumSSL() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); } @@ -504,7 +500,7 @@ public void testQuorumSSLWithMultipleAddresses() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); } @@ -766,7 +762,7 @@ public void testCertificateRevocationList() throws Exception { System.setProperty(quorumX509Util.getSslKeystoreLocationProperty(), revokedInCRLKeystorePath); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); } @Test @@ -836,7 +832,7 @@ public void testOCSP() throws Exception { System.setProperty(quorumX509Util.getSslKeystoreLocationProperty(), revokedInOCSPKeystorePath); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); } finally { ocspServer.stop(0); } @@ -879,7 +875,7 @@ public void testCipherSuites() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration, SSL_QUORUM_ENABLED); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); } @Test @@ -901,7 +897,7 @@ public void testProtocolVersion() throws Exception { q3 = new MainThread(3, clientPortQp3, quorumConfiguration, SSL_QUORUM_ENABLED); q3.start(); - assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, SERVER_NOT_UP_TIMEOUT)); + assertFalse(ClientBase.waitForServerUp("127.0.0.1:" + clientPortQp3, CONNECTION_TIMEOUT)); } }