redone the custom workspace support in matrix project.

The previous implementation was always appending the per-configuration unique suffix, making it impossible for different configuration builds to share workspaces. In this fix, we introduce a secondary field to control the workspace of sub-builds (which can be either absolute or relative to the matrix head workspace.)

changelog.html

@@ -55,7 +55,12 @@ Upcoming changes</a>
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
-  <li class=>
+  <li class=rfe>
+    Matrix custom workspace support is improved to allow configuration builds to share workspace
+  <li class=rfe>
+    Exposed plugin manager and update center to the REST API
+  <li class=rfe>
+    Enabled concurrent build support for matrix projects
 </ul>
 </div><!--=TRUNK-END=-->
 

core/src/main/java/hudson/matrix/MatrixConfiguration.java

@@ -316,7 +316,7 @@ public class MatrixConfiguration extends Project<MatrixConfiguration,MatrixRun>
      * See http://cygwin.com/ml/cygwin/2005-04/msg00395.html and
      * http://www.nabble.com/Windows-Filename-too-long-errors-t3161089.html for
      * the background of this issue. Setting this flag to true would
-     * cause Hudson to use cryptic but short path name, giving more room for
+     * cause Jenkins to use cryptic but short path name, giving more room for
      * jobs to use longer path names.
      */
     public static boolean useShortWorkspaceName = Boolean.getBoolean(MatrixConfiguration.class.getName()+".useShortWorkspaceName");

core/src/main/java/hudson/matrix/MatrixProject.java

@@ -34,6 +34,8 @@ import hudson.model.BuildableItemWithBuildWrappers;
 import hudson.model.DependencyGraph;
 import hudson.model.Descriptor;
 import hudson.model.Descriptor.FormException;
+import hudson.model.Node;
+import hudson.slaves.WorkspaceList;
 import jenkins.model.Jenkins;
 import hudson.model.Item;
 import hudson.model.ItemGroup;
@@ -47,7 +49,6 @@ import hudson.model.SCMedItem;
 import hudson.model.Saveable;
 import hudson.model.TopLevelItem;
 import hudson.tasks.BuildStep;
-import hudson.tasks.BuildStepDescriptor;
 import hudson.tasks.BuildWrapper;
 import hudson.tasks.BuildWrappers;
 import hudson.tasks.Builder;
@@ -159,6 +160,20 @@ public class MatrixProject extends AbstractProject<MatrixProject,MatrixBuild> im
 
     private MatrixExecutionStrategy executionStrategy;
 
+    /**
+     * Custom workspace location for {@link MatrixConfiguration}s.
+     * 
+     * <p>
+     * (Historically, we used {@link AbstractProject#customWorkspace} + some unique suffix (see {@link MatrixConfiguration#useShortWorkspaceName})
+     * for custom workspace, but we now separated that so that the user has more control.
+     * 
+     * <p>
+     * If null, the historical semantics is assumed.
+     *
+     * @since 1.466
+     */
+    private String childCustomWorkspace;
+
     public MatrixProject(String name) {
         this(Jenkins.getInstance(), name);
     }
@@ -167,7 +182,38 @@ public class MatrixProject extends AbstractProject<MatrixProject,MatrixBuild> im
         super(parent, name);
     }
 
+    /**
+     * Gets the workspace location that {@link MatrixConfiguration} uses.
+     * 
+     * @see MatrixRun.RunnerImpl#decideWorkspace(Node, WorkspaceList) 
+     * 
+     * @return never null
+     *      even when {@link MatrixProject} uses no custom workspace, this method still
+     *      returns something like "${PARENT_WORKSPACE}/${COMBINATION}" that controls
+     *      how the workspace should be laid out.
+     *
+     *      The return value can be absolute or relative. If relative, it is resolved
+     *      against the working directory of the overarching {@link MatrixBuild}.
+     */
+    public String getChildCustomWorkspace() {
+        String ws = childCustomWorkspace;
+        if (ws==null) {
+            ws = MatrixConfiguration.useShortWorkspaceName?"${SHORT_COMBINATION}":"${COMBINATION}";
+        }
+        return ws;
+    }
+
+    /**
+     * Do we have an explicit child custom workspace setting (true)? Or just using the default value (false)?
+     */
+    public boolean hasChildCustomWorkspace() {
+        return childCustomWorkspace!=null;
+    }
 
