Skip to content

[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

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

Merged
MarkEWaite merged 6 commits into from
Oct 12, 2024
Merged

[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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf30fcc
[JENKINS-73835] Do not allow builds to be deleted while they are stil…
dwnusbaum Sep 30, 2024
eec23fd
[JENKINS-73835] Avoid redundant calls to Job.logRotate when builds co…
dwnusbaum Oct 1, 2024
97fb6aa
[JENKINS-73835] Add issue reference to RunTest.buildsMayNotBeDeletedW…
dwnusbaum Oct 1, 2024
9a6a90a
[JENKINS-73835] Adjust DeleteBuildsCommandTest.deleteBuildsShouldSucc…
dwnusbaum Oct 2, 2024
f676be5
[JENKINS-73835] Run/delete.jelly should check Run.isLogUpdated, not R…
dwnusbaum Oct 2, 2024
9aaa7ba
Merge branch 'master' into JENKINS-73835
dwnusbaum Oct 8, 2024
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
9 changes: 3 additions & 6 deletions core/src/main/java/hudson/model/Run.java
View file
Open in desktop
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);
}
Comment on lines -1888 to -1893
Copy link
Member Author

@dwnusbaum dwnusbaum Oct 1, 2024

Choose a reason for hiding this comment

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

I moved this into GlobalBuildDiscarderListener so that isLogUpdated() == true when log rotation runs. It still runs synchronously, it just runs a little bit later now down in onEndBuilding. This allows us to check isLogUpdated() elsewhere to avoid race conditions with Pipeline builds.

} finally {
onEndBuilding();
if (logger != null) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/tasks/LogRotator.java
View file
Open in desktop
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()) {
Copy link
Member Author

@dwnusbaum dwnusbaum Oct 1, 2024

Choose a reason for hiding this comment

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

This is to avoid race conditions involving Pipeline builds. Previously it was possible for log rotation to delete builds which had not yet fully completed, which meant that a build.xml file could be written back out into the build directory after log rotation had deleted the build. I will try to demonstrate this downstream in workflow-job once I have an incremental build here.

Copy link
Member

@jglick jglick Oct 2, 2024

Choose a reason for hiding this comment

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

Are you confident that this is safe for freestyle builds? I.e., that !logUpdated → !building or conversely that building → logUpdated? The lifecycles for AbstractBuild and WorkflowRun are pretty different and the Javadoc has always been vague.

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am pretty sure it is safe, but if you prefer we can switch to isBuilding() || isLogUpdated() like the check for the UI in Jelly here (or maybe we should simplify that to only check isLogUpdated as well). See the non-Pipeline implementations here and the state enum here.

For !logUpdated → !building,!isLogUpdated is only true in State.COMPLETED, which ensures !isBuilding since COMPLETED is after POST_PRODUCTION.

The converse building → logUpdated should be ok as well, since if isBuilding then we must be in State.NOT_STARTED or State.BUILIDING, both of which are not COMPLETED, so isLogUpdated will be true.

For Pipeline things are simpler since WorkflowRun.isLogUpdated delegates to WorkflowRun.isBuilding.

Also just as a data point, if isLogUpdated could not be used on its own for non-pipelines, I think we would have seen it cause issues in tests due to the usage in JenkinsRule.waitForCompletion.

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 2, 2024

Choose a reason for hiding this comment

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

(And as far as I know the states for a Run can only move forward, although looking at the code I guess a subclass that calls some of the protected methods incorrectly could cause random state changes.)

Copy link
Member

@jglick jglick Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

as far as I know the states for a Run can only move forward

That looks right.

like the check for the UI in Jelly

Not sure what you mean.

jenkins/core/src/main/resources/hudson/model/Run/delete.jelly

Line 30 in 2082381

<j:if test="${!it.building and !it.keepLog}">
is only checking building and ought to be switched to only check logUpdated.

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 2, 2024

Choose a reason for hiding this comment

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

🤦 I misread keepLog as logUpdated. I'll update that.

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 2, 2024

Choose a reason for hiding this comment

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

I fixed that in f676be5.

LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s still building", r);
return true;
}
Expand Down
13 changes: 12 additions & 1 deletion core/src/main/java/jenkins/model/BackgroundGlobalBuildDiscarder.java
View file
Open in desktop
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
View file
Open in desktop
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.
Copy link
Member Author

@dwnusbaum dwnusbaum Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bit confusing, but essentially the old behavior was that Run.run called Job.logRotate while the build was in POST_PRODUCTION state, but also ever since #4368 if the configuration of GlobalBuildDiscarderConfiguration included JobGlobalBuildDiscarderStrategy (which it does by default), we also called Job.logRotate here while the build was in COMPLETED state. For backwards compatibility I think we need to ensure it gets called at least once regardless of the global configuration, and it's preferable to call it here in onFinalized so that we can check Run.isLogUpdated in LogRotator.shouldKeepRun and Run.delete to avoid race conditions with Pipeline builds. We avoid the redundant call by filtering out the relevant strategy when processing the globally-configured discarders.

Also, the old behavior is why I am not concerned about removing this call in workflow-job and also why I am not worried about making the above call asynchronous for the sake of Pipeline builds. BackgroundGlobalBuildDiscarder.processJob has been (redundantly) calling logRotate synchronously for years now in default configurations, so it seems that the asynchronous behavior in jenkinsci/workflow-job-plugin@63fdbe8 is no longer critical.

Copy link
Member

@jglick jglick Oct 2, 2024

