root
/
jenkins
mirror of https://github.com/jenkinsci/jenkins.git
1
0
Fork 0
Code Issues Actions 10 Packages Projects Releases Wiki Activity

[JENKINS-73835] Do not allow builds to be deleted while they are still 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
This commit is contained in:
Devin Nusbaum 2024-10-12 08:06:18 -04:00 committed by GitHub
parent 925de03663
commit d34b17ee4b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 64 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
core/src/main/java/hudson/model/Run.java
View File

@ -1551,6 +1551,9 @@ public abstract class Run<JobT extends Job<JobT, RunT>, RunT extends Run<JobT, R
* 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) {
@ -1885,12 +1888,6 @@ public abstract class Run<JobT extends Job<JobT, RunT>, RunT extends Run<JobT, R
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) {

2
core/src/main/java/hudson/tasks/LogRotator.java
View File

@ -250,7 +250,7 @@ public class LogRotator extends BuildDiscarder {
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;
}

13
core/src/main/java/jenkins/model/BackgroundGlobalBuildDiscarder.java
View File

@ -31,6 +31,7 @@ import hudson.model.TaskListener;
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;
@ -56,8 +57,18 @@ public class BackgroundGlobalBuildDiscarder extends AsyncPeriodicWork {
}
}
/**
* 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 {

13
core/src/main/java/jenkins/model/GlobalBuildDiscarderListener.java
View File

@ -35,7 +35,7 @@ import org.kohsuke.accmod.Restricted;
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)
@ -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
core/src/main/resources/hudson/model/Run/delete.jelly
View File

@ -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
test/src/test/java/hudson/cli/DeleteBuildsCommandTest.java
View File

@ -32,21 +32,18 @@ import static org.hamcrest.Matchers.containsString;
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;
/**
@ -139,8 +136,8 @@ 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));
@ -148,15 +145,9 @@ public class DeleteBuildsCommandTest {
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 {

14
test/src/test/java/hudson/model/RunTest.java
View File

@ -30,6 +30,7 @@ import static org.hamcrest.Matchers.instanceOf;
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.ExtensionList;
@ -62,6 +63,7 @@ import org.junit.Test;
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;
@ -128,6 +130,18 @@ public class RunTest {
}
}
@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);

16
test/src/test/java/hudson/tasks/LogRotatorTest.java
View File

@ -50,8 +50,10 @@ import java.util.concurrent.TimeUnit;
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;
@ -62,6 +64,9 @@ import org.jvnet.hudson.test.TestBuilder;
*/
public class LogRotatorTest {
@ClassRule
public static BuildWatcher watcher = new BuildWatcher();
@Rule
public JenkinsRule j = new JenkinsRule();
@ -96,6 +101,17 @@ public class LogRotatorTest {
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 {