+    public void setChildCustomWorkspace(String childCustomWorkspace) throws IOException {
+        this.childCustomWorkspace = Util.fixEmptyAndTrim(childCustomWorkspace);
+        save();
+    }
 
     /**
      * {@link MatrixProject} is relevant with all the labels its configurations are relevant.
@@ -705,6 +751,12 @@ public class MatrixProject extends AbstractProject<MatrixProject,MatrixBuild> im
             this.combinationFilter = null;
         }
 
+        if(req.hasParameter("customWorkspace")) {
+            setChildCustomWorkspace(Util.fixEmptyAndTrim(req.getParameter("childCustomWorkspace.directory")));
+        } else {
+            setChildCustomWorkspace(null);
+        }
+
         List<MatrixExecutionStrategyDescriptor> esd = getDescriptor().getExecutionStrategyDescriptors();
         if (esd.size()>1)
             executionStrategy = req.bindJSON(MatrixExecutionStrategy.class,json.getJSONObject("executionStrategy"));

core/src/main/java/hudson/matrix/MatrixRun.java

@@ -23,11 +23,12 @@
  */
 package hudson.matrix;
 
+import hudson.EnvVars;
 import hudson.FilePath;
 import hudson.model.AbstractBuild;
+import hudson.model.TopLevelItem;
 import hudson.slaves.WorkspaceList;
 import hudson.slaves.WorkspaceList.Lease;
-import static hudson.matrix.MatrixConfiguration.useShortWorkspaceName;
 import hudson.model.Build;
 import hudson.model.Node;
 import org.kohsuke.stapler.Ancestor;
@@ -147,29 +148,39 @@ public class MatrixRun extends Build<MatrixConfiguration,MatrixRun> {
     }
 
     protected class RunnerImpl extends Build<MatrixConfiguration,MatrixRun>.RunnerImpl {
-        @Override
-        protected Lease decideWorkspace(Node n, WorkspaceList wsl) throws InterruptedException, IOException {
-            // Map current combination to a directory subtree, e.g. 'axis1=a,axis2=b' to 'axis1/a/axis2/b'.
-            String subtree;
-            if(useShortWorkspaceName) {
-                subtree = getParent().getDigestName(); 
-            } else {
-                subtree = getParent().getCombination().toString('/','/');
-            }
-            
-            String customWorkspace = getParent().getParent().getCustomWorkspace();
+        protected Lease getParentWorkspaceLease(Node n, WorkspaceList wsl) throws InterruptedException, IOException {
+            MatrixProject mp = getParent().getParent();
+
+            String customWorkspace = mp.getCustomWorkspace();
             if (customWorkspace != null) {
-                // Use custom workspace as defined in the matrix project settings.
-                FilePath ws = n.getRootPath().child(getEnvironment(listener).expand(customWorkspace));
-                // We allow custom workspaces to be used concurrently between jobs.
-                return Lease.createDummyLease(ws.child(subtree));
-            } else {
-                // Use default workspace as assigned by Jenkins.
-                Node node = getBuiltOn();
-                FilePath ws = node.getWorkspaceFor(getParent().getParent());
-                // Allocate unique workspace (not to be shared between jobs and runs).
-                return wsl.allocate(ws.child(subtree));
+                // we allow custom workspaces to be concurrently used between jobs.
+                return Lease.createDummyLease(n.getRootPath().child(getEnvironment(listener).expand(customWorkspace)));
             }
+            return wsl.allocate(n.getWorkspaceFor(mp), getParentBuild());
+        }
+
+        @Override
+        protected Lease decideWorkspace(Node n, WorkspaceList wsl) throws InterruptedException, IOException {
+            MatrixProject mp = getParent().getParent();
+
+            // lock is done at the parent level, so that concurrent MatrixProjects get respective workspace,
+            // but within MatrixConfigurations that belong to the same MatrixBuild.
+            // if MatrixProject is configured with custom workspace, we assume that the user knows what he's doing
+            // and try not to append unique random suffix.
+            Lease baseLease = getParentWorkspaceLease(n,wsl);
+
+            // resolve the relative path against the parent workspace, which needs locking
+            FilePath baseDir = baseLease.path;
+
+            // prepare variables that can be used in the child workspace setting
+            EnvVars env = getEnvironment(listener);
+            env.put("COMBINATION",getParent().getCombination().toString('/','/'));  // e.g., "axis1/a/axis2/b"
+            env.put("SHORT_COMBINATION",getParent().getDigestName());               // e.g., "0fbcab35"
+            env.put("PARENT_WORKSPACE",baseDir.getRemote());
+
+            // child workspace need no individual locks, whether or not we use custom workspace
+            String childWs = mp.getChildCustomWorkspace();
+            return Lease.createLinkedDummyLease(baseDir.child(env.expand(childWs)),baseLease);
         }
     }
 }

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

