Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-54078] JEP-210 compatibility #74

Merged
merged 10 commits into from
Oct 26, 2018
Merged

[JENKINS-54078] JEP-210 compatibility #74

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2ad8c70
[JENKINS-26521] Prove that Channel.export is needed for JEP-210 compa…
jglick Oct 8, 2018
11c9999
Updated test to match change in logging format.
jglick Oct 9, 2018
746f455
[JENKINS-54078] Reproduced error in test.
jglick Oct 16, 2018
f66bb29
May as well use nonbeta versions of JEP-210 plugins.
jglick Oct 16, 2018
128f7d2
[JENKINS-54078] Do not rely on Channel.export as we have little contr…
jglick Oct 16, 2018
5d8c464
https://github.com/jenkinsci/workflow-durable-task-step-plugin/pull/7…
jglick Oct 23, 2018
0918918
Handle deserialization of old ConsoleLogFilterImpl with null id.
jglick Oct 23, 2018
46433e1
Minimize the number of ResetTimer calls made.
jglick Oct 25, 2018
3af06c9
If the Tick sees an idle stream, reschedule sooner to avoid a false t…
jglick Oct 25, 2018
066e961
Retain the original ConsoleLogFilterImpl for derialization, but do no…
jglick Oct 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 39 additions & 8 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.19</version>
<version>3.25</version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various changes to pick up JEP-210 binaries.

<relativePath />
</parent>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -67,10 +67,12 @@
<jenkins.version>2.121.1</jenkins.version>
<java.level>8</java.level>
<workflow-step-api-plugin.version>2.15</workflow-step-api-plugin.version>
<workflow-cps-plugin.version>2.32</workflow-cps-plugin.version>
<workflow-support-plugin.version>2.14</workflow-support-plugin.version>
<workflow-api-plugin.version>2.28</workflow-api-plugin.version>
<workflow-cps-plugin.version>2.58</workflow-cps-plugin.version>
<workflow-support-plugin.version>2.21</workflow-support-plugin.version>
<workflow-api-plugin.version>2.30</workflow-api-plugin.version>
<useBeta>true</useBeta>
<git-plugin.version>4.0.0-beta3</git-plugin.version>
<scm-api-plugin.version>2.2.6</scm-api-plugin.version>
</properties>
<dependencies>
<dependency>
Expand Down Expand Up @@ -98,7 +100,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.14</version>
<version>1.17</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
Expand All @@ -121,13 +123,13 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
<version>2.10</version>
<version>2.26</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-durable-task-step</artifactId>
<version>2.3</version>
<version>2.24</version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR this was to pick up the fix of JENKINS-28822.

