Merge commit 'cd526e99'

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/Node.java

@@ -371,7 +371,7 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable
      *      null if this node is not connected hence the path is not available
      */
     // TODO: should this be modified now that getWorkspace is moved from AbstractProject to AbstractBuild?
-    public abstract FilePath getWorkspaceFor(TopLevelItem item);
+    public abstract @CheckForNull FilePath getWorkspaceFor(TopLevelItem item);
 
     /**
      * Gets the root directory of this node.
@@ -384,7 +384,7 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable
      *      null if the node is offline and hence the {@link FilePath}
      *      object is not available.
      */
-    public abstract FilePath getRootPath();
+    public abstract @CheckForNull FilePath getRootPath();
 
     /**
      * Gets the {@link FilePath} on this node.

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

@@ -57,6 +57,7 @@ import java.util.Set;
 import javax.servlet.ServletException;
 
 import hudson.util.TimeUnit2;
+import javax.annotation.CheckForNull;
 import jenkins.model.Jenkins;
 import jenkins.slaves.WorkspaceLocator;
 
@@ -284,7 +285,7 @@ public abstract class Slave extends Node implements Serializable {
      * @return
      *      null if not connected.
      */
-    public FilePath getWorkspaceRoot() {
+    public @CheckForNull FilePath getWorkspaceRoot() {
         FilePath r = getRootPath();
         if(r==null) return null;
         return r.child(WORKSPACE_ROOT);

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;
+        }
+    }
+
+}