Extract inbound tcp and websocket connection flow to separate classes, unify retrying by Vlatombe · Pull Request #773 · jenkinsci/remoting

Vlatombe · 2024-10-25T09:43:42Z

Extracts Inbound TCP and websocket connection flows to separate classes, and unify the retrying logic, so that both Inbound TCP and Websocket use retries, with exponential retries.

Bumped requirement to Java 17, since we are past End-october, and records makes things pretttier.

Testing done

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

A more ambitious attempt to extract logic from Engine into separate classes

Reorganize code, javadoc

jglick · 2024-11-04T17:51:39Z

incremental diff

jglick · 2024-11-04T17:58:22Z

+        private final RSAPublicKey publicKey;
+        private final Map<String, String> headers;
+
+        public EngineJnlpConnectionStateListener(RSAPublicKey publicKey, Map<String, String> headers) {
+            this.publicKey = publicKey;
+            this.headers = headers;
+        }


Could be a record. (see #749)

Too bad JnlpConnectionStateListener is an abstract class (for no good reason)

jglick · 2024-11-04T18:03:27Z

Seems OK from a quick glance, though the diff is too long to readily see what is actually being edited.

And implement records

Vlatombe · 2024-11-05T09:03:18Z

    private static class NoReconnectException extends RuntimeException {}

-    @Test(timeout = 30_000)
+    @Test(timeout = 10_000)


Shortening timeout since the new retry logic is snappier

Vlatombe · 2024-11-05T09:08:53Z

+        private final RSAPublicKey publicKey;
+        private final Map<String, String> headers;
+
+        public EngineJnlpConnectionStateListener(RSAPublicKey publicKey, Map<String, String> headers) {
+            this.publicKey = publicKey;
+            this.headers = headers;
+        }


Too bad JnlpConnectionStateListener is an abstract class (for no good reason)

Vlatombe · 2024-11-05T09:09:20Z


    public Engine(
            EngineListener listener,
-            List<URL> hudsonUrls,


well... long overdue.

Vlatombe · 2024-11-05T09:10:09Z

-                SSLContext context;
-                // prepare our SSLContext
-                try {
-                    context = SSLContext.getInstance("TLS");
-                } catch (NoSuchAlgorithmException e) {
-                    throw new IllegalStateException("Java runtime specification requires support for TLS algorithm", e);
-                }
-                char[] password = "password".toCharArray();
-                KeyStore store;
-                try {
-                    store = KeyStore.getInstance(KeyStore.getDefaultType());
-                } catch (KeyStoreException e) {
-                    throw new IllegalStateException("Java runtime specification requires support for JKS key store", e);
-                }
-                try {
-                    store.load(null, password);
-                } catch (NoSuchAlgorithmException e) {
-                    throw new IllegalStateException("Java runtime specification requires support for JKS key store", e);
-                } catch (CertificateException e) {
-                    throw new IllegalStateException("Empty keystore", e);
-                }
-                KeyManagerFactory kmf;
-                try {
-                    kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
-                } catch (NoSuchAlgorithmException e) {
-                    throw new IllegalStateException(
-                            "Java runtime specification requires support for default key manager", e);
-                }
-                try {
-                    kmf.init(store, password);
-                } catch (KeyStoreException | NoSuchAlgorithmException | UnrecoverableKeyException e) {
-                    throw new IllegalStateException(e);
-                }
-                try {
-                    context.init(kmf.getKeyManagers(), new TrustManager[] {agentTrustManager}, null);
-                } catch (KeyManagementException e) {
-                    events.error(e);
-                    return;
-                }


Moved to SSLUtils

Vlatombe · 2024-11-05T09:10:33Z

    @SuppressFBWarnings(
-            value = {"REC_CATCH_EXCEPTION"},
-            justification = "checked exceptions were a mistake to begin with; connecting to Jenkins from agent")
-    private void runWebSocket() {


Moved to WebSocketConnector

Vlatombe · 2024-11-05T09:11:11Z

-    private static class ExponentialRetry {
-        final int factor;
-        final Instant beginning;
-        final Duration delay;
-        final Duration timeout;
-        final Duration incrementDelay;
-        final Duration maxDelay;
-
-        ExponentialRetry(Duration timeout) {
-            this(Duration.ofSeconds(0), timeout, 2, Duration.ofSeconds(1), Duration.ofSeconds(10));
-        }
-
-        ExponentialRetry(
-                Duration initialDelay, Duration timeout, int factor, Duration incrementDelay, Duration maxDelay) {
-            this.beginning = Instant.now();
-            this.delay = initialDelay;
-            this.timeout = timeout;
-            this.factor = factor;
-            this.incrementDelay = incrementDelay;
-            this.maxDelay = maxDelay;
-        }
-
-        ExponentialRetry(ExponentialRetry previous) {
-            beginning = previous.beginning;
-            factor = previous.factor;
-            timeout = previous.timeout;
-            incrementDelay = previous.incrementDelay;
-            maxDelay = previous.maxDelay;
-            delay = min(maxDelay, previous.delay.multipliedBy(previous.factor).plus(incrementDelay));
-        }
-
-        private static Duration min(Duration a, Duration b) {
-            return a.compareTo(b) < 0 ? a : b;
-        }
-
-        boolean timeoutExceeded() {
-            return Util.shouldBailOut(beginning, timeout);
-        }
-
-        ExponentialRetry next(EngineListenerSplitter events) throws InterruptedException {
-            var next = new ExponentialRetry(this);
-            if (next.timeoutExceeded()) {
-                events.status("Bailing out after " + DurationFormatter.format(next.timeout));
-                return null;
-            } else {
-                events.status("Waiting " + DurationFormatter.format(next.delay) + " before retry");
-                Thread.sleep(next.delay.toMillis());
-            }
-            return next;
-        }
-    }


Moved to RetryUtils

Vlatombe · 2024-11-05T09:11:53Z

-        }
-    }
-
-    private void innerRun(IOHub hub, SSLContext context, ExecutorService service) {


Moved to InboundTCPConnector

Vlatombe · 2024-11-05T09:12:14Z

    private static final Logger LOGGER = Logger.getLogger(Engine.class.getName());

-    @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "File path is loaded from system properties.")
-    static KeyStore getCacertsKeyStore()


Moved to SSLUtils

Vlatombe · 2024-11-05T09:12:30Z

        return this.protocolName;
    }
