[JENKINS-26580] Activate JNLP3 support #2010

kohsuke · 2016-02-02T10:09:15Z

Follow up to jenkinsci/remoting#69 to activate JNLP3 support on the server side

This way, the inner-working of the protocol can be kept private within the remoting module, which increases a modularity.

Given that there's only one caller, I think handling AbstractMethodError is better than doing the compatibility method ping pong

Conflicts: core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java pom.xml

kohsuke · 2016-02-02T10:09:48Z

cc @akshayabd @oleg-nenashev @jglick

Merging this requires a corresponding remoting release

oleg-nenashev · 2016-02-02T10:15:01Z

core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java

@@ -55,7 +57,16 @@
     * @throws Exception
     *      Any exception thrown from this method will fatally terminate the connection.
     */
-    public abstract boolean handle(String name, JnlpSlaveHandshake handshake) throws IOException, InterruptedException;
+    public abstract boolean handle(String name, JnlpServerHandshake handshake) throws IOException, InterruptedException;


It still breaks the binary compatibility, because external classes won't implement the new abstract method. Maybe NIT in the case of remoting

BTW the invocation below catches the AbstractMethodError

oleg-nenashev · 2016-02-02T10:24:17Z

LGTM in general.

amuniz · 2016-02-02T10:29:56Z

core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java

