Simplifying `JNLPLauncher` #8762

jglick · 2023-12-07T21:04:46Z

#8639 (comment) cleaning up from jenkinsci/remoting#677. The situation is more complicated than that in #6543 because the deprecated properties will still be used if you use the deprecated -jnlpUrl mode.

Testing done

jenkinsci/configuration-as-code-plugin#2430 continues to pass against this change. Interactively:

Preferred config

Deprecated config

When reconfiguring an agent predating this change.

Launcher page

Proposed changelog entries

Deprecating all configurable options in Launch agent by connecting it to the controller (inbound in JCasC), as these are only useful in conjunction with the deprecated -jnlpUrl mode.

Proposed upgrade guidelines

Reset all inbound launcher options to defaults, and follow new guidance for command-line options for agent.jar.

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options

jglick · 2023-12-07T21:08:52Z

core/src/main/java/hudson/slaves/JNLPLauncher.java

-    // which is already defined and deprecated. Could retroactively let no-arg constructor use default for workDirSettings,
-    // which would be a behavioral change only for callers of the Java constructor (unlikely).


Would only affect

Java callers

which use the no-arg ctor that had been deprecated for years

which use the now-deprecated -jnlpUrl mode

which even care about the location of the work dir

core/src/main/java/hudson/slaves/JNLPLauncher.java

jglick · 2023-12-07T21:11:07Z

core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly

@@ -38,28 +38,6 @@ THE SOFTWARE.
      <j:set var="copy_java_cmd_unix" value="java -jar agent.jar -url ${jenkinsURL} ${it.launcher.getRemotingOptionsUnix(it)}${it.launcher.getWorkDirOptions(it)}" />
      <j:set var="copy_java_cmd_windows" value="java -jar agent.jar -url ${jenkinsURL} ${it.launcher.getRemotingOptionsWindows(it)}${it.launcher.getWorkDirOptions(it)}" />
      <j:if test="${h.hasPermission(it, it.CONNECT)}">
-          <j:choose>
-            <j:when test="${it.ACL.hasPermission(app.ANONYMOUS, it.CONNECT)}">


This clause tried to omit the -secret when Jenkins is run without security, which did not actually work (perhaps as of jenkinsci/remoting#677, or perhaps due to changes in #8639). I do not see any compelling reason not to just always offer -secret.

jglick · 2023-12-07T21:12:27Z

core/src/main/resources/hudson/slaves/JNLPLauncher/main.properties

+powerShell.cli.curl=Note: PowerShell users must use curl.exe instead of curl because curl is a default PowerShell cmdlet alias for Invoke-WebRequest.
+commonOptions=\
+  The most commonly used option is <code>-webSocket</code>. \


I doubt -tunnel and the various workdir-related options are relevant to enough people to be worth enumerating here.

basil

Very nice!

core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly

core/src/main/java/hudson/slaves/JNLPLauncher.java

…LPLauncher

…nsci#8762 (comment)

…n the launch screen jenkinsci#8762 (comment)

jglick · 2023-12-08T17:57:10Z

core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly

+              <p>
+                  ${%instance-identity-missing}
+                  <l:isAdmin><a href="${rootURL}/manage/pluginManager/available">${%install-instance-identity}</a>.</l:isAdmin>
+              </p>


jglick · 2023-12-08T17:57:59Z

core/src/main/java/hudson/model/Slave.java

@@ -245,8 +245,7 @@ public ComputerLauncher getLauncher() {
                LOGGER.log(Level.WARNING, "could not update historical agentCommand setting to CommandLauncher", x);
            }
        }
-        // Default launcher does not use Work Directory


Should only affect loading of ancient node definitions from disk, or I guess someone defining nodes via XML produced from template who for some reason declined to specify a launcher.

jglick · 2023-12-08T17:58:46Z

core/src/main/java/hudson/slaves/JNLPLauncher.java

