Add `-noReconnectAfter` option #738

Vlatombe · 2024-05-07T11:51:35Z

This option allows to abort retrying after a given timeout expressed in simple form (e.g. 1d, 2h, 10m, 60s, ...)

~~Downstream work to include this option in the docker image for agents: jenkinsci/docker-agent#802~~

Testing done

Interactive testing

Inbound TCP
- On initial connection ✅
- On disconnect ✅
Websocket
- Initial connection ❌: -noReconnect not implemented
- On disconnect ✅

Submitter checklist

Give feedback

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
Options

This option allows to abort retrying after a given timeout in seconds. Interactive testing * Inbound TCP ** On initial connection ✅ ** On disconnect ✅ * Websocket ** Initial connection ❌: `-noReconnect` not implemented ** On disconnect ✅

jglick · 2024-05-07T12:08:55Z

[on WebSocket] -noReconnect not implemented

Right, this is a known bug that could be addressed separately.

jglick · 2024-05-07T12:10:51Z

Does this need to be an option, or can we just hard-code a timeout of (say) one hour? I guess that could break some use case involving a static agent connected to a controller which is down for maintenance over a long weekend?

Vlatombe · 2024-05-07T12:19:18Z

I could give the option a sensible default value, but I think one hour may be too short, considering some maintenance followed by a startup sequence on slow hardware for the controller. Maybe one day?

jglick · 2024-05-07T12:25:16Z

Not sure. I guess making the timeout opt-in makes sense in that it can be set by all cloud plugins, as a complement for example to jenkinsci/kubernetes-plugin#1543; and anyone using static agents can decide for themselves if it is helpful.

jglick

Looks good so far.

src/main/java/hudson/remoting/DurationSecondsOptionHandler.java

src/main/java/hudson/remoting/Engine.java

jglick · 2024-05-07T12:29:06Z

src/main/java/hudson/remoting/Engine.java