+     * @deprecated
+     *      Use {@link #handle(String, JnlpServerHandshake)}
+     */
+    public boolean handle(String name, JnlpSlaveHandshake handshake) throws IOException, InterruptedException {


Missing @Deprecated - given the javadoc.

One or the other is sufficient.

kohsuke · 2016-02-02T11:21:32Z

My plane is about to leave so I can't check this now but does this code correctly call ChannelConfigurator

jglick · 2016-02-07T02:31:08Z

core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol2.java

+                    if (recv.handle(nodeName,this))
+                        return;
+                } catch (AbstractMethodError e) {
+                    if (recv.handle(nodeName,new JnlpSlaveHandshake(this)))


Looks alarming. Why not just retain the original method signature, deprecate it, and delegate?

This was much shorter than the ping-pong and loop detection, plus it has the benefit of abstract method clearly marked as such.

jglick · 2016-02-07T02:33:59Z

does this code correctly call ChannelConfigurator

The fix of SECURITY-206 includes an assertion in JnlpAccessWithSecuredHudsonTest.testServiceUsingDirectSecret. If the agent and master automatically agree to use the v3 protocol, I suppose the existing test should cover this.

jglick · 2016-02-07T02:35:48Z

Could not resolve dependencies for project org.jenkins-ci.main:cli:jar:1.647-SNAPSHOT: Could not find artifact org.jenkins-ci.main:remoting:jar:2.54-SNAPSHOT in cloudbees-private-snapshot-repository

You need to deploy a snapshot of the library to have this PR be tested.

kohsuke · 2016-02-20T01:55:33Z

Wow, Jesse predicted the exact test case that fails!!

akshayabd · 2016-02-22T21:50:06Z

core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol3.java

+ * @author Akshay Dayal
+ * @since 1.XXX
+ */
+@Extension


Initial manual testing looks good - the JNLP node connects with JNLP3 protocol and I can build a project on the node.

I think more stress testing needs to be done before JNLP3 is made the default protocol. In this initial release JNLP3 should only be available if the user explicitly asks for it. There are 3 approaches:

Master-side control
1. Making master support JNLP3 through a flag
2. Making master support JNLP3 through UI option
Node-side control
3. Making node try JNLP3 first using flag

Option 1 can be done in "easy" ways I suppose, this class can check for a boolean system property, if its not set fail and let JNLP2 take over.
Option 2 is a lot of work and I would not favor it.
Option 3 is easy, but requires some extra work for users starting up nodes with JNLP3.

I'm ok with either option 1 or 3, prefer 1 slightly since I like controlling the behavior from the master side, not the node.

OK, I'll ease this in by making this feature optional in the beginning for a while.

I'm going to spend some time dusting out the stress-test suites that we used before and run them for JNLP3. Will probably have to modify them a bit to have significant "work" being done for each build instead of just "sleep 5". Will let you know when I'm done.

JNLP3 is activated now for 10% of users. Let's keep it like this for a while and if no major issue occurs we should expose it to everyone.

Conflicts: pom.xml

kohsuke · 2016-03-04T22:14:11Z

I discovered that the protocol doesn't gracefully handle a server that doesn't understand the protocol. Needs further tweak in remoting.

See jenkinsci/remoting#78

kohsuke · 2016-03-04T22:54:47Z

Can somebody help me understand why the PR validation build doesn't pick up http://repo.jenkins-ci.org/snapshots/org/jenkins-ci/main/remoting/2.56-SNAPSHOT/ ?

kohsuke · 2016-03-04T23:57:11Z

FWIW locally it passes all the tests.

aheritier · 2016-03-07T10:49:51Z

@kohsuke https://repository-jenkins.forge.cloudbees.com/snapshot/org/jenkins-ci/main/remoting/ there is no 2.56-SNAPSHOT version here ....
Did you manually deployed it ?
I suppose you did and it was removed because Maven tries to get a version 2.56-20160304.223037-1 thus it must have this version in its local repository cache. The problem is that it doesn't use it because it isn't available in the remote repository. It is the crappy feature added in Maven 3.x and which always reports a stupid error message that doesn't explain the error cc @stephenc
Building with -U option (or repositories options in settings to always check snapshots) is safer/cleaner to enforce maven to try to get a new SNAPSHOt if available. It won't solve the issue that the SNAPSHOT was deleted in the remote. The problem can be bypassed (but it's crappy) and let maven use its local cached SNAPSHOT with the option

-llr,--legacy-local-repository         Use Maven 2 Legacy Local
                                        Repository behaviour, ie no use of
                                        _remote.repositories. Can also be
                                        activated by using
                                        -Dmaven.legacyLocalRepo=true

kohsuke · 2016-03-07T17:03:50Z

@aheritier see the link I provided above, which is the official snapshot repo for the Jenkins project. Sounds like the issue is that we are not configuring our POMs to refer to that snapshot repository, though it makes me wonder how others have been doing this.

aheritier · 2016-03-07T18:16:07Z

@kohsuke https://repository-jenkins.forge.cloudbees.com/snapshot/org/jenkins-ci/main/remoting/ is where the build is looking for snapshots instead of http://repo.jenkins-ci.org/snapshots/org/jenkins-ci/main/remoting

I agree that the snapshot repo http://repo.jenkins-ci.org/snapshots/org/jenkins-ci/main/remoting should be defined in the POM or in the maven settings

For now it's not defined in any POM
https://github.com/jenkinsci/pom/blob/master/pom.xml#L77
https://github.com/jenkinsci/jenkins/blob/master/pom.xml#L111

Thus I imagine that people using SNAPSHOTs are defining the repo in the maven settings used on CI
There is a settings.xml config defined in the Config File Provider plugin called JenkinsSettings activating in a jenkins profile snapshots from http://repo.jenkins-ci.org/public/ but this one isn't used by the job. I don't know what are the rules and how is managed this instance. maybe @abayer @rtyler or @daniel-beck know ...

kohsuke · 2016-03-08T23:26:58Z

The test failure is a fluke

aheritier · 2016-03-09T08:30:59Z

@kohsuke Great !!
I think that we should use for all jobs on https://jenkins.ci.cloudbees.com the same set of (maven) settings and to activate SNAPSHOTs for everyone (but I have no clue where to request it - INFRA ? - and who can/should take care of this)

[FIXED JENKINS-26580] Activate JNLP3 support

akshayabd · 2016-03-09T19:42:55Z

Great to see this merged in - thanks @kohsuke! I always recommend caution, I still haven't done enough scale testing to see what sort of [CPU] performance requirements the Ciphers have. Is there a smaller Jenkins cluster you can test this on before activating it on the main https://jenkins.ci.cloudbees.com?

stephenc · 2016-03-09T19:55:17Z

Also, as I have pointed out, this is a non-NIO implementation and requires one reader thread per slave... I am working on a v4 protocol that uses a new SSLEngine aware NIOHub and hope to have that soon.

The consideration of @kohsuke and I is that allowing some encryption of the channel at the cost of threads is more important than waiting while I battle with SSLEngine to deliver a v4 protocol

Sent from my iPad

On 9 Mar 2016, at 19:43, akshayabd notifications@github.com wrote:

Great to see this merged in - thanks @kohsuke! I always recommend caution, I still haven't done enough scale testing to see what sort of [CPU] performance requirements the Ciphers have. Is there a smaller Jenkins cluster you can test this on before activating it on the main https://jenkins.ci.cloudbees.com?

—
Reply to this email directly or view it on GitHub.

akshayabd · 2016-03-09T20:18:32Z

@stephenc Yes good catch, I had not considered the NIOHub issue. Performance-wise it would probably make a significant difference.

[FIXED JENKINS-26580][FIXED JENKINS-28289] Activate JNLP3 support (cherry picked from commit 6d3e054)

This reverts commit af1a53d.

akshayabd and others added 5 commits April 17, 2015 08:11

[JENKINS-26580] Initial implementation of JNLP3-connect protocol

9bb35a7

[JENKINS-26580] Updated implementation of Jnlp3 protocol

e9f5caa

Moved some of the server-side logic to remoting

4915c31

This way, the inner-working of the protocol can be kept private within the remoting module, which increases a modularity.

JnlpAgentReceiver needs to be adjusted for the new type

a1d3634

Given that there's only one caller, I think handling AbstractMethodError is better than doing the compatibility method ping pong

Merge remote-tracking branch 'origin/master' into jnlp3-protocol

da07e54

Conflicts: core/src/main/java/jenkins/slaves/JnlpAgentReceiver.java pom.xml

oleg-nenashev reviewed Feb 2, 2016
View reviewed changes

amuniz reviewed Feb 2, 2016
View reviewed changes

kohsuke added 3 commits February 2, 2016 11:32

Fixing a compilation problem

3e17176

Java7 code transformation

7b6a6c6

Log cannot be opened twice so it needs to be kept around.

19ad0e5

Terminology fix

1ba9c30

jglick reviewed Feb 7, 2016
View reviewed changes

2.54 contains JNLP3 change

451186d

jglick changed the title ~~Active JNLP3 support~~ [JENKINS-26580] Activate JNLP3 support Feb 19, 2016

kohsuke added 2 commits February 22, 2016 11:10

Need to call ChannelConfigurator

a6cd33f

When this gets merged let's record the version number

e606506

akshayabd reviewed Feb 22, 2016
View reviewed changes

kohsuke added 2 commits March 4, 2016 13:45

A/B test this feature in production

b1e3f6f

JNLP3 is activated now for 10% of users. Let's keep it like this for a while and if no major issue occurs we should expose it to everyone.

Merge remote-tracking branch 'origin/master' into jnlp3

0ca7814

Conflicts: pom.xml

Pick up a new version of remoting

d0d3970

See jenkinsci/remoting#78

This was referenced Mar 4, 2016

Need a line with known text to find failures jenkinsci/remoting#78

Merged

[FIXED JENKINS-28086][FIXED JENKINS-28289] Update remoting version to 2.55 #2074

Closed

Pick up a released version

04651f2

kohsuke closed this Mar 8, 2016

kohsuke reopened this Mar 8, 2016

kohsuke added a commit that referenced this pull request Mar 9, 2016

Merge pull request #2010 from jenkinsci/jnlp3

6d3e054

[FIXED JENKINS-26580] Activate JNLP3 support

kohsuke merged commit 6d3e054 into master Mar 9, 2016

kohsuke deleted the jnlp3 branch March 9, 2016 16:10

aheritier mentioned this pull request Mar 21, 2016

[INFRA-588] Upgrade + Remove inherited values #2143

Merged

olivergondza pushed a commit that referenced this pull request Mar 30, 2016

Merge pull request #2010 from jenkinsci/jnlp3

af1a53d

[FIXED JENKINS-26580][FIXED JENKINS-28289] Activate JNLP3 support (cherry picked from commit 6d3e054)

olivergondza added a commit that referenced this pull request Mar 31, 2016

Revert "Merge pull request #2010 from jenkinsci/jnlp3"

343e65f

This reverts commit af1a53d.

jglick mentioned this pull request Oct 22, 2024

Removing configurability of Jenkins.agentProtocols #9903

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-26580] Activate JNLP3 support #2010