-                    return FormValidation.error(Messages.JNLPLauncher_WebsocketNotEnabled());
-                }
-                if (Util.fixEmptyAndTrim(tunnel) != null) {
-                    return FormValidation.error(Messages.JNLPLauncher_TunnelingNotSupported());


This and other improper combinations now handled by args4j in the launcher itself.

jglick · 2023-12-08T17:59:44Z

core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly

+                  <l:isAdmin><a href="${rootURL}/manage/pluginManager/available">${%install-instance-identity}</a>.</l:isAdmin>
+              </p>
+          </j:if>
+          <j:if test="${!it.launcher.descriptor.webSocketSupported}">


jglick · 2023-12-08T18:00:29Z

core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly

+              <p>
+                  ${%tcp-port-disabled}
+                  <l:isAdmin><a href="${rootURL}/manage/configureSecurity/">${%configure.link.text}</a>.</l:isAdmin>
+              </p>


Similar to older message, but not marked as an error since it may be perfectly normal in a WebSockets-only installation.

jglick · 2023-12-08T18:01:27Z

core/src/main/resources/hudson/slaves/JNLPLauncher/main_es.properties

@@ -20,7 +20,6 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 # THE SOFTWARE.

-slaveAgentPort.disabled=El puerto TCP para los agentes via JNLP está deshabilitado.


Renaming key and deleting old translations since there is now a significant extra clause about WebSockets.

jglick · 2023-12-08T19:59:01Z

OOME creating the WAR; probably unrelated?

MarkEWaite · 2023-12-08T20:02:19Z

OOME creating the WAR; probably unrelated?

Very likely unrelated. I've seem OOM errors on Java 21 environments for plugin builds in the last 1-2 weeks but have not investigated further to see if there is a root reason for the failures.

NotMyFault · 2023-12-12T07:05:15Z

/label ready-for-merge

This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

akomakom · 2024-02-07T18:31:14Z

FYI, I am seeing consistent NullPointerExceptions when using the docker plugin in JNLP mode, starting with Jenkins 2.437 (The first version with these changes). See jenkinsci/docker-plugin#1047 for details. jnlpLauncher is null in affected versions.

Simplifying JNLPLauncher

0d101a6

jglick commented Dec 7, 2023

View reviewed changes

jglick added 3 commits December 7, 2023 16:15

Amending JNLPLauncherTest.testDefaults to reflect behavioral change

3be5ae0

Checkstyle

601722a

JNLPLauncherRealTest uncompilable after doCheckWebSocket deletion

457557d

basil approved these changes Dec 7, 2023

View reviewed changes

core/src/main/resources/hudson/slaves/JNLPLauncher/main.jelly Outdated Show resolved Hide resolved

core/src/main/java/hudson/slaves/JNLPLauncher.java Show resolved Hide resolved

jglick marked this pull request as draft December 8, 2023 14:55

jglick added 4 commits December 8, 2023 12:19

Merge branch 'master' of https://github.com/jenkinsci/jenkins into JN…

44ebca2

…LPLauncher

Some deprecated constructor calls jenkinsci#8762 (comment)

ed5818a

Missing .launcher caused text to be displayed unconditionally jenki…

8f57907

…nsci#8762 (comment)

Reimplementing the most important parts of form validation as notes i…

c6d02b4

…n the launch screen jenkinsci#8762 (comment)

jglick commented Dec 8, 2023

View reviewed changes

Can restore assertion in JNLPLauncherRealTest

d24f3fb

jglick commented Dec 8, 2023

View reviewed changes

jglick marked this pull request as ready for review December 8, 2023 18:05

basil approved these changes Dec 8, 2023

View reviewed changes

NotMyFault added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted developer Changes which impact plugin developers labels Dec 9, 2023

NotMyFault approved these changes Dec 9, 2023

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 12, 2023

basil merged commit 6a2d94b into jenkinsci:master Dec 13, 2023
17 checks passed

jglick deleted the JNLPLauncher branch December 13, 2023 21:40

jglick mentioned this pull request Dec 19, 2023

Restore JCasC compatibility for JNLPLauncher.tunnel #8793

Merged

6 tasks

jglick mentioned this pull request Jan 30, 2024

Updates and fixing deprecations jenkinsci/mock-slave-plugin#183

Merged

akomakom mentioned this pull request Feb 7, 2024

NPE in DockerComputerJNLPConnector.beforeContainerCreated starting with Jenkins 2.437 jenkinsci/docker-plugin#1047

Closed

jglick mentioned this pull request Apr 14, 2024

[JENKINS-73011] Round-trip JNLPLauncher.tunnel to null not "" #9170

Merged

jglick mentioned this pull request Aug 8, 2024

Removing JNLPLauncher.webSocket notation in nodes.md jenkinsci/support-core-plugin#566

Merged

Vlatombe mentioned this pull request Aug 27, 2024

Add -webSocket option by default when creating an inbound agent #9665

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplifying `JNLPLauncher` #8762

Simplifying `JNLPLauncher` #8762

jglick commented Dec 7, 2023 •

edited

Loading

Maintainer checklist

jglick Dec 7, 2023

jglick Dec 7, 2023

jglick Dec 7, 2023

basil left a comment

jglick Dec 8, 2023

jglick Dec 8, 2023

jglick Dec 8, 2023

jglick Dec 8, 2023

jglick Dec 8, 2023

jglick Dec 8, 2023

jglick commented Dec 8, 2023

MarkEWaite commented Dec 8, 2023 •

edited

Loading

NotMyFault commented Dec 12, 2023

akomakom commented Feb 7, 2024

		// which is already defined and deprecated. Could retroactively let no-arg constructor use default for workDirSettings,
		// which would be a behavioral change only for callers of the Java constructor (unlikely).

Simplifying JNLPLauncher #8762

Simplifying JNLPLauncher #8762

Conversation

jglick commented Dec 7, 2023 • edited Loading

Testing done

Preferred config

Deprecated config

Launcher page

Proposed changelog entries

Proposed upgrade guidelines

Maintainer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Dec 8, 2023

MarkEWaite commented Dec 8, 2023 • edited Loading

NotMyFault commented Dec 12, 2023

akomakom commented Feb 7, 2024

Simplifying `JNLPLauncher` #8762

Simplifying `JNLPLauncher` #8762

jglick commented Dec 7, 2023 •

edited

Loading

MarkEWaite commented Dec 8, 2023 •

edited

Loading