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

Store DownstreamBuildAction on Run instead of FlowNode for consistent persistence even on completed builds #128

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Store DownstreamBuildAction on Run instead of FlowNode for consistent persistence even on completed builds #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f5ed9d0
Make DownstreamBuildAction immutable to fix persistence across restar…
dwnusbaum Dec 14, 2023
cd63d16
Store DownstreamBuildAction on Run instead of FlowNode for consistent…
dwnusbaum Dec 14, 2023
ec1d5f8
Refactor DownstreamBuildAction to hold multiple downstream builds int…
dwnusbaum Dec 14, 2023
339adf5
Change DownstreamBuildAction.downstreamBuilds from a Map to a List
dwnusbaum Dec 14, 2023
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
8 changes: 1 addition & 7 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
<dependency>
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-2.387.x</artifactId>
<version>2143.ve4c3c9ec790a</version>
<version>2543.vfb_1a_5fb_9496d</version>
<scope>import</scope>
<type>pom</type>
</dependency>
Expand Down Expand Up @@ -90,16 +90,12 @@
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<scope>test</scope>
<!-- TODO: Remove version declaration when workflow-cps plugin 3691.v28b_14c465a_b_b_ or newer is in plugin BOM -->
<version>3691.v28b_14c465a_b_b_</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<classifier>tests</classifier>
<scope>test</scope>
<!-- TODO: Remove version declaration when workflow-cps plugin 3691.v28b_14c465a_b_b_ or newer is in plugin BOM -->
<version>3691.v28b_14c465a_b_b_</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down Expand Up @@ -151,8 +147,6 @@
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>git</artifactId>
<scope>test</scope>
<!-- TODO: Remove version declaration when git plugin 5.1.0 or newer is in plugin BOM -->
<version>5.1.0</version>
</dependency>
<dependency>
<groupId>org.awaitility</groupId>
Expand Down
27 changes: 5 additions & 22 deletions src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import java.util.logging.Logger;
import jenkins.util.Timer;
import org.jenkinsci.plugins.workflow.actions.WarningAction;
import org.jenkinsci.plugins.workflow.flow.FlowExecution;
import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
Expand Down Expand Up @@ -88,32 +87,16 @@ public void onDeleted(final Run<?,?> run) {
}
}

