Skip to content

Commit

Permalink
[JENKINS-73835] Do not allow builds to be deleted while they are stil…
Browse files Browse the repository at this point in the history
…l running and ensure build discarders run after builds are fully complete (#9810)

* [JENKINS-73835] Do not allow builds to be deleted while they are still running

* [JENKINS-73835] Avoid redundant calls to Job.logRotate when builds complete and always call Job.logRotate after build finalization

* [JENKINS-73835] Add issue reference to RunTest.buildsMayNotBeDeletedWhileRunning

* [JENKINS-73835] Adjust DeleteBuildsCommandTest.deleteBuildsShouldSuccessEvenTheBuildIsRunning to match new behavior

* [JENKINS-73835] Run/delete.jelly should check Run.isLogUpdated, not Run.isBuilding

(cherry picked from commit d34b17e)
  • Loading branch information
dwnusbaum authored and basil committed Oct 15, 2024
1 parent a347514 commit d383906
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 26 deletions.
9 changes: 3 additions & 6 deletions core/src/main/java/hudson/model/Run.java
Original file line number Diff line number Diff line change
Expand Up @@ -1551,6 +1551,9 @@ public synchronized void deleteArtifacts() throws IOException {
* if we fail to delete.
*/
public void delete() throws IOException {
if (isLogUpdated()) {
throw new IOException("Unable to delete " + this + " because it is still running");
}
synchronized (this) {
// Avoid concurrent delete. See https://issues.jenkins.io/browse/JENKINS-61687
if (isPendingDelete) {
Expand Down Expand Up @@ -1883,12 +1886,6 @@ protected final void execute(@NonNull RunExecution job) {
LOGGER.log(Level.SEVERE, "Failed to save build record", e);
}
}

try {
getParent().logRotate();
} catch (Exception e) {
LOGGER.log(Level.SEVERE, "Failed to rotate log", e);
}
} finally {
onEndBuilding();
if (logger != null) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/tasks/LogRotator.java
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ private boolean shouldKeepRun(Run r, Run lsb, Run lstb) {
LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s the last stable build", r);
return true;
}
if (r.isBuilding()) {
if (r.isLogUpdated()) {
LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s still building", r);
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.io.IOException;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Stream;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

Expand All @@ -56,8 +57,18 @@ protected void execute(TaskListener listener) throws IOException, InterruptedExc
}
}

/**
* Runs all globally configured build discarders against a job.
*/
public static void processJob(TaskListener listener, Job job) {
GlobalBuildDiscarderConfiguration.get().getConfiguredBuildDiscarders().forEach(strategy -> {
processJob(listener, job, GlobalBuildDiscarderConfiguration.get().getConfiguredBuildDiscarders().stream());
}

/**
* Runs the specified build discarders against a job.
*/
public static void processJob(TaskListener listener, Job job, Stream<GlobalBuildDiscarderStrategy> strategies) {
strategies.forEach(strategy -> {
String displayName = strategy.getDescriptor().getDisplayName();
if (strategy.isApplicable(job)) {
try {
Expand Down
13 changes: 11 additions & 2 deletions core/src/main/java/jenkins/model/GlobalBuildDiscarderListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* Run background build discarders on an individual job once a build is finalized
* Run build discarders on an individual job once a build is finalized
*/
@Extension
@Restricted(NoExternalUse.class)
Expand All @@ -46,6 +46,15 @@ public class GlobalBuildDiscarderListener extends RunListener<Run> {
@Override
public void onFinalized(Run run) {
Job job = run.getParent();
BackgroundGlobalBuildDiscarder.processJob(new LogTaskListener(LOGGER, Level.FINE), job);
try {
// Job-level build discarder execution is unconditional.
job.logRotate();
} catch (Exception e) {
LOGGER.log(Level.WARNING, e, () -> "Failed to rotate log for " + run);
}
// Avoid calling Job.logRotate twice in case JobGlobalBuildDiscarderStrategy is configured globally.
BackgroundGlobalBuildDiscarder.processJob(new LogTaskListener(LOGGER, Level.FINE), job,
GlobalBuildDiscarderConfiguration.get().getConfiguredBuildDiscarders().stream()
.filter(s -> !(s instanceof JobGlobalBuildDiscarderStrategy)));
}
}
2 changes: 1 addition & 1 deletion core/src/main/resources/hudson/model/Run/delete.jelly
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ THE SOFTWARE.
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
<j:if test="${!it.building and !it.keepLog}">
<j:if test="${!it.logUpdated and !it.keepLog}">
<l:task href="${buildUrl.baseUrl}/confirmDelete" icon="icon-edit-delete icon-md" permission="${it.DELETE}" title="${%delete.build(it.displayName)}"/>
</j:if>
</j:jelly>
21 changes: 6 additions & 15 deletions test/src/test/java/hudson/cli/DeleteBuildsCommandTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,18 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assume.assumeFalse;

import hudson.Functions;
import hudson.model.ExecutorTest;
import hudson.model.FreeStyleProject;
import hudson.model.Item;
import hudson.model.Run;
import hudson.model.labels.LabelAtom;
import hudson.tasks.Shell;
import java.io.IOException;
import jenkins.model.Jenkins;
import org.junit.AssumptionViolatedException;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;

/**
Expand Down Expand Up @@ -139,24 +136,18 @@ public class DeleteBuildsCommandTest {
assertThat(result.stdout(), containsString("Deleted 0 builds"));
}

@Test public void deleteBuildsShouldSuccessEvenTheBuildIsRunning() throws Exception {
assumeFalse("You can't delete files that are in use on Windows", Functions.isWindows());
@Issue("JENKINS-73835")
@Test public void deleteBuildsShouldFailIfTheBuildIsRunning() throws Exception {
FreeStyleProject project = j.createFreeStyleProject("aProject");
ExecutorTest.startBlockingBuild(project);
assertThat(((FreeStyleProject) j.jenkins.getItem("aProject")).getBuilds(), hasSize(1));

final CLICommandInvoker.Result result = command
.authorizedTo(Jenkins.READ, Item.READ, Run.DELETE)
.invokeWithArgs("aProject", "1");
assertThat(result, succeeded());
assertThat(result.stdout(), containsString("Deleted 1 builds"));
assertThat(((FreeStyleProject) j.jenkins.getItem("aProject")).getBuilds(), hasSize(0));
assertThat(project.isBuilding(), equalTo(false));
try {
project.delete();
} catch (IOException | InterruptedException x) {
throw new AssumptionViolatedException("Could not delete test project (race condition?)", x);
}
assertThat(result, failedWith(1));
assertThat(result, hasNoStandardOutput());
assertThat(result.stderr(), containsString("Unable to delete aProject #1 because it is still running"));
}

@Test public void deleteBuildsShouldSuccessEvenTheBuildIsStuckInTheQueue() throws Exception {
Expand Down
14 changes: 14 additions & 0 deletions test/src/test/java/hudson/model/RunTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;

import hudson.FilePath;
Expand Down Expand Up @@ -59,6 +60,7 @@
import org.junit.experimental.categories.Category;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.SleepBuilder;
import org.jvnet.hudson.test.SmokeTest;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.stapler.DataBoundConstructor;
Expand Down Expand Up @@ -114,6 +116,18 @@ public class RunTest {
assertTrue(Mgr.deleted.get());
}

@Issue("JENKINS-73835")
@Test public void buildsMayNotBeDeletedWhileRunning() throws Exception {
var p = j.createFreeStyleProject();
p.getBuildersList().add(new SleepBuilder(999999));
var b = p.scheduleBuild2(0).waitForStart();
var ex = assertThrows(IOException.class, () -> b.delete());
assertThat(ex.getMessage(), containsString("Unable to delete " + b + " because it is still running"));
b.getExecutor().interrupt();
j.waitForCompletion(b);
b.delete(); // Works fine.
}

@Issue("SECURITY-1902")
@Test public void preventXssInBadgeTooltip() throws Exception {
j.jenkins.setQuietPeriod(0);
Expand Down
16 changes: 16 additions & 0 deletions test/src/test/java/hudson/tasks/LogRotatorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@
import java.util.concurrent.TimeoutException;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.FailureBuilder;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
Expand All @@ -62,6 +64,9 @@
*/
public class LogRotatorTest {

@ClassRule
public static BuildWatcher watcher = new BuildWatcher();

@Rule
public JenkinsRule j = new JenkinsRule();

Expand Down Expand Up @@ -96,6 +101,17 @@ public void successVsFailureWithRemoveLastBuild() throws Exception {
assertEquals(2, numberOf(project.getLastFailedBuild()));
}

@Test
public void ableToDeleteCurrentBuild() throws Exception {
var p = j.createFreeStyleProject();
// Keep 0 builds, i.e. immediately delete builds as they complete.
LogRotator logRotator = new LogRotator(-1, 0, -1, -1);
logRotator.setRemoveLastBuild(true);
p.setBuildDiscarder(logRotator);
j.buildAndAssertStatus(Result.SUCCESS, p);
assertNull(p.getBuildByNumber(1));
}

@Test
@Issue("JENKINS-2417")
public void stableVsUnstable() throws Exception {
Expand Down

0 comments on commit d383906

Please sign in to comment.