[FIXED JENKINS-21023] Rewrote WorkspaceCleanupThread to determine where...

[FIXED JENKINS-21023] Rewrote WorkspaceCleanupThread to determine where workspaces for known items should be, rather than looking for directories that look like they might be workspaces.
One key advantage is that this correctly handles the new default workspace location on the master.
Another is that this allows folders to be correctly skipped, and jobs in folders to be correctly deleted.
A subtler advantage would be handling of configured nondefault workspace locations, and compatibility with WorkspaceLocator.
(Orphaned workspaces from deleted jobs are not removed, but these were not removed before either.)

changelog.html

@@ -67,6 +67,9 @@ Upcoming changes</a>
   <li class=bug>
     f:combobox is narrow.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-21612">issue 21612</a>)
+  <li class=bug>
+    The workspace cleanup thread failed to handle the modern workspace location on master, and mishandled folders.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-21023">issue 21023</a>)
   <li class=bug>
     Fixed missing help items on "Configure Global Security" page
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-19832">issue 19832</a>)

core/src/main/java/hudson/model/WorkspaceCleanupThread.java

@@ -23,18 +23,18 @@
  */
 package hudson.model;
 
+import hudson.Extension;
 import hudson.FilePath;
 import hudson.Util;
-import hudson.Extension;
-import jenkins.model.Jenkins;
-
-import java.io.File;
-import java.io.FileFilter;
 import java.io.IOException;
-import java.io.Serializable;
+import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
+import java.util.logging.Level;
 import java.util.logging.Logger;