@@ -740,9 +750,14 @@ public void closeRead() throws IOException {
                if (noReconnect) {
                    return;
                }
+                firstAttempt = Instant.now();


(WS reconnect)

jglick · 2024-05-07T12:29:16Z

src/main/java/hudson/remoting/Engine.java

@@ -795,14 +810,18 @@ private void innerRun(IOHub hub, SSLContext context, ExecutorService service) {

        try {
            boolean first = true;
+            firstAttempt = Instant.now();


TCP initial connection?

jglick · 2024-05-07T12:29:50Z

src/main/java/hudson/remoting/Engine.java

@@ -915,7 +934,7 @@ private void innerRun(IOHub hub, SSLContext context, ExecutorService service) {
                }
                if(noReconnect)
                    return; // exit
-
+                firstAttempt = Instant.now();


TCP reconnect?

src/main/java/hudson/remoting/Launcher.java

jglick · 2024-05-07T12:31:12Z

src/main/java/hudson/remoting/Launcher.java

@@ -682,6 +690,7 @@ private List<String> parseJnlpArguments() throws ParserConfigurationException, S
                throw new IOException("-jnlpCredentials and -secret are mutually exclusive");
            }
        }
+        Instant firstAttempt = Instant.now();


Fine though this mode is deprecated so we do not really need to add enhancements to it.

jglick · 2024-05-07T12:31:41Z

src/main/java/org/jenkinsci/remoting/engine/JnlpAgentEndpointResolver.java

@@ -401,8 +407,13 @@ public void waitForReady() throws InterruptedException {
        String oldName = t.getName();
        try {
            int retries = 0;
+            Instant firstAttempt = Instant.now();


(TCP discovery via HTTP)

jglick · 2024-05-07T12:35:43Z

(complements #676 I think)

src/main/java/org/jenkinsci/remoting/util/DurationStyle.java

src/main/java/org/jenkinsci/remoting/DurationOptionHandler.java

jglick · 2024-05-07T16:40:33Z

src/main/java/org/jenkinsci/remoting/util/DurationFormatter.java

+public final class DurationFormatter {
+    private DurationFormatter(){}
+
+    public static String format(Duration d) {


BTW there is also https://github.com/jenkinsci/jenkins/blob/f8e381fb44d59432d654bc64eb314af7d23f79e9/core/src/main/java/hudson/Util.java#L736-L745

src/main/java/org/jenkinsci/remoting/util/DurationStyle.java

jglick · 2024-05-13T11:25:00Z

src/main/java/org/jenkinsci/remoting/util/DurationStyle.java

+
+        public static Unit fromSuffix(String suffix) {
+            for (Unit candidate : values()) {
+                if (candidate.suffix.equals(suffix.toLowerCase(Locale.ROOT))) {


https://find-sec-bugs.github.io/bugs.htm#IMPROPER_UNICODE seems to be triggering improperly, since use of Locale.ROOT should be safe AFAIK.

jglick · 2024-05-15T12:46:32Z

Is there a corresponding PR to kubernetes-plugin to activate this mode? I guess there is no simple way to consume jenkinsci/docker-agent#809 prior to merge & release?

Vlatombe · 2024-05-15T13:09:31Z

Unfortunately no, there isn't.

Add -noReconnectAfter option

ef46eba

This option allows to abort retrying after a given timeout in seconds. Interactive testing * Inbound TCP ** On initial connection ✅ ** On disconnect ✅ * Websocket ** Initial connection ❌: `-noReconnect` not implemented ** On disconnect ✅

Vlatombe requested a review from jglick May 7, 2024 11:51

jglick approved these changes May 7, 2024

View reviewed changes

Vlatombe added 6 commits May 7, 2024 14:46

Use forbids attribute rather than checking manually.

f30f0c3

Duration parsing/formatting using 1s / 1m / 1d format

893b2b1

Handle date parsing and formatting

6cbfbcd

Add license header

ad37bf0

Another one

4146898

Add some tests

f405874

Vlatombe mentioned this pull request May 7, 2024

Add -noReconnectAfter option support jenkinsci/docker-agent#802

Closed

1 task

Vlatombe added 2 commits May 7, 2024 16:36

Fix tests

bae4f1d

Fix Spotbugs issue

5a48bbe

Vlatombe marked this pull request as ready for review May 7, 2024 16:18

jglick reviewed May 7, 2024

View reviewed changes

src/main/java/org/jenkinsci/remoting/util/DurationStyle.java Outdated Show resolved Hide resolved

jglick approved these changes May 7, 2024

View reviewed changes

Fix reviews

f61308d

jglick approved these changes May 13, 2024

View reviewed changes

Mute the spotbugs issue

a3004af

Vlatombe requested a review from basil May 13, 2024 13:30

Merge branch 'master' into noReconnectAfter

623bc8c

Vlatombe enabled auto-merge May 15, 2024 06:50

Vlatombe added the enhancement For changelog: An enhancement providing new capability. label May 15, 2024

Vlatombe merged commit 4280e21 into jenkinsci:master May 15, 2024
13 checks passed

Vlatombe deleted the noReconnectAfter branch May 15, 2024 07:37

Vlatombe mentioned this pull request May 21, 2024

Add --noReconnectAfter 1d option when available in the agent image. jenkinsci/kubernetes-plugin#1553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `-noReconnectAfter` option #738

Add `-noReconnectAfter` option #738

Vlatombe commented May 7, 2024 •

edited

Loading

Submitter checklist

jglick commented May 7, 2024

jglick commented May 7, 2024

Vlatombe commented May 7, 2024

jglick commented May 7, 2024

jglick left a comment

jglick May 7, 2024

jglick May 7, 2024

jglick May 7, 2024

jglick May 7, 2024

jglick May 7, 2024

jglick commented May 7, 2024

jglick May 7, 2024

jglick May 13, 2024

jglick commented May 15, 2024

Vlatombe commented May 15, 2024

Add -noReconnectAfter option #738

Add -noReconnectAfter option #738

Conversation

Vlatombe commented May 7, 2024 • edited Loading

Testing done

Submitter checklist

jglick commented May 7, 2024

jglick commented May 7, 2024

Vlatombe commented May 7, 2024

jglick commented May 7, 2024

jglick left a comment

Choose a reason for hiding this comment

jglick May 7, 2024

Choose a reason for hiding this comment

jglick May 7, 2024

Choose a reason for hiding this comment

jglick May 7, 2024

Choose a reason for hiding this comment

jglick May 7, 2024

Choose a reason for hiding this comment

jglick May 7, 2024

Choose a reason for hiding this comment

jglick commented May 7, 2024

jglick May 7, 2024

Choose a reason for hiding this comment

jglick May 13, 2024

Choose a reason for hiding this comment

jglick commented May 15, 2024

Vlatombe commented May 15, 2024

Add `-noReconnectAfter` option #738

Add `-noReconnectAfter` option #738

Vlatombe commented May 7, 2024 •

edited

Loading