Choose a reason for hiding this comment

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

(jenkinsci/workflow-job-plugin#70 for better linking)

Copy link

@IppX IppX Feb 3, 2025

Choose a reason for hiding this comment

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

Hi, after some digging into the behavior of globalBuildDiscarder I ended here.

As a Jenkins admin, I expected the specific (or simple) global discarder to take precedence over any project build discarder, especially if I remove the global project build discarder. This is also what I implied from reading the descriptions here and here (even though it's not explicitly mentioned there and does not contradict the actual behavior). I also found multiple occurrences on SO and forums mentioning that global specific job discarder will take precedence, which is not the case. This is now obvious with the change line 51 here but was already the behavior before as I tested with 2.462.2.

What makes it even more confusing, is the way the discarders are merged and applied. The most aggressive policy takes over. This is confirmed to be an expected behavior from the tests. However, from an admin perspective I'd see the least aggressive discarder to be a safer approach (e.g. for compliance reasons).

I'm curious however, to get your input on the topic. Also let me know if I should raise this somewhere else.

For context, this investigation started as a result of the security eng. telling me that he can erase his traces by overriding build discarder in a job. Example: normal run -> evil run (set buildDiscarder to 1) -> normal run. Then we can't see what happened in the evil run anymore. We also have some compliance policy that require us to keep production pipelines history up to a year which we would like to enforce.

Copy link
Member

@jglick jglick Feb 10, 2025

Choose a reason for hiding this comment

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

I think the current behavior is as designed; build discarders are mainly meant to trim disk space, so it would be normal to have a blanket policy that builds over a month old are not kept, while a particular job that runs every five minutes for some automation is configured to discard all but the last build since history is trash. If your interest is in auditing, you would better use one of the many plugins which stream events or even whole build logs from Jenkins to external systems, which could be configured to only accept “create/append”-type operations and reject attempts to delete anything even if the Jenkins controller were to be compromised somehow. After all, even without any discarders, your white-hat engineer could simply delete the job after one build, along with all of its builds. You could also make the entire controller view-only to regular developers, using various “as code” systems.

Copy link
Member

@daniel-beck daniel-beck Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@IppX

This is also what I implied from reading the descriptions here and here (even though it's not explicitly mentioned there and does not contradict the actual behavior). I also found multiple occurrences on SO and forums mentioning that global specific job discarder will take precedence, which is not the case. … The most aggressive policy takes over

In addition to what Jesse wrote, for global build discarders, this seems to be reflected in the difference between "Default Build Discarder" and "Specific Build Discarder" for Global Build Discarders. Compare

Build discarder will be applied for jobs without a specific build discarder, after any build finishes. This makes it possible to set a sensible default and override the discarder for specific jobs, that needs a longer retention policy.

and

The selected build discarder will be applied after any build finishes, as well as periodically.

"Precedence" perhaps in the sense that, if it's more restrictive (less retention time/fewer retained builds) you end up keeping fewer builds (so incorrect generally, but correct for specific such examples).

BackgroundGlobalBuildDiscarder.processJob(new LogTaskListener(LOGGER, Level.FINE), job,
GlobalBuildDiscarderConfiguration.get().getConfiguredBuildDiscarders().stream()
.filter(s -> !(s instanceof JobGlobalBuildDiscarderStrategy)));
}
}
13 changes: 13 additions & 0 deletions test/src/test/java/hudson/model/RunTest.java
View file
Open in desktop
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,17 @@ public class RunTest {
assertTrue(Mgr.deleted.get());
}

@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
View file
Open in desktop
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 {
Copy link
Member Author

@dwnusbaum dwnusbaum Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, this was to check that the switch to isLogUpdated in LogRotator.shouldKeepRun did not break anything, since previously the call in Run.run would not have been able to delete the build with the switch. Because of the redundant call via GlobalBuildDiscarderListener though it worked anyway, the only difference was a log warning from the deleted code in Run.run, which we can't assert against any more. So maybe this test is no longer checking anything useful.

Copy link
Member

@jglick jglick Oct 2, 2024

Choose a reason for hiding this comment

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

I guess, in principle, someone could want to keep zero builds if they were using publishers (GitHub Checks, Slack, etc.) to track failures?

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));
Copy link
Member

@jglick jglick Oct 28, 2024

Choose a reason for hiding this comment

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

Failure in https://github.com/jenkinsci/jenkins/pull/9921/checks?check_run_id=32166988156 (#9921) looks like a flake? Missing await?

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 28, 2024

Choose a reason for hiding this comment

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

I don't know what we would await for, because log rotation runs synchronously during build completion. Unless this is a test-specific timing issue due to JenkinsRule.buildAndAssertSuccess just waiting for QueueTaskFuture.get instead of !Run.isLogUpdated, then this test probably just needs to be skipped on Windows.

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 28, 2024

Choose a reason for hiding this comment

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

From some testing, j.buildAndAssertStatus(Result.SUCCESS, p) is guaranteed to block until GlobalBuildDiscarderListener.onFinalized has completed. Perhaps the BuildWatcher background thread tried to copy the logs at an inopportune time and caused log rotation to fail. Seems best to delete the BuildWatcher here in case it made any of the other tests flaky on Windows and then probably skip this test explicitly on Windows as well.

Copy link
Member Author

@dwnusbaum dwnusbaum Oct 28, 2024

Choose a reason for hiding this comment

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

Also the AccessDeniedException seems strange. Either way, I filed #9923.

}

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