private void updateDownstreamBuildAction(Run<?, ?> run) {
for (Cause cause : run.getCauses()) {
private void updateDownstreamBuildAction(Run<?, ?> downstream) {
for (Cause cause : downstream.getCauses()) {
if (cause instanceof BuildUpstreamCause) {
BuildUpstreamCause buildUpstreamCause = (BuildUpstreamCause) cause;
Run<?, ?> upstream = buildUpstreamCause.getUpstreamRun();
if (upstream instanceof FlowExecutionOwner.Executable) {
FlowExecutionOwner owner = ((FlowExecutionOwner.Executable) upstream).asFlowExecutionOwner();
if (owner == null) {
LOGGER.log(Level.FINE, () -> "Unable to update DownstreamBuildAction for " + upstream + " node " + buildUpstreamCause.getNodeId());
continue;
}
String flowNodeId = buildUpstreamCause.getNodeId();
DownstreamBuildAction.getOrCreate(upstream, flowNodeId, downstream.getParent()).setBuild(downstream);
try {
FlowExecution execution = owner.get();
FlowNode node = execution.getNode(buildUpstreamCause.getNodeId());
if (node == null) {
LOGGER.log(Level.FINE, () -> "Unable to update DownstreamBuildAction for " + upstream + " node " + buildUpstreamCause.getNodeId());
continue;
}
DownstreamBuildAction downstreamAction = node.getPersistentAction(DownstreamBuildAction.class);
if (downstreamAction == null) {
// Should only happen for builds already in the queue when this plugin is updated to include DownstreamBuildAction.
downstreamAction = new DownstreamBuildAction(run.getParent());
node.addAction(downstreamAction);
}
downstreamAction.setBuild(run);
run.save();
upstream.save();
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment this out and downstreamBuildActionUpstreamCompletesBeforeDownstreamStarts will fail.

} catch (IOException e) {
LOGGER.log(Level.FINE, e, () -> "Unable to update DownstreamBuildAction for " + upstream + " node " + buildUpstreamCause.getNodeId());
}
Expand Down
10 changes: 6 additions & 4 deletions ...in/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,21 +66,23 @@
@Override
public boolean start() throws Exception {
String job = step.getJob();
Item item = Jenkins.get().getItem(job, getContext().get(Run.class).getParent(), Item.class);
Run<?, ?> upstream = getContext().get(Run.class);
Item item = Jenkins.get().getItem(job, upstream.getParent(), Item.class);
if (item == null) {
throw new AbortException("No item named " + job + " found");
}
item.checkPermission(Item.BUILD);
if ((step.getWait() || step.getWaitForStart()) && !(item instanceof Job)) {
// TODO find some way of allowing ComputedFolders to hook into the listener code

Check warning on line 76 in src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepExecution.java

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

TODO 

NORMAL:
find some way of allowing ComputedFolders to hook into the listener code
throw new AbortException("Waiting for non-job items is not supported");
}

FlowNode node = getContext().get(FlowNode.class);
node.addAction(new DownstreamBuildAction(item));
DownstreamBuildAction.getOrCreate(upstream, node.getId(), item);

List<Action> actions = new ArrayList<>();
actions.add(new CauseAction(new BuildUpstreamCause(getContext().get(FlowNode.class), getContext().get(Run.class))));
actions.add(new BuildUpstreamNodeAction(node, getContext().get(Run.class)));
actions.add(new CauseAction(new BuildUpstreamCause(getContext().get(FlowNode.class), upstream)));
actions.add(new BuildUpstreamNodeAction(node, upstream));

if (item instanceof ParameterizedJobMixIn.ParameterizedJob) {
final ParameterizedJobMixIn.ParameterizedJob project = (ParameterizedJobMixIn.ParameterizedJob) item;
Expand Down
96 changes: 68 additions & 28 deletions src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/DownstreamBuildAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,47 +6,87 @@
import hudson.model.Item;
import hudson.model.ItemGroup;
import hudson.model.Run;
import org.jenkinsci.plugins.workflow.actions.PersistentAction;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.springframework.security.access.AccessDeniedException;

/**
* Tracks the downstream build triggered by the {@code build} step.
* Tracks downstream builds triggered by the {@code build} step, as well as the {@link FlowNode#getId} of the step.
*
* @see BuildUpstreamCause
*/
public final class DownstreamBuildAction extends InvisibleAction implements PersistentAction {
private final String jobFullName;
private Integer buildNumber;
public final class DownstreamBuildAction extends InvisibleAction {
private final List<DownstreamBuild> downstreamBuilds = new ArrayList<>();

DownstreamBuildAction(Item job) {
this.jobFullName = job.getFullName();
public static @NonNull DownstreamBuild getOrCreate(@NonNull Run<?, ?> run, @NonNull String flowNodeId, @NonNull Item job) {
DownstreamBuildAction downstreamBuildAction;
synchronized (DownstreamBuildAction.class) {
Copy link
Member Author

@dwnusbaum dwnusbaum Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unnecessary in the normal case since the relevant part of BuildTriggerStepExecution runs synchronously on the CPS VM thread, but if there are queued triggered builds when updating Jenkins to pick up this PR or when Jenkins crashes then there is a possibility of race conditions leading to multiple instances of DownstreamBuildAction without some kind of synchronization. We could drop the synchronization if we don't care about that case though.

Copy link
Member

Choose a reason for hiding this comment

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

May as well keep it, does not add much clutter.

downstreamBuildAction = run.getAction(DownstreamBuildAction.class);
if (downstreamBuildAction == null) {
downstreamBuildAction = new DownstreamBuildAction();
run.addAction(downstreamBuildAction);
}
}
return downstreamBuildAction.getOrAddDownstreamBuild(flowNodeId, job);
}

public @NonNull String getJobFullName() {
return jobFullName;
public synchronized @NonNull List<DownstreamBuild> getDownstreamBuilds() {
return Collections.unmodifiableList(new ArrayList<>(downstreamBuilds));
}

/**
* Get the build number of the downstream build, or {@code null} if the downstream build has not yet started or the queue item was cancelled.
*/
public @CheckForNull Integer getBuildNumber() {
return buildNumber;
private synchronized @NonNull DownstreamBuild getOrAddDownstreamBuild(@NonNull String flowNodeId, @NonNull Item job) {
for (DownstreamBuild build : downstreamBuilds) {
if (build.getFlowNodeId().equals(flowNodeId)) {
return build;
}
}
var build = new DownstreamBuild(flowNodeId, job);
downstreamBuilds.add(build);
return build;
}

/**
* Load the downstream build, if it has started and still exists.
* <p>Loading builds indiscriminately will affect controller performance, so use this carefully. If you only need
* to know whether the build started at one point, use {@link #getBuildNumber}.
* @throws AccessDeniedException as per {@link ItemGroup#getItem}
*/
public @CheckForNull Run<?, ?> getBuild() throws AccessDeniedException {
if (buildNumber == null) {
return null;
}
return Run.fromExternalizableId(jobFullName + '#' + buildNumber);
}
public static final class DownstreamBuild {
private final String flowNodeId;
private final String jobFullName;
private Integer buildNumber;

DownstreamBuild(String flowNodeId, @NonNull Item job) {
this.flowNodeId = flowNodeId;
this.jobFullName = job.getFullName();
}

public @NonNull String getFlowNodeId() {
return flowNodeId;
}

void setBuild(Run<?, ?> build) {
this.buildNumber = build.getNumber();
public @NonNull String getJobFullName() {
return jobFullName;
}

/**
* Get the build number of the downstream build, or {@code null} if the downstream build has not yet started or the queue item was cancelled.
*/
public @CheckForNull Integer getBuildNumber() {
return buildNumber;
}

/**
* Load the downstream build, if it has started and still exists.
* <p>Loading builds indiscriminately will affect controller performance, so use this carefully. If you only need
* to know whether the build started at one point, use {@link #getBuildNumber}.
* @throws AccessDeniedException as per {@link ItemGroup#getItem}
*/
public @CheckForNull Run<?, ?> getBuild() throws AccessDeniedException {
if (buildNumber == null) {
return null;
}
return Run.fromExternalizableId(jobFullName + '#' + buildNumber);
}

void setBuild(@NonNull Run<?, ?> run) {
this.buildNumber = run.getNumber();
}
}
}
89 changes: 87 additions & 2 deletions .../java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepRestartTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,37 @@

import hudson.model.FreeStyleBuild;
import hudson.model.FreeStyleProject;
import hudson.model.Label;
import hudson.model.Queue;
import hudson.model.Result;
import hudson.model.Run;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.junit.Assert;
import org.jenkinsci.plugins.workflow.support.steps.build.DownstreamBuildAction.DownstreamBuild;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.JenkinsSessionRule;
import static org.awaitility.Awaitility.await;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;

/**
* @author Vivek Pandey
*/
public class BuildTriggerStepRestartTest extends Assert {
public class BuildTriggerStepRestartTest {

@ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
@Rule public JenkinsSessionRule sessions = new JenkinsSessionRule();
Expand Down Expand Up @@ -64,6 +76,79 @@ public void restartBetweenJobs() throws Throwable {
sessions.then(j -> j.jenkins.getItemByFullName("test1", FreeStyleProject.class).getBuildByNumber(1).delete());
}

@Test
public void downstreamBuildActionUpstreamCompletesBeforeDownstreamStarts() throws Throwable {
sessions.then(j -> {
FreeStyleProject downstream = j.createFreeStyleProject("downstream");
downstream.setAssignedLabel(Label.parseExpression("agent"));
WorkflowJob upstream = j.jenkins.createProject(WorkflowJob.class, "upstream");
upstream.setDefinition(new CpsFlowDefinition("build job: 'downstream', wait: false", true));
WorkflowRun upstreamRun = j.buildAndAssertSuccess(upstream);
// Check action while the build is still in the queue.
String buildStepId = BuildTriggerStepTest.findFirstNodeWithDescriptor(upstreamRun.getExecution(), BuildTriggerStep.DescriptorImpl.class).getId();
var downstreamBuildAction = upstreamRun.getAction(DownstreamBuildAction.class);
var downstreamBuild = downstreamBuildAction.getDownstreamBuilds().get(0);
assertThat(downstreamBuild.getFlowNodeId(), equalTo(buildStepId));
assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
assertThat(downstreamBuild.getBuildNumber(), nullValue());
assertThat(downstreamBuild.getBuild(), nullValue());
// Check again once the build has started.
j.createOnlineSlave(Label.parseExpression("agent"));
await().atMost(10, TimeUnit.SECONDS).until(() -> downstreamBuild.getBuildNumber(), notNullValue());
Run<?, ?> downstreamRun = downstream.getLastBuild();
assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
assertThat(downstreamBuild.getBuildNumber(), equalTo(downstreamRun.getNumber()));
assertThat(downstreamBuild.getBuild(), equalTo(downstreamRun));
j.waitForCompletion(downstreamRun);
});
sessions.then(j -> {
FreeStyleProject downstream = j.jenkins.getItemByFullName("downstream", FreeStyleProject.class);
WorkflowJob upstream = j.jenkins.getItemByFullName("upstream", WorkflowJob.class);
WorkflowRun upstreamRun = upstream.getLastBuild();
String buildStepId = BuildTriggerStepTest.findFirstNodeWithDescriptor(upstreamRun.getExecution(), BuildTriggerStep.DescriptorImpl.class).getId();
var downstreamBuildAction = upstreamRun.getAction(DownstreamBuildAction.class);
var downstreamBuild = downstreamBuildAction.getDownstreamBuilds().get(0);
assertThat(downstreamBuild.getFlowNodeId(), equalTo(buildStepId));
assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
assertThat(downstreamBuild.getBuildNumber(), equalTo(downstream.getLastBuild().getNumber()));
assertThat(downstreamBuild.getBuild(), equalTo(downstream.getLastBuild()));
});
}

@Test
public void downstreamBuildActionMultipleBuilds() throws Throwable {
sessions.then(j -> {
FreeStyleProject downstream = j.createFreeStyleProject("downstream");
WorkflowJob upstream = j.jenkins.createProject(WorkflowJob.class, "upstream");
upstream.setDefinition(new CpsFlowDefinition("build 'downstream'; build 'downstream'", true));
WorkflowRun upstreamRun = j.buildAndAssertSuccess(upstream);
List<DownstreamBuild> downstreamBuilds = upstreamRun.getAction(DownstreamBuildAction.class).getDownstreamBuilds();
await().atMost(10, TimeUnit.SECONDS).until(
() -> downstreamBuilds.stream().map(DownstreamBuild::getBuildNumber).filter(Objects::nonNull).count(),
equalTo(2L));
for (Run<?, ?> downstreamRun : downstream.getBuilds()) {
String nodeId = downstreamRun.getCause(BuildUpstreamCause.class).getNodeId();
var downstreamBuild = downstreamBuilds.stream().filter(db -> db.getFlowNodeId().equals(nodeId)).findFirst().get();
assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
assertThat(downstreamBuild.getBuildNumber(), equalTo(downstreamRun.getNumber()));
assertThat(downstreamBuild.getBuild(), equalTo(downstreamRun));
}
});
sessions.then(j -> {
FreeStyleProject downstream = j.jenkins.getItemByFullName("downstream", FreeStyleProject.class);
WorkflowJob upstream = j.jenkins.getItemByFullName("upstream", WorkflowJob.class);
WorkflowRun upstreamRun = upstream.getLastBuild();
List<DownstreamBuild> downstreamBuilds = upstreamRun.getAction(DownstreamBuildAction.class).getDownstreamBuilds();
for (Run<?, ?> downstreamRun : downstream.getBuilds()) {
String nodeId = downstreamRun.getCause(BuildUpstreamCause.class).getNodeId();
var downstreamBuild = downstreamBuilds.stream().filter(db -> db.getFlowNodeId().equals(nodeId)).findFirst().get();
assertThat(downstreamBuild.getJobFullName(), equalTo(downstream.getFullName()));
assertThat(downstreamBuild.getBuildNumber(), equalTo(downstreamRun.getNumber()));
assertThat(downstreamBuild.getBuild(), equalTo(downstreamRun));
}
});
}

private static void assertFreeStyleProjectsInQueue(int count, JenkinsRule j) {
Queue.Item[] items = j.jenkins.getQueue().getItems();
int actual = 0;
Expand Down
Loading
Loading