+import javax.annotation.Nonnull;
+import jenkins.model.Jenkins;
+import jenkins.model.ModifiableTopLevelItemGroup;
 
 /**
  * Clean up old left-over workspaces from slaves.
@@ -47,7 +47,7 @@ public class WorkspaceCleanupThread extends AsyncPeriodicWork {
         super("Workspace clean-up");
     }
 
-    public long getRecurrencePeriod() {
+    @Override public long getRecurrencePeriod() {
         return DAY;
     }
 
@@ -55,121 +55,87 @@ public class WorkspaceCleanupThread extends AsyncPeriodicWork {
         Jenkins.getInstance().getExtensionList(AsyncPeriodicWork.class).get(WorkspaceCleanupThread.class).run();
     }
 
-    // so that this can be easily accessed from sub-routine.
-    private TaskListener listener;
-
-    protected void execute(TaskListener listener) throws InterruptedException, IOException {
-        try {
-            if(disabled) {
-                LOGGER.fine("Disabled. Skipping execution");
-                return;
-            }
-            
-            this.listener = listener;
-
-            Jenkins h = Jenkins.getInstance();
-            for (Node n : h.getNodes())
-                if (n instanceof Slave) process((Slave)n);
-
-            process(h);
-        } finally {
-            this.listener = null;
+    @Override protected void execute(TaskListener listener) throws InterruptedException, IOException {
+        if (disabled) {
+            LOGGER.fine("Disabled. Skipping execution");
+            return;
         }
-    }
-
-    private void process(Jenkins h) throws IOException, InterruptedException {
-        File jobs = new File(h.getRootDir(), "jobs");
-        File[] dirs = jobs.listFiles(DIR_FILTER);
-        if(dirs==null)      return;
-        for (File dir : dirs) {
-            FilePath ws = new FilePath(new File(dir, "workspace"));
-            if(shouldBeDeleted(dir.getName(),ws,h)) {
-                delete(ws);
+        List<Node> nodes = new ArrayList<Node>();
+        Jenkins j = Jenkins.getInstance();
+        nodes.add(j);
+        nodes.addAll(j.getNodes());
+        for (TopLevelItem item : j.getAllItems(TopLevelItem.class)) {
+            if (item instanceof ModifiableTopLevelItemGroup) { // no such thing as TopLevelItemGroup, and ItemGroup offers no access to its type parameter
+                continue; // children will typically have their own workspaces as subdirectories; probably no real workspace of its own
+            }
+            listener.getLogger().println("Checking " + item.getFullDisplayName());
+            for (Node node : nodes) {
+                FilePath ws = node.getWorkspaceFor(item);
+                if (ws == null) {
+                    continue; // offline, fine
+                }
+                boolean check;
+                try {
+                    check = shouldBeDeleted(item, ws, node);
+                } catch (IOException x) {
+                    x.printStackTrace(listener.error("Failed to check " + node.getDisplayName()));
+                    continue;
+                } catch (InterruptedException x) {
+                    x.printStackTrace(listener.error("Failed to check " + node.getDisplayName()));
+                    continue;
+                }
+                if (check) {
+                    listener.getLogger().println("Deleting " + ws + " on " + node.getDisplayName());
+                    try {
+                        ws.deleteRecursive();
+                    } catch (IOException x) {
+                        x.printStackTrace(listener.error("Failed to delete " + ws + " on " + node.getDisplayName()));
+                    } catch (InterruptedException x) {
+                        x.printStackTrace(listener.error("Failed to delete " + ws + " on " + node.getDisplayName()));
+                    }
+                }
             }
         }
     }
 
-    private boolean shouldBeDeleted(String workspaceDirectoryName, FilePath dir, Node n) throws IOException, InterruptedException {
+    private boolean shouldBeDeleted(@Nonnull TopLevelItem item, FilePath dir, Node n) throws IOException, InterruptedException {
         // TODO: the use of remoting is not optimal.
         // One remoting can execute "exists", "lastModified", and "delete" all at once.
-        TopLevelItem item = Jenkins.getInstance().getItem(workspaceDirectoryName);
-
-        if(!dir.exists())
+        // (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() + 30 * DAY > now) {
-            LOGGER.fine("Directory "+dir+" is only "+ Util.getTimeSpanString(now-dir.lastModified())+" old, so not deleting");
+            LOGGER.log(Level.FINE, "Directory {0} is only {1} old, so not deleting", new Object[] {dir, Util.getTimeSpanString(now-dir.lastModified())});
             return false;
         }
 
-        // Could mean that directory doesn't belong to a job. But can also mean that it's a custom workspace belonging to a job.
-        // So better leave it alone - will still be deleted after 30 days - until we have a proper check for custom workspaces.
-        // TODO: implement proper check for custom workspaces.
-        // TODO: If we do the above, could also be good to add checkbox that lets users configure a workspace to never be auto-cleaned.
-        if(item==null) {
-            return false;
-        }
+        // TODO could also be good to add checkbox that lets users configure a workspace to never be auto-cleaned.
 
         if (item instanceof AbstractProject<?,?>) {
             AbstractProject<?,?> p = (AbstractProject<?,?>) item;
             Node lb = p.getLastBuiltOn();
-            LOGGER.finer("Directory "+dir+" is last built on "+lb);
+            LOGGER.log(Level.FINER, "Directory {0} is last built on {1}", new Object[] {dir, lb});
             if(lb!=null && lb.equals(n)) {
                 // this is the active workspace. keep it.
-                LOGGER.fine("Directory "+dir+" is the last workspace for "+p);
+                LOGGER.log(Level.FINE, "Directory {0} is the last workspace for {1}", new Object[] {dir, p});
                 return false;
             }
             
             if(!p.getScm().processWorkspaceBeforeDeletion(p,dir,n)) {
-                LOGGER.fine("Directory deletion of "+dir+" is vetoed by SCM");
+                LOGGER.log(Level.FINE, "Directory deletion of {0} is vetoed by SCM", dir);
                 return false;
             }
         }
 
-        LOGGER.finer("Going to delete directory "+dir);
+        LOGGER.log(Level.FINER, "Going to delete directory {0}", dir);
         return true;
     }
 
-    private void process(Slave s) throws InterruptedException {
-        listener.getLogger().println("Scanning "+s.getNodeName());
-
-        try {
-            FilePath path = s.getWorkspaceRoot();
-            if(path==null)  return;
-
-            List<FilePath> dirs = path.list(DIR_FILTER);
-            if(dirs ==null) return;
-            for (FilePath dir : dirs) {
-                if(shouldBeDeleted(dir.getName(),dir,s))
-                    delete(dir);
-            }
-        } catch (IOException e) {
-            e.printStackTrace(listener.error("Failed on "+s.getNodeName()));
-        }
-    }
-
-    private void delete(FilePath dir) throws InterruptedException {
-        try {
-            listener.getLogger().println("Deleting "+dir);
-            dir.deleteRecursive();
-        } catch (IOException e) {
-            e.printStackTrace(listener.error("Failed to delete "+dir));
-        }
-    }
-
-
-    private static class DirectoryFilter implements FileFilter, Serializable {
-        public boolean accept(File f) {
-            return f.isDirectory();
-        }
-        private static final long serialVersionUID = 1L;
-    }
-    private static final FileFilter DIR_FILTER = new DirectoryFilter();
-
-    private static final long DAY = 1000*60*60*24;
-
     private static final Logger LOGGER = Logger.getLogger(WorkspaceCleanupThread.class.getName());
 
     /**

test/src/test/java/hudson/model/WorkspaceCleanupThreadTest.java

+/*
+ * The MIT License
+ *
+ * Copyright 2014 Jesse Glick.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package hudson.model;
+
+import hudson.FilePath;
+import hudson.FilePath.FileCallable;
+import hudson.remoting.VirtualChannel;
+import hudson.slaves.DumbSlave;
+import hudson.util.StreamTaskListener;
+import java.io.File;
+import java.io.IOException;
+import java.util.logging.ConsoleHandler;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import static org.junit.Assert.*;
+import org.junit.Assume;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockFolder;
+
+public class WorkspaceCleanupThreadTest {
+
+    // TODO test that new workspaces are skipped
+    // TODO test that SCM.processWorkspaceBeforeDeletion can reject
+
+    @Rule public JenkinsRule r = new JenkinsRule();
+
+    private static final Logger logger = Logger.getLogger(WorkspaceCleanupThread.class.getName());
+    @BeforeClass public static void logging() {
+        logger.setLevel(Level.ALL);
+        Handler handler = new ConsoleHandler();
+        handler.setLevel(Level.ALL);
+        logger.addHandler(handler);
+    }
+    
+    @Test public void cleanUpSlaves() throws Exception {
+        DumbSlave s1 = r.createOnlineSlave();
+        FreeStyleProject p = r.createFreeStyleProject();
+        p.setAssignedNode(s1);
+        FreeStyleBuild b1 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        assertEquals(s1, b1.getBuiltOn());
+        FilePath ws1 = b1.getWorkspace();
+        assertNotNull(ws1);
+        ws1.act(new Detouch());
+        DumbSlave s2 = r.createOnlineSlave();
+        p.setAssignedNode(s2);
+        FreeStyleBuild b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        assertEquals(s2, b2.getBuiltOn());
+        FilePath ws2 = b2.getWorkspace();
+        assertNotNull(ws2);
+        ws2.act(new Detouch());
+        p.setAssignedNode(r.jenkins);
+        FreeStyleBuild b3 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        assertEquals(r.jenkins, b3.getBuiltOn());
+        assertEquals(r.jenkins, p.getLastBuiltOn());
+        new WorkspaceCleanupThread().execute(StreamTaskListener.fromStdout());
+        assertFalse(ws1.exists());
+        assertFalse(ws2.exists());
+    }
+
+    @Bug(21023)
+    @Test public void modernMasterWorkspaceLocation() throws Exception {
+        FreeStyleProject p = r.createFreeStyleProject();
+        FreeStyleBuild b1 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        assertEquals(r.jenkins, b1.getBuiltOn());
+        FilePath ws1 = b1.getWorkspace();
+        assertNotNull(ws1);
+        ws1.act(new Detouch());
+        DumbSlave s = r.createOnlineSlave();
+        p.setAssignedNode(s);
+        FreeStyleBuild b2 = r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        assertEquals(s, b2.getBuiltOn());
+        FilePath ws2 = b2.getWorkspace();
+        assertNotNull(ws2);
+        ws2.act(new Detouch());
+        assertEquals(s, p.getLastBuiltOn());
+        new WorkspaceCleanupThread().execute(StreamTaskListener.fromStdout());
+        assertFalse(ws1.exists());
+        assertTrue(ws2.exists());
+    }
+
+    @Bug(21023)
+    @Test public void jobInFolder() throws Exception {
+        MockFolder d = r.createFolder("d");
+        FreeStyleProject p1 = d.createProject(FreeStyleProject.class, "p");
+        FreeStyleBuild b1 = r.assertBuildStatusSuccess(p1.scheduleBuild2(0));
+        assertEquals(r.jenkins, b1.getBuiltOn());
+        FilePath ws1 = b1.getWorkspace();
+        assertNotNull(ws1);
+        ws1.act(new Detouch());
+        DumbSlave s1 = r.createOnlineSlave();
+        p1.setAssignedNode(s1);
+        FreeStyleBuild b2 = r.assertBuildStatusSuccess(p1.scheduleBuild2(0));
+        assertEquals(s1, b2.getBuiltOn());
+        FilePath ws2 = b2.getWorkspace();
+        assertNotNull(ws2);
+        ws2.act(new Detouch());
+        DumbSlave s2 = r.createOnlineSlave();
+        p1.setAssignedNode(s2);
+        FreeStyleBuild b3 = r.assertBuildStatusSuccess(p1.scheduleBuild2(0));
+        assertEquals(s2, b3.getBuiltOn());
+        FilePath ws3 = b3.getWorkspace();
+        assertNotNull(ws3);
+        ws3.act(new Detouch());
+        assertEquals(s2, p1.getLastBuiltOn());
+        FreeStyleProject p2 = d.createProject(FreeStyleProject.class, "p2");
+        p2.setAssignedNode(s1);
+        FreeStyleBuild b4 = r.assertBuildStatusSuccess(p2.scheduleBuild2(0));
+        assertEquals(s1, b4.getBuiltOn());
+        FilePath ws4 = b4.getWorkspace();
+        assertNotNull(ws4);
+        ws4.act(new Detouch());
+        assertEquals(s1, p2.getLastBuiltOn());
+        ws2.getParent().act(new Detouch()); // ${s1.rootPath}/workspace/d/
+        new WorkspaceCleanupThread().execute(StreamTaskListener.fromStdout());
+        assertFalse(ws1.exists());
+        assertFalse(ws2.exists());
+        assertTrue(ws3.exists());
+        assertTrue(ws4.exists());
+    }
+
+    private static final class Detouch implements FileCallable<Void> {
+        @Override public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+            Assume.assumeTrue("failed to reset lastModified on " + f, f.setLastModified(0));
+            return null;
+        }
+    }
+
+}