<scope>test</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -182,7 +184,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.28</version>
<version>1.46</version>
<scope>test</scope>
</dependency>
<dependency>
Expand All @@ -204,5 +206,34 @@
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>git</artifactId>
<version>${git-plugin.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
<version>${scm-api-plugin.version}</version>
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>git</artifactId>
<version>${git-plugin.version}</version>
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
</dependencies>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
<version>${scm-api-plugin.version}</version>
</dependency>
</dependencies>
</dependencyManagement>
</project>
54 changes: 40 additions & 14 deletions src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,21 @@
import hudson.model.Result;
import hudson.model.Run;
import hudson.model.TaskListener;
import hudson.remoting.Callable;
import hudson.remoting.Channel;
import java.io.IOException;
import java.io.OutputStream;
import java.io.Serializable;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;
import jenkins.model.CauseOfInterruption;
import jenkins.security.SlaveToMasterCallable;
import jenkins.util.Timer;
import org.jenkinsci.plugins.workflow.flow.FlowExecution;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
Expand All @@ -46,10 +50,14 @@ public class TimeoutStepExecution extends AbstractStepExecutionImpl {
/** whether we are forcing the body to end */
private boolean forcible;

/** Token for {@link #activity} callbacks. */
private final String id;

TimeoutStepExecution(TimeoutStep step, StepContext context) {
super(context);
this.step = step;
this.activity = step.isActivity();
id = activity ? UUID.randomUUID().toString() : null;
}

@Override
Expand All @@ -62,7 +70,7 @@ public boolean start() throws Exception {
bodyInvoker = bodyInvoker.withContext(
BodyInvoker.mergeConsoleLogFilters(
context.get(ConsoleLogFilter.class),
new ConsoleLogFilterImpl(new ResetCallbackImpl())
new ConsoleLogFilterImpl(id)
)
);
}
Expand Down Expand Up @@ -230,29 +238,42 @@ public String getShortDescription() {
}
}

public interface ResetCallback extends Serializable {
void logWritten();
}
private static final class ResetTimer extends SlaveToMasterCallable<Void, RuntimeException> {

private class ResetCallbackImpl implements ResetCallback {
private static final long serialVersionUID = 1L;
@Override public void logWritten() {
resetTimer();

/** non-null unless running from an old deserialized ConsoleLogFilterImpl */
private final @CheckForNull String id;

ResetTimer(String id) {
dwnusbaum marked this conversation as resolved.
Show resolved Hide resolved
this.id = id;
}

@Override public Void call() throws RuntimeException {
StepExecution.applyAll(TimeoutStepExecution.class, e -> {
if (id == null || id.equals(e.id)) {
e.resetTimer();
}
return null;
});
return null;
}

}

private static class ConsoleLogFilterImpl extends ConsoleLogFilter implements /* TODO Remotable */ Serializable {
private static final long serialVersionUID = 1L;

private final ResetCallback callback;
private final @CheckForNull String id;
private transient @CheckForNull Channel channel;

ConsoleLogFilterImpl(ResetCallback callback) {
this.callback = callback;
ConsoleLogFilterImpl(String id) {
this.id = id;
}

private Object writeReplace() {
Channel ch = Channel.current();
return ch == null ? this : new ConsoleLogFilterImpl(ch.export(ResetCallback.class, callback));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the issue is that only the marshaller holds the reference to the exported object, so after the object is completely serialized it is free to be garbage collected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT the issue was that this code released all exported objects when a Callable finished, insofar as those objects had originally been exported within the dynamic scope of that Callable’s serialization. Or something like this. I could not follow exactly what the logic was, and the documentation was scant. At any rate, the revised strategy with the SlaveToMasterCallable is simpler to reason about explicitly.

private Object readResolve() {
channel = Channel.current();
return this;
}

@Override
Expand All @@ -262,7 +283,12 @@ public OutputStream decorateLogger(@SuppressWarnings("rawtypes") Run build, fina
@Override
protected void eol(byte[] b, int len) throws IOException {
logger.write(b, 0, len);
callback.logWritten();
Callable<Void, RuntimeException> resetTimer = new ResetTimer(id);
if (channel != null) {
channel.callAsync(resetTimer);
} else {
resetTimer.call();
}
}

@Override
Expand Down
8 changes: 4 additions & 4 deletions src/test/java/org/jenkinsci/plugins/workflow/steps/EnvStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,18 @@ public class EnvStepTest {
WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition(
"parallel a: {\n" +
" node {withEnv(['TOOL=aloc']) {semaphore 'a'; isUnix() ? sh('echo TOOL=$TOOL') : bat('echo TOOL=%TOOL%')}}\n" +
" node {withEnv(['TOOL=aloc']) {semaphore 'a'; isUnix() ? sh('echo a TOOL=$TOOL') : bat('echo a TOOL=%TOOL%')}}\n" +
"}, b: {\n" +
" node {withEnv(['TOOL=bloc']) {semaphore 'b'; isUnix() ? sh('echo TOOL=$TOOL') : bat('echo TOOL=%TOOL%')}}\n" +
" node {withEnv(['TOOL=bloc']) {semaphore 'b'; isUnix() ? sh('echo b TOOL=$TOOL') : bat('echo b TOOL=%TOOL%')}}\n" +
"}", true));
WorkflowRun b = p.scheduleBuild2(0).getStartCondition().get();
SemaphoreStep.waitForStart("a/1", b);
SemaphoreStep.waitForStart("b/1", b);
SemaphoreStep.success("a/1", null);
SemaphoreStep.success("b/1", null);
story.j.assertBuildStatusSuccess(story.j.waitForCompletion(b));
story.j.assertLogContains("[a] TOOL=aloc", b);
story.j.assertLogContains("[b] TOOL=bloc", b);
story.j.assertLogContains("a TOOL=aloc", b);
story.j.assertLogContains("b TOOL=bloc", b);
}
});
}
Expand Down
41 changes: 41 additions & 0 deletions src/test/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

package org.jenkinsci.plugins.workflow.steps;

import hudson.Functions;
import hudson.model.Result;
import hudson.model.TaskListener;
import hudson.model.listeners.RunListener;
Expand All @@ -33,6 +34,7 @@
import java.util.concurrent.TimeUnit;
import jenkins.model.CauseOfInterruption;
import jenkins.model.InterruptedBuildAction;
import jenkins.plugins.git.GitSampleRepoRule;
import org.jenkinsci.plugins.workflow.actions.ErrorAction;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.cps.nodes.StepAtomNode;
Expand All @@ -43,6 +45,7 @@
import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
import org.junit.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assume.*;
import org.junit.runners.model.Statement;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
Expand All @@ -57,6 +60,8 @@ public class TimeoutStepTest extends Assert {

@Rule public RestartableJenkinsRule story = new RestartableJenkinsRule();

@Rule public GitSampleRepoRule git = new GitSampleRepoRule();

@Test public void configRoundTrip() {
story.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
Expand Down Expand Up @@ -239,6 +244,42 @@ public void evaluate() throws Throwable {
});
}

@Test
public void activityRemote() {
assumeFalse(Functions.isWindows()); // TODO create analogue using bat
story.then(r -> {
r.createSlave();
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("" +
"node('!master') {\n" +
" timeout(time:5, unit:'SECONDS', activity: true) {\n" +
" sh 'set +x; echo NotHere; sleep 3; echo NotHereYet; sleep 3; echo JustHere; sleep 10; echo ShouldNot'\n" +
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot use echo steps as in the other tests, because we need to demonstrate serialization of the ConsoleLogFilter impl, which only happens if a nested step actually remotes its TaskListener.

" }\n" +
"}\n", true));
WorkflowRun b = r.assertBuildStatus(Result.ABORTED, p.scheduleBuild2(0));
story.j.assertLogContains("JustHere", b);
story.j.assertLogNotContains("ShouldNot", b);
});
}

@Issue("JENKINS-54078")
@Test public void activityGit() {
story.then(r -> {
r.createSlave();
git.init();
git.write("file", "content");
git.git("commit", "--all", "--message=init");
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("" +
"node('!master') {\n" +
" timeout(time: 5, unit: 'MINUTES', activity: true) {\n" +
" git($/" + git + "/$)\n" +
" }\n" +
"}\n", true));
r.buildAndAssertSuccess(p);
});
}

@Issue("JENKINS-26163")
@Test
public void restarted() throws Exception {
Expand Down