-
-    private class EngineJnlpConnectionStateListener extends JnlpConnectionStateListener {


Moved to InboundTCPConnector

Vlatombe · 2024-11-05T09:21:20Z

-            }
+            value = {"HARD_CODE_PASSWORD", "REC_CATCH_EXCEPTION"},
+            justification = "Password doesn't need to be protected.; We need to catch all exceptions")
+    public void run() {


This method contains the key structural changes.

Vlatombe · 2024-11-05T09:57:34Z

+        var clientProtocols = new JnlpProtocolHandlerFactory(data.executor())
+                .withIOHub(hub)
+                .withSSLContext(context)
+                .withPreferNonBlockingIO(false) // we only have one connection, prefer blocking I/O
+                .handlers();
+        var negotiatedProtocols = clientProtocols.stream()
+                .filter(JnlpProtocolHandler::isEnabled)
+                .filter(p -> endpoint.isProtocolSupported(p.getName()))
+                .collect(Collectors.toSet());
+        var serverProtocols = endpoint.getProtocols() == null ? "?" : String.join(",", endpoint.getProtocols());
+        LOGGER.info(buildDebugProtocolsMessage(serverProtocols, clientProtocols, negotiatedProtocols));


Gives clearer output to users on which protocols are supported on either side of the connection.

jglick

OK AFAICT

basil · 2024-11-06T18:11:49Z

See this document for a discussion of various retry strategies. Exponential backoff does appear to be a strict improvement over the status quo, as it balances retries against latency when the controller is unavailable for an unknown duration; however, in contrast with jitter-based solutions, it does not resolve contention for the controller. As in #676, testing of various retry strategies in real-world scenarios is needed to evaluate the effectiveness of those strategies. I would be fine with this PR if it was a simple refactoring that did not introduce exponential backoff. I would also be fine with introducing exponential backoff it was confirmed to have a positive impact in a functional test scenario.

Extract EndpointConnectors with 2 implementions

fb1338a

A more ambitious attempt to extract logic from Engine into separate classes

Vlatombe requested a review from jglick October 25, 2024 09:43

Vlatombe added 3 commits October 25, 2024 14:20

Spotless

a255435

Simplify

9f25128

Extract JNLPEndpointResolver out of InboundTCPConnector

b4c9850

Reorganize code, javadoc

jglick reviewed Nov 4, 2024

View reviewed changes

Comment thread src/main/java/hudson/remoting/Engine.java Outdated

jglick reviewed Nov 4, 2024

View reviewed changes

Comment thread src/main/java/org/jenkinsci/remoting/engine/AbstractEndpointConnector.java Outdated

jglick reviewed Nov 4, 2024

View reviewed changes

Comment thread src/main/java/org/jenkinsci/remoting/engine/EndpointConnector.java Outdated

jglick reviewed Nov 4, 2024

View reviewed changes

Comment thread src/main/java/org/jenkinsci/remoting/engine/RetryUtils.java Outdated

Vlatombe added 3 commits November 5, 2024 09:26

Merge branch 'master' into endpoint-connector

02f8a57

Move to JDK17

6a1ce9d

And implement records

Add restrictions and @SInCE as needed.

70f312a

Vlatombe commented Nov 5, 2024

View reviewed changes

Vlatombe added 4 commits November 5, 2024 10:23

Javadoc

1ecdd9e

Simplify noReconnectAfter handling

9ba77f7

Attempt to give slighly more memory to tests

7bc0d04

Spotbugs

55ef806

Vlatombe marked this pull request as ready for review November 5, 2024 09:56

Vlatombe commented Nov 5, 2024

View reviewed changes

Vlatombe added 4 commits November 5, 2024 11:02

Fix delay

887773e

Eat stacktrace here as it is too noisy

21150c6

Extract next delay logic to a method

9b1ab2a

Simplify loop

0d7cb97

jglick approved these changes Nov 5, 2024

View reviewed changes

timja approved these changes Nov 5, 2024

View reviewed changes

Comment thread src/main/java/org/jenkinsci/remoting/engine/RetryUtils.java Outdated

RetryUtils is not public

23276a1

timja reviewed Nov 6, 2024

View reviewed changes

Comment thread pom.xml

Update pom.xml

c3a646c

timja reviewed Nov 6, 2024

View reviewed changes

Comment thread pom.xml Outdated

timja added 2 commits November 6, 2024 08:58

Update pom.xml

325f332

Update jvm.config

04af69d

timja added the enhancement For changelog: An enhancement providing new capability. label Nov 6, 2024

Vlatombe mentioned this pull request Oct 3, 2025

No need to retry inside connectTcp #827

Merged

Uh oh!

Conversation

Vlatombe commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing done

Submitter checklist

Uh oh!

jglick commented Nov 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jglick Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jglick commented Nov 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

basil commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Vlatombe commented Oct 25, 2024 •

edited

Loading

jglick Nov 4, 2024 •

edited

Loading

basil commented Nov 6, 2024 •

edited

Loading