Skip to content

Commit

Permalink
[JENKINS-65829] Fix WorkspaceCleanupThread to consider suffixed works… (
Browse files Browse the repository at this point in the history 
#9083)

* [JENKINS-65829] Fix WorkspaceCleanupThread to consider suffixed workspaces even if original is inexistent

* [JENKINS-65829] Make sur controller system properties is used across all nodes

* [JENKINS-65829] Adapt test for tmp + rename test for libs
  • Loading branch information
@Dohbedoh
Dohbedoh authored Apr 14, 2024
1 parent e9adbc9 commit 323ad6a
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 19 deletions.
89 changes: 70 additions & 19 deletions core/src/main/java/hudson/model/WorkspaceCleanupThread.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,28 @@

package hudson.model;

import static hudson.Util.fileToPath;

import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.FilePath;
import hudson.Functions;
import hudson.Util;
import hudson.remoting.VirtualChannel;
import hudson.slaves.WorkspaceList;
import java.io.File;
import java.io.FileFilter;
import java.io.IOException;
import java.io.Serializable;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.MasterToSlaveFileCallable;
import jenkins.model.Jenkins;
import jenkins.model.ModifiableTopLevelItemGroup;
import jenkins.util.SystemProperties;
Expand Down Expand Up @@ -90,8 +99,7 @@ public static void invoke() {
if (check) {
listener.getLogger().println("Deleting " + ws + " on " + node.getDisplayName());
try {
ws.deleteSuffixesRecursive();
ws.deleteRecursive();
ws.act(new CleanupOldWorkspaces(retainForDays));
} catch (IOException | InterruptedException x) {
Functions.printStackTrace(x, listener.error("Failed to delete " + ws + " on " + node.getDisplayName()));
}
Expand All @@ -101,21 +109,6 @@ public static void invoke() {
}

private boolean shouldBeDeleted(@NonNull TopLevelItem item, FilePath dir, @NonNull Node n) throws IOException, InterruptedException {
// TODO: the use of remoting is not optimal.
// One remoting can execute "exists", "lastModified", and "delete" all at once.
// (Could even invert master loop so that one FileCallable takes care of all known items.)
if (!dir.exists()) {
LOGGER.log(Level.FINE, "Directory {0} does not exist", dir);
return false;
}

// if younger than a month, keep it
long now = new Date().getTime();
if (dir.lastModified() + retainForDays * DAY > now) {
LOGGER.log(Level.FINE, "Directory {0} is only {1} old, so not deleting", new Object[] {dir, Util.getTimeSpanString(now - dir.lastModified())});
return false;
}

// TODO could also be good to add checkbox that lets users configure a workspace to never be auto-cleaned.

// TODO check instead for SCMTriggerItem:
Expand Down Expand Up @@ -143,11 +136,69 @@ private boolean shouldBeDeleted(@NonNull TopLevelItem item, FilePath dir, @NonNu
return false;
}
}

LOGGER.log(Level.FINER, "Going to delete directory {0}", dir);
return true;
}

private static class CleanupOldWorkspaces extends MasterToSlaveFileCallable<Void> {

private final int retentionInDays;

public CleanupOldWorkspaces(int retentionInDays) {
this.retentionInDays = retentionInDays;
}

@Override
public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
File[] workspaces = null;
File parentWs = f.getParentFile();
if (parentWs != null) {
workspaces = parentWs.listFiles(new ShouldBeDeletedFilter(this.retentionInDays, f.getName()));
}

if (workspaces != null) {
for (File workspace : workspaces) {
LOGGER.log(Level.FINER, "Going to delete directory {0}", workspace);
Util.deleteRecursive(fileToPath(workspace), Path::toFile);
}
}
return null;
}
}

private static class ShouldBeDeletedFilter implements FileFilter, Serializable {

private final int retentionInDays;

private final String workspaceBaseName;

public ShouldBeDeletedFilter(int retentionInDays, String workspaceBaseName) {
this.retentionInDays = retentionInDays;
this.workspaceBaseName = workspaceBaseName;
}

@Override
public boolean accept(File dir) {

if (!dir.isDirectory()) {
return false;
}

// if not the workspace or a workspace suffix
if (!dir.getName().equals(workspaceBaseName) && !dir.getName().startsWith(workspaceBaseName + WorkspaceList.COMBINATOR)) {
return false;
}

// if younger than a month, keep it
long now = new Date().getTime();
if (dir.lastModified() + this.retentionInDays * DAY > now) {
LOGGER.log(Level.FINE, "Directory {0} is only {1} old, so not deleting", new Object[] {dir, Util.getTimeSpanString(now - dir.lastModified())});
return false;
}

return true;
}
}

private static final Logger LOGGER = Logger.getLogger(WorkspaceCleanupThread.class.getName());

/**
Expand Down
24 changes: 24 additions & 0 deletions test/src/test/java/hudson/model/WorkspaceCleanupThreadTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import jenkins.MasterToSlaveFileCallable;
import jenkins.model.Jenkins;
import org.junit.Assume;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -181,12 +182,28 @@ public void deleteTemporaryDirectory() throws Exception {
FilePath ws = createOldWorkspaceOn(r.jenkins, p);
FilePath tmp = WorkspaceList.tempDir(ws);
tmp.child("stuff").write("content", null);
tmp.act(new Touch(0));
createOldWorkspaceOn(r.createOnlineSlave(), p);
performCleanup();
assertFalse(ws.exists());
assertFalse("temporary directory should be cleaned up as well", tmp.exists());
}

@Issue("JENKINS-65829")
@Test
public void deleteSoleLibsDirectory() throws Exception {
FreeStyleProject p = r.createFreeStyleProject();
FilePath jobWs = Jenkins.get().getWorkspaceFor(p);
FilePath libsWs = jobWs.withSuffix(WorkspaceList.COMBINATOR + "libs");
libsWs.child("test-libs").write("content", null);
libsWs.act(new Touch(0));
assertFalse(jobWs.exists());
assertTrue(libsWs.exists());
performCleanup();
assertFalse(jobWs.exists());
assertFalse("libs directory should be cleaned up as well", libsWs.exists());
}

private FilePath createOldWorkspaceOn(Node slave, FreeStyleProject p) throws Exception {
p.setAssignedNode(slave);
FreeStyleBuild b1 = r.buildAndAssertSuccess(p);
Expand All @@ -197,6 +214,13 @@ private FilePath createOldWorkspaceOn(Node slave, FreeStyleProject p) throws Exc
return ws;
}

private FilePath createOldLibsWorkspace(FreeStyleProject p) throws IOException, InterruptedException {
FilePath libsWs = Jenkins.get().getWorkspaceFor(p).withSuffix(WorkspaceList.COMBINATOR + "libs");
libsWs.child("test-libs").write("content", null);
libsWs.act(new Touch(0));
return libsWs;
}

private void performCleanup() throws InterruptedException, IOException {
new WorkspaceCleanupThread().execute(StreamTaskListener.fromStdout());
}
Expand Down

0 comments on commit 323ad6a

Please sign in to comment.