@@ -444,7 +444,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
                 return Lease.createDummyLease(n.getRootPath().child(getEnvironment(listener).expand(customWorkspace)));
             }
             // TODO: this cast is indicative of abstraction problem
-            return wsl.allocate(n.getWorkspaceFor((TopLevelItem)getProject()));
+            return wsl.allocate(n.getWorkspaceFor((TopLevelItem)getProject()), getBuild());
         }
 
         public Result run(BuildListener listener) throws Exception {

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

@@ -1707,8 +1707,6 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
 
         if(req.hasParameter("customWorkspace")) {
             customWorkspace = Util.fixEmptyAndTrim(req.getParameter("customWorkspace.directory"));
-            if(customWorkspace==null)
-            	throw new FormException("Custom workspace is empty", "customWorkspace");
         } else {
             customWorkspace = null;
         }

core/src/main/java/hudson/slaves/WorkspaceList.java

@@ -36,9 +36,6 @@ import java.util.logging.Logger;
 /**
  * Used by {@link Computer} to keep track of workspaces that are actively in use.
  *
- * <p>
- * SUBJECT TO CHANGE! Do not use this from plugins directly.
- *
  * @author Kohsuke Kawaguchi
  * @since 1.319
  * @see Computer#getWorkspaceList()
@@ -71,9 +68,21 @@ public final class WorkspaceList {
 
         public final FilePath path;
 
+        /**
+         * Multiple threads can acquire the same lock if they share the same context object.
+         */
+        public final Object context;
+        
+        public int lockCount=1;
+
         private Entry(FilePath path, boolean quick) {
+            this(path,quick,new Object()); // unique context
+        }
+        
+        private Entry(FilePath path, boolean quick, Object context) {
             this.path = path;
             this.quick = quick;
+            this.context = context;
         }
 
         @Override
@@ -110,6 +119,18 @@ public final class WorkspaceList {
                 }
             };
         }
+
+        /**
+         * Creates a {@link Lease} object that points  to the specified path, but the lock
+         * is controlled by the given parent lease object.
+         */
+        public static Lease createLinkedDummyLease(FilePath p, final Lease parent) {
+            return new Lease(p) {
+                public void release() {
+                    parent.release();
+                }
+            };
+        }
     }
 
     private final Map<FilePath,Entry> inUse = new HashMap<FilePath,Entry>();
