[FIXES JENKINS-28245] Allow finer-grained tuning of ChannelPinger. #1687

deadmoose · 2015-05-05T20:43:18Z

Allows customization in seconds, not minutes
Allows customization of the ping timeout (before, you could set a
custom interval, but the timeout would always be PingThread's 4
minute default)

This also drops the serialVersionUID from ChannelPinger.SetUpRemotePing;
without one provided, the JVM will generate one on demand which is
sufficient for the purposes here since these are never persisted and
master & slave run the same compiled code. (And it demonstrably works
since countless other MasterToSlaveCallables fail to specify their own
custom IDs)

KostyaSha · 2015-05-10T23:17:42Z

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

@@ -136,6 +150,7 @@ public void onClosed(Channel channel, IOException cause) {
        });

        t.start();
-        LOGGER.fine("Ping thread started for " + channel + " with a " + interval + " minute interval");
+        LOGGER.fine("Ping thread started for " + channel + " with a " +
+            intervalSeconds + " second interval and a " + timeoutSeconds + " second timeout.");


Maybe also change to not have slow logging?

What do you mean by slow logging? Building up the string like that (including in case of FINE not actually being logged)?

Logger.log(Level.FINE, "This is a {0} {1} {2}", new Object[] {"paramaterised", "message", "in a log"});

Using this the string concatenation will not take place if the log is not enabled (whereas the code in ln187 will always perform String concatenation).

Third argument should be array of argument objects

Without benchmarking, I'd guess overhead for a string concatenation like this is going to be roughly comparable to the overhead of creating/GCing that object array, especially in the context of something that only happens once per slave connection, but I can swap to that.

(My main objection is the ugliness of losing the log-level convenience wrappers + needing to manually make the array... I really wish we'd just ship FormattingLogger as part of guava.)

SC will be worse - as it still involves garbage collection for the implicit StringBuilder and Object/array creation is highly optimized
(I pine for slf4j....)

KostyaSha · 2015-05-10T23:20:21Z

👍 for seconds
If this change will be merged, then you need also update https://wiki.jenkins-ci.org/display/JENKINS/Features+controlled+by+system+properties

master & slave run the same compiled code.

Do you mean slave.jar?

deadmoose · 2015-05-11T03:53:43Z

master & slave run the same compiled code.

Do you mean slave.jar?

No, slave.jar is the remoting jar, which is just enough to bootstrap the system. After that, the Jenkins master squirts its classes over the wire to the slave.

jtnord · 2015-05-11T08:47:42Z

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

+    private static final String INTERVAL_MINUTES_PROPERTY = ChannelPinger.class.getName() + ".pingInterval";
+    private static final String INTERVAL_SECONDS_PROPERTY = ChannelPinger.class.getName() + ".pingIntervalSeconds";
+
+    private static final int DEFAULT_TIMEOUT_SECONDS = Integer.getInteger(TIMEOUT_SECONDS_PROPERTY, 4 * 60);


this is not a "default" if it gets the value from a system property.
Also there is no other code change here or linked to call the new constructor outside of the no args constructor - so this adds confusion where the attempt is to reduce it.

oleg-nenashev · 2015-05-18T17:44:44Z

Marked as WiP (comments from @jtnord )

deadmoose · 2015-05-18T20:37:40Z

@jtnord - Updated. I'd had the other constructor for tests (which I hadn't finished buttoning up), but ditched it since the test harness needs to be able to play with the system properties anyways.

Added my test class to confirm that those properties are getting read & passed along, though it does NOT actually dig into the guts of the resulting PingThread like I'd originally intended.

KostyaSha · 2015-05-18T22:33:21Z

Marked as WiP (comments from @jtnord )

@oleg-nenashev 1) what marked?
2) Just create WIP label and stop renaming topics?

jtnord · 2015-05-19T09:31:20Z

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

