Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions core/src/main/java/hudson/model/Run.java
Original file line number Diff line number Diff line change
Expand Up @@ -1541,6 +1541,20 @@ public ACL getACL() {
return getParent().getACL();
}

/**
* Deletes this build's entire log, but keeps build
*
* @throws IOException
* if we fail to delete.
*/
public void deleteLog() throws IOException {
File rootDir = getRootDir();
LOGGER.log(FINE, "{0}: {1} logs successfully deleted", new Object[] { this, rootDir });
synchronized (this) {
Util.deleteFile(getLogFile());
Copy link
Member

Choose a reason for hiding this comment

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

It makes an assumption that the logs are stored on the disk.
Some plugins like Logstash plugin do not actually do that, especially if there is External Logging implementation like jenkinsci/logstash-plugin#18 in place

Not a big problem for this PR as long as the method is overridable.
But it would great to ensure it does not blow up if the log file does not exist

}
}

/**
* Deletes this build's artifacts.
*
Expand Down
129 changes: 87 additions & 42 deletions core/src/main/java/hudson/tasks/LogRotator.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
import java.util.List;
import java.util.logging.Logger;

import static java.util.logging.Level.*;
import static java.util.logging.Level.FINE;
import static java.util.logging.Level.FINER;

/**
* Default implementation of {@link BuildDiscarder}.
Expand Down Expand Up @@ -76,10 +77,25 @@ public class LogRotator extends BuildDiscarder {
*/
private final Integer artifactNumToKeep;

/**
* If not -1 nor null, build logs are only kept up to this days.
* Null handling is necessary to remain data compatible with old versions.
* @since FIXME
*/
private final Integer logDaysToKeep;

/**
* If not -1, only this number of build logs are kept.
* Null handling is necessary to remain data compatible with old versions.
* @since FIXME
*/
private final Integer logNumToKeep;

@DataBoundConstructor
public LogRotator (String daysToKeepStr, String numToKeepStr, String artifactDaysToKeepStr, String artifactNumToKeepStr) {
public LogRotator (String daysToKeepStr, String numToKeepStr, String artifactDaysToKeepStr, String artifactNumToKeepStr, String logDaysToKeepStr, String logNumToKeepStr) {
Copy link
Member

Choose a reason for hiding this comment

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

🐛 this method is a part of public API, so this change breaks binary compatibility. The original method should be retained OR you could use @DataBoundSetters

this (parse(daysToKeepStr),parse(numToKeepStr),
parse(artifactDaysToKeepStr),parse(artifactNumToKeepStr));
parse(artifactDaysToKeepStr),parse(artifactNumToKeepStr),
parse(logDaysToKeepStr),parse(logNumToKeepStr));
}