@@ -119,19 +140,34 @@ public final class WorkspaceList {
 
     /**
      * Allocates a workspace by adding some variation to the given base to make it unique.
+     *
+     * <p>
+     * This method doesn't block prolonged amount of time. Whenever a desired workspace
+     * is in use, the unique variation is added.
      */
     public synchronized Lease allocate(FilePath base) throws InterruptedException {
+        return allocate(base,new Object());
+    }
+
+    /**
+     * See {@link #allocate(FilePath)}
+     * 
+     * @param context
+     *      Threads that share the same context can re-acquire the same lock (which will just increment the lock count.)
+     *      This allows related executors to share the same workspace.
+     */
+    public synchronized Lease allocate(FilePath base, Object context) throws InterruptedException {
         for (int i=1; ; i++) {
             FilePath candidate = i==1 ? base : base.withSuffix(COMBINATOR+i);
             Entry e = inUse.get(candidate);
-            if(e!=null && !e.quick)
+            if(e!=null && !e.quick && e.context!=context)
                 continue;
-            return acquire(candidate);
+            return acquire(candidate,false,context);
         }
     }
 
     /**
-     * Just record that this workspace is being used, without paying any attention to the sycnhronization support.
+     * Just record that this workspace is being used, without paying any attention to the synchronization support.
      */
     public synchronized Lease record(FilePath p) {
         log("recorded  "+p);
@@ -145,9 +181,12 @@ public final class WorkspaceList {
      * Releases an allocated or acquired workspace.
      */
     private synchronized void _release(FilePath p) {
-        Entry old = inUse.remove(p);
+        Entry old = inUse.get(p);
         if (old==null)
             throw new AssertionError("Releasing unallocated workspace "+p);
+        old.lockCount--;
+        if (old.lockCount==0)
+            inUse.remove(p);
         notifyAll();
     }
 
@@ -169,18 +208,36 @@ public final class WorkspaceList {
      *      This makes other calls to {@link #allocate(FilePath)} to wait for the release of this workspace.
      */
     public synchronized Lease acquire(FilePath p, boolean quick) throws InterruptedException {
+        return acquire(p,quick,new Object());
+    }
+    
+    /**
+     * See {@link #acquire(FilePath,boolean)}
+     *
+     * @param context
+     *      Threads that share the same context can re-acquire the same lock (which will just increment the lock count.)
+     *      This allows related executors to share the same workspace.
+     */
+    public synchronized Lease acquire(FilePath p, boolean quick, Object context) throws InterruptedException {
+        Entry e;
+
         Thread t = Thread.currentThread();
         String oldName = t.getName();
         t.setName("Waiting to acquire "+p+" : "+t.getName());
         try {
-            while (inUse.containsKey(p)) {
+            while (true) {
+                e = inUse.get(p);
+                if (e==null || e.context==context)
+                    break;
                 wait();
             }
         } finally {
             t.setName(oldName);
         }
         log("acquired "+p);
-        inUse.put(p,new Entry(p,quick));
+        
+        if (e!=null)    e.lockCount++;
+        else            inUse.put(p,new Entry(p,quick,context));
         return lease(p);
     }
 

core/src/main/resources/hudson/matrix/MatrixProject/configure-entries.jelly

@@ -38,7 +38,17 @@ THE SOFTWARE.
       <p:config-retryCount />
       <p:config-blockWhenUpstreamBuilding />
       <p:config-blockWhenDownstreamBuilding />
-      <p:config-customWorkspace />
+      <f:optionalBlock name="customWorkspace" title="${%Use custom workspace}"
+                       checked="${instance.customWorkspace!=null or instance.hasChildCustomWorkspace()}"
+                       help="/help/project-config/custom-workspace.html">
+        <f:entry title="${%Directory}">
+          <f:textbox name="customWorkspace.directory" field="customWorkspace" />
+        </f:entry>
+        <f:entry title="${%Directory for sub-builds}">
+          <f:textbox name="childCustomWorkspace.directory" field="childCustomWorkspace"  value="${instance.hasChildCustomWorkspace()?instance.childCustomWorkspace:''}" />
+        </f:entry>
+      </f:optionalBlock>
+
       <f:entry title="${%Display Name}" field="displayNameOrNull">
          <f:textbox checkUrl="'${rootURL}/checkDisplayName?displayName='+encodeURIComponent(this.value)+'&amp;jobName='+encodeURIComponent('${it.name}')"/>
       </f:entry>

core/src/main/resources/hudson/matrix/MatrixProject/help-childCustomWorkspace.html

+<div>
+    Override the workspace directory assigned to each configuration build.
+
+    <p>
+    By default, Jenkins runs each configuration build in its own unique directory.
+    (this happens by appending the name and the value of the axes to the workspace of the overall matrix execution,
+    for example, <tt>/path/to/slaveroot/job/someMatrixJob/axis1/value1/axis2/value2/</tt>. Specifying this value
+    allows you to override this behaviour.
+
+    <p>
+    A common value used here is ".", indicating that all the configuration builds run on the same
+    directory as the overall matrix execution.
+
+    <p>
+    The path specified here can be either absolute or relative. If relative, it is resolved against the workspace
+    of the overall matrix execution. The value can also contains environment variable references. Environment variables
+    include all the axis values, so for example if you specify <tt>$FOO</tt> when you define
+    two axes FOO=[a,b] and BAR=[x,y], then FOO=a,BAR=x and FOO=a,BAR=y will build in somewhere like
+    <tt>/slaveroot/job/someMatrixJob/a/</tt> while FOO=b,BAR=x and FOO=b,BAR=y will build in
+    <tt>/slaveroot/job/someMatrixJob/b/</tt>. In this way, you can be selective about what subset
+    of configuration builds share the same workspace and what doesn't.
+
+    <p>
+    Environment variables include special tokens
+    <tt>${COMBINATION}</tt>, which expands to <tt>axis1/value1/axis2/value2/...</tt>, and
+    <tt>${SHORT_COMBINATION}</tt>, which expands to 8 character string unique to each configuration.
+
+</div>
\ No newline at end of file

core/src/main/resources/lib/hudson/project/config-customWorkspace.jelly

@@ -25,7 +25,7 @@ THE SOFTWARE.
 <!-- custom workspace -->
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
-<f:optionalBlock name="customWorkspace" title="${%Use custom workspace}" checked="${it.customWorkspace!=null}" help="/help/project-config/custom-workspace.html">
+  <f:optionalBlock name="customWorkspace" title="${%Use custom workspace}" checked="${it.customWorkspace!=null}" help="/help/project-config/custom-workspace.html">
     <f:entry title="${%Directory}">
       <f:textbox name="customWorkspace.directory" field="customWorkspace" />
     </f:entry>

test/src/test/groovy/hudson/matrix/MatrixProjectCustomWorkspaceTest.groovy

+package hudson.matrix
+
+import java.util.concurrent.CountDownLatch
+import org.jvnet.hudson.test.TestBuilder
+import hudson.model.AbstractBuild
+import hudson.Launcher
+import hudson.model.BuildListener
+import org.jvnet.hudson.test.HudsonTestCase
+
+/**
+ * Tests the custom workspace support in {@link MatrixProject}.
+ *
+ * To validate the lease behaviour, use concurrent builds to run two builds and make sure they get
+ * same/different workspaces.
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class MatrixProjectCustomWorkspaceTest extends HudsonTestCase {
+    /**
+     * Test the case where both the parent and the child has custom workspace specified.
+     */
+    void testCustomWorkspace1() {
+        def p = createMatrixProject()
+        def dir = env.temporaryDirectoryAllocator.allocate()
+        p.customWorkspace = dir
+        p.childCustomWorkspace = "xyz"
+
+        configRoundtrip(p)
+        configureCustomWorkspaceConcurrentBuild(p)
+
+        // all concurrent builds should build on the same one workspace
+        runTwoConcurrentBuilds(p).each { b ->
+            assertEquals(dir.path, b.workspace.getRemote())
+            b.runs.each { r -> assertEquals(new File(dir,"xyz").path, r.workspace.getRemote()) }
+        }
+    }
+
+    /**
+     * Test the case where only the parent has a custom workspace.
+     */
+    void testCustomWorkspace2() {
+        def p = createMatrixProject()
+        def dir = env.temporaryDirectoryAllocator.allocate()
+        p.customWorkspace = dir
+        p.childCustomWorkspace = null
+
+        configRoundtrip(p)
+        configureCustomWorkspaceConcurrentBuild(p)
+
+        def bs = runTwoConcurrentBuilds(p)
+
+        // all parent builds share the same workspace
+        bs.each { b ->
+            assertEquals(dir.path, b.workspace.getRemote())
+        }
+        // foo=1 #1 and foo=1 #2 shares the same workspace,
+        (0..<2).each { i ->
+            assertTrue bs[0].runs[i].workspace == bs[1].runs[i].workspace
+        }
+        // but foo=1 #1 and foo=2 #1 shouldn't.
+        (0..<2).each { i ->
+            assertTrue bs[i].runs[0].workspace != bs[i].runs[1].workspace
+        }
+    }
+
+    /**
+     * Test the case where only the child has a custom workspace.
+     */
+    void testCustomWorkspace3() {
+        def p = createMatrixProject()
+        p.customWorkspace = null
+        p.childCustomWorkspace = "."
+
+        configRoundtrip(p)
+        configureCustomWorkspaceConcurrentBuild(p)
+
+        def bs = runTwoConcurrentBuilds(p)
+
+        // each parent gets different directory
+        assertTrue bs[0].workspace != bs[1].workspace
+        // but all #1 builds should get the same workspace
+        bs.each { b ->
+            (0..<2).each { i-> assertTrue b.workspace == b.runs[i].workspace }
+        }
+    }
+
+    /**
+     * Test the case where neither has custom workspace
+     */
+    void testCustomWorkspace4() {
+        def p = createMatrixProject()
+        p.customWorkspace = null
+        p.childCustomWorkspace = null
+
+        configRoundtrip(p)
+        configureCustomWorkspaceConcurrentBuild(p)
+
+        def bs = runTwoConcurrentBuilds(p)
+
+        // each parent gets different directory
+        assertTrue bs[0].workspace != bs[1].workspace
+        // and every sub-build gets a different directory
+        bs.each { b ->
+            def x = b.runs[0].workspace
+            def y = b.runs[1].workspace
+            def z = b.workspace
+            
+            assertTrue x!=y
+            assertTrue y!=z
+            assertTrue z!=x
+        }
+    }
+
+    /**
+     * Configures MatrixProject such that two builds run concurrently.
+     */
+    def configureCustomWorkspaceConcurrentBuild(MatrixProject p) {
+        // needs sufficient parallel execution capability
+        jenkins.numExecutors = 10
+        jenkins.updateComputerList()
+
+        p.axes = new AxisList(new TextAxis("foo", "1", "2"))
+        p.concurrentBuild = true;
+        def latch = new CountDownLatch(4)
+
+        p.buildersList.add(new TestBuilder() {
+            boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) {
+                latch.countDown()
+                latch.await()
+                return true
+            }
+        })
+    }
+
+    /**
+     * Runs two concurrent builds and return their results.
+     */
+    List<MatrixBuild> runTwoConcurrentBuilds(MatrixProject p) {
+        def f1 = p.scheduleBuild2(0)
+        // get one going
+        Thread.sleep(1000)
+        def f2 = p.scheduleBuild2(0)
+
+        def bs = [f1, f2]*.get().each { assertBuildStatusSuccess(it) }
+        return bs
+    }
+}