-        public SetUpRemotePing(int pingInterval) {
-            this.pingInterval = pingInterval;
+    static class SetUpRemotePing extends MasterToSlaveCallable<Void, IOException> {
+        private final int pingTimeoutSeconds;


I think (but may be mistaken) that this should have a serialVersionUID defined.

As mentioned in the change description, it really isn't; these don't get persisted and re-read by potentially differently-compiled code, and remoting is already taking care of making master/slave run the same compiled bits with the JVM's same auto-rolled UID.

There are already plenty of Callables demonstrating this if you grep for MasterToSlaveCallable, e.g.:
https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/util/VirtualFile.java#L417-L438
https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/slaves/restarter/JnlpSlaveRestarterInstaller.java#L52-L92
https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/slaves/StandardOutputSwapper.java#L38-L74
https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/model/Jenkins.java#L2048-L2052

There are issues with differences in JVM versions between master and slave. I have customer issues where not having a serialVersionUID defined has caused Jenkins to blow up (thankfully all of these cases were in plugins). Just because some instances of callables in Core have been lucky so far does not mean it is a safe pattern to follow.

In addition, assuming that the Jenkins core implementation of Remoting (with its current contract of classloaders) is the only implementation is a risky assumption. We have an alternative implementation for Master-to-Master communication which (for security reasons) does not allow remote class loading and as such the serialVersionUID must match for any callables that are transferred over that Channel. While we are not in a position to release that implementation at the present time, I know it is on our roadmap to release it in the future. It would be better if the habit of serialVersionUID alway was better applied in Core so that when we do open source this code we can set an earlier lower-bound on the Master version that the code applies to.

jtnord · 2015-05-20T10:20:27Z

pom.xml

+        <groupId>com.google.guava</groupId>
+        <artifactId>guava-testlib</artifactId>
+        <version>${guavaVersion}</version>
+        <scope>test</scope>


sorry didn't notice this before but adding scope to a dependency in a dependency Management section is IMHO just evil (at some point you may want to actually depend on this thing and then all the transitive dependencies break - its also not clear in the using pom that it is using it at the right scope - and I have been bitten by this on one too many occasions now).

Other than that LGTM.

Whoops, good catch. Got my two poms mixed up there.

Updated!

* Allows customization in seconds, not minutes * Allows customization of the ping timeout (before, you could set a custom interval, but the timeout would always be PingThread's 4 minute default) This also drops the serialVersionUID from ChannelPinger.SetUpRemotePing; without one provided, the JVM will generate one on demand which is sufficient for the purposes here since these are never persisted and master & slave run the same compiled code. (And it demonstrably works since countless other MasterToSlaveCallables fail to specify their own custom IDs)

jtnord · 2015-05-20T16:39:23Z

👍

oleg-nenashev · 2015-05-21T06:02:32Z

pom.xml

+      <dependency>
+        <groupId>com.google.guava</groupId>
+        <artifactId>guava-testlib</artifactId>
+        <version>${guavaVersion}</version>


If I get the code correctly, it should be included in the test scope only

See the above comments on the previous version where I'd had a mental hiccup and done just that. This is the big block that's specifying versions of things, and then it's scoped to test down in in core/pom.xml where it's actually consumed. (And then, anecdotally, it appears scoping inside dependencyManagement is completely ignored since core/pom.xml still successfully picked up a version number from the supposedly scoped definition here)

oleg-nenashev · 2015-05-21T06:04:01Z

👍 for the improvement BTW

daniel-beck · 2015-06-10T14:35:20Z

I think the serialVersionUID issue should be addressed, then this is ready to go.

oleg-nenashev · 2015-10-06T09:14:30Z

@deadmoose
There is an open comment from @daniel-beck , Moved the issue to WiP state

alvarolobato · 2016-11-24T07:00:32Z

@deadmoose are you planning on finishing this PR? If not I propose to close it. I'll submit one with similar changes.

batmat · 2016-11-28T11:48:36Z

Closing as superseded by #2645 where @deadmoose's work was preserved.

Please @deadmoose feel totally free to tell us if you feel something should (have) be done differently here. Even in private if you prefer. Thanks for your work!

KostyaSha reviewed May 10, 2015
View reviewed changes

jtnord reviewed May 11, 2015
View reviewed changes

deadmoose force-pushed the channelpinger_seconds_and_timeout branch from 9abf88c to 3743bfe Compare May 18, 2015 20:33

jtnord reviewed May 19, 2015
View reviewed changes

deadmoose force-pushed the channelpinger_seconds_and_timeout branch from 3743bfe to 8c55dd2 Compare May 19, 2015 22:16

jtnord reviewed May 20, 2015
View reviewed changes

deadmoose force-pushed the channelpinger_seconds_and_timeout branch from 8c55dd2 to b20f158 Compare May 20, 2015 16:31

oleg-nenashev reviewed May 21, 2015
View reviewed changes

oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Oct 6, 2015

oleg-nenashev added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Oct 25, 2015

daniel-beck added the unresolved-merge-conflict There is a merge conflict with the target branch. label May 9, 2016

alvarolobato mentioned this pull request Nov 24, 2016

[FIX JENKINS-28245] - Allow defining agent ping interval and ping timeout in seconds #2645

Merged

batmat added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Nov 24, 2016

batmat closed this Nov 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXES JENKINS-28245] Allow finer-grained tuning of ChannelPinger. #1687

[FIXES JENKINS-28245] Allow finer-grained tuning of ChannelPinger. #1687

deadmoose commented May 5, 2015

KostyaSha May 10, 2015

deadmoose May 11, 2015

jtnord May 19, 2015

KostyaSha May 19, 2015

deadmoose May 19, 2015

jtnord May 20, 2015

KostyaSha commented May 10, 2015

deadmoose commented May 11, 2015

jtnord May 11, 2015

oleg-nenashev commented May 18, 2015

deadmoose commented May 18, 2015

KostyaSha commented May 18, 2015

jtnord May 19, 2015

deadmoose May 19, 2015

stephenc Jun 2, 2015

jtnord May 20, 2015

deadmoose May 20, 2015

jtnord commented May 20, 2015

oleg-nenashev May 21, 2015

deadmoose May 21, 2015

oleg-nenashev Oct 20, 2015

oleg-nenashev commented May 21, 2015

daniel-beck commented Jun 10, 2015

oleg-nenashev commented Oct 6, 2015

alvarolobato commented Nov 24, 2016 •

edited

Loading

batmat commented Nov 28, 2016

[FIXES JENKINS-28245] Allow finer-grained tuning of ChannelPinger. #1687

[FIXES JENKINS-28245] Allow finer-grained tuning of ChannelPinger. #1687

Conversation

deadmoose commented May 5, 2015

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

KostyaSha commented May 10, 2015

deadmoose commented May 11, 2015

Choose a reason for hiding this comment

oleg-nenashev commented May 18, 2015

deadmoose commented May 18, 2015

KostyaSha commented May 18, 2015

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

jtnord commented May 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented May 21, 2015

daniel-beck commented Jun 10, 2015

oleg-nenashev commented Oct 6, 2015

alvarolobato commented Nov 24, 2016 • edited Loading

batmat commented Nov 28, 2016

alvarolobato commented Nov 24, 2016 •

edited

Loading