public static int parse(String p) {
Expand All @@ -93,30 +109,61 @@ public static int parse(String p) {

/**
* @deprecated since 1.350.
* Use {@link #LogRotator(int, int, int, int)}
* Use {@link #LogRotator(int, int, int, int, int, int)}
*/
@Deprecated
public LogRotator(int daysToKeep, int numToKeep) {
this(daysToKeep, numToKeep, -1, -1);
this(daysToKeep, numToKeep, -1, -1, -1, -1);
}

/**
* @deprecated since FIXME
* Use {@link #LogRotator(int, int, int, int, int, int)}
*/
@Deprecated
public LogRotator(int daysToKeep, int numToKeep, int artifactDaysToKeep, int artifactNumToKeep) {
this(daysToKeep, numToKeep, artifactDaysToKeep, artifactNumToKeep, -1, -1);
}

public LogRotator(int daysToKeep, int numToKeep, int artifactDaysToKeep, int artifactNumToKeep, int logDaysToKeep, int logNumToKeep) {
this.daysToKeep = daysToKeep;
this.numToKeep = numToKeep;
this.artifactDaysToKeep = artifactDaysToKeep;
this.artifactNumToKeep = artifactNumToKeep;

this.logDaysToKeep = logDaysToKeep;
this.logNumToKeep = logNumToKeep;

}

@SuppressWarnings("rawtypes")
public void perform(Job<?,?> job) throws IOException, InterruptedException {
LOGGER.log(FINE, "Running the log rotation for {0} with numToKeep={1} daysToKeep={2} artifactNumToKeep={3} artifactDaysToKeep={4}", new Object[] {job, numToKeep, daysToKeep, artifactNumToKeep, artifactDaysToKeep});

LOGGER.log(FINE, "Running the log rotation for {0} with numToKeep={1} daysToKeep={2} artifactNumToKeep={3} " +
"artifactDaysToKeep={4} logNumToKeep={5} logDaysToKeep={6}", new Object[] {job, numToKeep, daysToKeep,
artifactNumToKeep, artifactDaysToKeep, logNumToKeep, logDaysToKeep});

performDeletionOf("build", job, numToKeep, daysToKeep, Run::delete);
performDeletionOf("artifacts", job, artifactNumToKeep, artifactDaysToKeep, Run::deleteArtifacts);
performDeletionOf("log", job, logNumToKeep, logDaysToKeep, Run::deleteLog);
}

/**
* Deletes build metadata based on the parameters passed.
*
* Each build or build metadata have different delete functions as specified in {@link Run}
* @param buildMetaData String indicating whether builds, artifacts, or logs are being deleted (used for logging)
* @param job the Jenkins job whose builds and/or metadata are to be deleted
* @param numToKeep the number of the most recent builds to keep
* @param daysToKeep the number of days to keep the most recent builds up until
* @param runAction the deletion function passed
* @throws IOException if fail to delete
* @since FIXME
*/
private void performDeletionOf(String buildMetaData, Job<?,?> job, Integer numToKeep, Integer daysToKeep,
RunAction runAction) throws IOException{
// always keep the last successful and the last stable builds
Run lsb = job.getLastSuccessfulBuild();
Run lstb = job.getLastStableBuild();

if(numToKeep!=-1) {
if(numToKeep!=null && numToKeep!=-1) {
// Note that RunList.size is deprecated, and indeed here we are loading all the builds of the job.
// However we would need to load the first numToKeep anyway, just to skip over them;
// and we would need to load the rest anyway, to delete them.
Expand All @@ -127,12 +174,12 @@ public void perform(Job<?,?> job) throws IOException, InterruptedException {
if (shouldKeepRun(r, lsb, lstb)) {
continue;
}
LOGGER.log(FINE, "{0} is to be removed", r);
r.delete();
LOGGER.log(FINE, "{0}'s {1} to be removed", new Object[] {r, buildMetaData});
runAction.call(r);
}
}

if(daysToKeep!=-1) {
if(daysToKeep!= null && daysToKeep!=-1) {
Calendar cal = new GregorianCalendar();
cal.add(Calendar.DAY_OF_YEAR,-daysToKeep);
Run r = job.getFirstBuild();
Expand All @@ -141,35 +188,8 @@ public void perform(Job<?,?> job) throws IOException, InterruptedException {
break;
}
if (!shouldKeepRun(r, lsb, lstb)) {
LOGGER.log(FINE, "{0} is to be removed", r);
r.delete();
}
r = r.getNextBuild();
}
}

if(artifactNumToKeep!=null && artifactNumToKeep!=-1) {
List<? extends Run<?,?>> builds = job.getBuilds();
for (Run r : copy(builds.subList(Math.min(builds.size(), artifactNumToKeep), builds.size()))) {
if (shouldKeepRun(r, lsb, lstb)) {
continue;
}
LOGGER.log(FINE, "{0} is to be purged of artifacts", r);
r.deleteArtifacts();
}
}

if(artifactDaysToKeep!=null && artifactDaysToKeep!=-1) {
Calendar cal = new GregorianCalendar();
cal.add(Calendar.DAY_OF_YEAR,-artifactDaysToKeep);
Run r = job.getFirstBuild();
while (r != null) {
if (tooNew(r, cal)) {
break;
}
if (!shouldKeepRun(r, lsb, lstb)) {
LOGGER.log(FINE, "{0} is to be purged of artifacts", r);
r.deleteArtifacts();
LOGGER.log(FINE, "{0}'s {1} to be removed", new Object[] {r, buildMetaData});
runAction.call(r);
}
r = r.getNextBuild();
}
Expand Down Expand Up @@ -228,6 +248,14 @@ public int getArtifactNumToKeep() {
return unbox(artifactNumToKeep);
}

public int getLogDaysToKeep() {
return unbox(logDaysToKeep);
}

public int getLogNumToKeep() {
return unbox(logNumToKeep);
}

public String getDaysToKeepStr() {
return toString(daysToKeep);
}
Expand All @@ -244,6 +272,14 @@ public String getArtifactNumToKeepStr() {
return toString(artifactNumToKeep);
}

public String getLogDaysToKeepStr() {
return toString(logDaysToKeep);
}

public String getLogNumToKeepStr() {
return toString(logNumToKeep);
}

private int unbox(Integer i) {
return i==null ? -1: i;
}
Expand All @@ -253,6 +289,15 @@ private String toString(Integer i) {
return String.valueOf(i);
}

/**
* Interface to allow for lambda functions to run a call on a given run.
* @since FIXME
*/
@FunctionalInterface
interface RunAction {
void call(Run<?,?> r) throws IOException;
}

@Extension @Symbol("logRotator")
public static final class LRDescriptor extends BuildDiscarderDescriptor {
public String getDisplayName() {
Expand Down
64 changes: 60 additions & 4 deletions test/src/test/java/hudson/tasks/LogRotatorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.TestBuilder;
import org.jvnet.hudson.test.UnstableBuilder;

/**
* Verifies that the last successful and stable builds of a job will be kept if requested.
Expand All @@ -67,7 +68,7 @@ public class LogRotatorTest {
@Test
public void successVsFailure() throws Exception {
FreeStyleProject project = j.createFreeStyleProject();
project.setLogRotator(new LogRotator(-1, 2, -1, -1));
project.setBuildDiscarder(new LogRotator(-1, 2, -1, -1, -1, -1));
assertEquals(Result.SUCCESS, build(project)); // #1
project.getBuildersList().replaceBy(Collections.singleton(new FailureBuilder()));
assertEquals(Result.FAILURE, build(project)); // #2
Expand All @@ -85,7 +86,7 @@ public void successVsFailure() throws Exception {
@Issue("JENKINS-2417")
public void stableVsUnstable() throws Exception {
FreeStyleProject project = j.createFreeStyleProject();
project.setLogRotator(new LogRotator(-1, 2, -1, -1));
project.setBuildDiscarder(new LogRotator(-1, 2, -1, -1, -1, -1));
assertEquals(Result.SUCCESS, build(project)); // #1
project.getPublishersList().replaceBy(Collections.singleton(new TestsFail()));
assertEquals(Result.UNSTABLE, build(project)); // #2
Expand All @@ -101,7 +102,7 @@ public void stableVsUnstable() throws Exception {
@Issue("JENKINS-834")
public void artifactDelete() throws Exception {
FreeStyleProject project = j.createFreeStyleProject();
project.setLogRotator(new LogRotator(-1, 6, -1, 2));
project.setBuildDiscarder(new LogRotator(-1, 6, -1, 2, -1, -1));
project.getPublishersList().replaceBy(Collections.singleton(new ArtifactArchiver("f", "", true, false)));
assertEquals("(no artifacts)", Result.FAILURE, build(project)); // #1
assertFalse(project.getBuildByNumber(1).getHasArtifacts());
Expand Down Expand Up @@ -153,7 +154,7 @@ public void artifactDelete() throws Exception {
public void artifactsRetainedWhileBuilding() throws Exception {
j.getInstance().setNumExecutors(3);
FreeStyleProject p = j.createFreeStyleProject();
p.setBuildDiscarder(new LogRotator(-1, 3, -1, 1));
p.setBuildDiscarder(new LogRotator(-1, 3, -1, 1, -1, -1));
StallBuilder sync = new StallBuilder();
p.getBuildersList().replaceBy(Arrays.asList(new CreateArtifact(), sync));
p.setConcurrentBuild(true);
Expand Down Expand Up @@ -201,6 +202,61 @@ public void artifactsRetainedWhileBuilding() throws Exception {
assertThat("we have artifacts in run3", run3.getHasArtifacts(), is(true));
}

@Test
@Issue("JENKINS-51229")
public void logDelete() throws Exception
{
FreeStyleProject project = j.createFreeStyleProject();
project.setBuildDiscarder(new LogRotator(-1, -1, -1, -1, -1, 2));
assertEquals(Result.SUCCESS, build(project)); // #1
project.getBuildersList().replaceBy(Collections.singleton(new FailureBuilder()));
assertEquals(Result.FAILURE, build(project)); // #2
assertEquals(Result.FAILURE, build(project)); // #3
assertEquals(1, numberOf(project.getLastSuccessfulBuild()));
project.getBuildersList().replaceBy(Collections.<Builder> emptySet());
assertEquals(Result.SUCCESS, build(project)); // #4
assertFalse(project.getBuildByNumber(1).getLogFile().isFile());
assertFalse(project.getBuildByNumber(2).getLogFile().isFile());
assertTrue(project.getBuildByNumber(3).getLogFile().isFile());
assertTrue(project.getBuildByNumber(4).getLogFile().isFile());
}

@Test
public void keepLogForMarkKeeped() throws Exception
{
FreeStyleProject project = j.createFreeStyleProject();
project.setBuildDiscarder(new LogRotator(-1, -1, -1, -1, -1, 2));
build(project); // #1
project.getBuildByNumber(1).keepLog();
build(project); // #2
assertTrue(project.getBuildByNumber(1).getLogFile().isFile());
build(project); // #3
build(project); // #4
assertTrue(project.getBuildByNumber(1).getLogFile().isFile());
assertFalse(project.getBuildByNumber(2).getLogFile().isFile());
assertTrue(project.getBuildByNumber(3).getLogFile().isFile());
assertTrue(project.getBuildByNumber(4).getLogFile().isFile());
}

@Test
public void keepLogForLastStableBuild() throws Exception
{
FreeStyleProject project = j.createFreeStyleProject();
project.setBuildDiscarder(new LogRotator(-1, -1, -1, -1, -1, 2));
build(project); // #1
project.getBuildersList().replaceBy(Collections.singleton(new UnstableBuilder()));
build(project); // #2
build(project); // #3
assertTrue(project.getBuildByNumber(1).getLogFile().isFile());
assertTrue(project.getBuildByNumber(2).getLogFile().isFile());
assertTrue(project.getBuildByNumber(3).getLogFile().isFile());
build(project); // #4
assertTrue(project.getBuildByNumber(1).getLogFile().isFile());
assertFalse(project.getBuildByNumber(2).getLogFile().isFile());
assertTrue(project.getBuildByNumber(3).getLogFile().isFile());
assertTrue(project.getBuildByNumber(4).getLogFile().isFile());
}

static Result build(FreeStyleProject project) throws Exception {
return project.scheduleBuild2(0).get().getResult();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,5 +91,7 @@ private static void verifyLogRotatorSanity(AbstractProject<?,?> p) {
assertEquals(3, d.getNumToKeep());
assertEquals(2, d.getArtifactDaysToKeep());
assertEquals(1, d.getArtifactNumToKeep());
assertEquals(6, d.getLogDaysToKeep());
assertEquals(5, d.getLogNumToKeep());
}
}
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
<numToKeep>3</numToKeep>
<artifactDaysToKeep>2</artifactDaysToKeep>
<artifactNumToKeep>1</artifactNumToKeep>
<logDaysToKeep>6</logDaysToKeep>
<logNumToKeep>5</logNumToKeep>
</logRotator>
<keepDependencies>false</keepDependencies>
<properties/>
Expand Down