[JENKINS-26580] Activate JNLP3 support #2010

kohsuke commented Feb 2, 2016

kohsuke commented Feb 2, 2016

oleg-nenashev Feb 2, 2016

oleg-nenashev Feb 2, 2016

oleg-nenashev commented Feb 2, 2016

amuniz Feb 2, 2016

kohsuke Feb 22, 2016

kohsuke commented Feb 2, 2016

jglick Feb 7, 2016

kohsuke Feb 19, 2016

jglick commented Feb 7, 2016

jglick commented Feb 7, 2016

kohsuke commented Feb 20, 2016

akshayabd Feb 22, 2016

kohsuke Feb 22, 2016

akshayabd Feb 22, 2016

kohsuke commented Mar 4, 2016

kohsuke commented Mar 4, 2016

kohsuke commented Mar 4, 2016

aheritier commented Mar 7, 2016

kohsuke commented Mar 7, 2016

aheritier commented Mar 7, 2016

kohsuke commented Mar 8, 2016

aheritier commented Mar 9, 2016

akshayabd commented Mar 9, 2016

stephenc commented Mar 9, 2016

akshayabd commented Mar 9, 2016

[JENKINS-26580] Activate JNLP3 support #2010

[JENKINS-26580] Activate JNLP3 support #2010

Conversation

kohsuke commented Feb 2, 2016

kohsuke commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohsuke commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Feb 7, 2016

jglick commented Feb 7, 2016

kohsuke commented Feb 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohsuke commented Mar 4, 2016

kohsuke commented Mar 4, 2016

kohsuke commented Mar 4, 2016

aheritier commented Mar 7, 2016

kohsuke commented Mar 7, 2016

aheritier commented Mar 7, 2016

kohsuke commented Mar 8, 2016

aheritier commented Mar 9, 2016

akshayabd commented Mar 9, 2016

stephenc commented Mar 9, 2016

akshayabd commented Mar 9, 2016