[FIXED JENKINS-24773] Allocated and blacklisted display number are sh…

…ared accross all slaves
jenkinsci · Sep 19, 2014 · 3627859 · 3627859
1 parent cbdcafe
commit 3627859
Show file tree

Hide file tree

Showing 3 changed files with 178 additions and 5 deletions.
diff --git a/src/main/java/hudson/plugins/xvnc/DisplayAllocator.java b/src/main/java/hudson/plugins/xvnc/DisplayAllocator.java
@@ -1,8 +1,14 @@
 package hudson.plugins.xvnc;
 
+import hudson.model.Node;
+import hudson.slaves.NodeProperty;
+
+import java.util.HashSet;
 import java.util.Random;
 import java.util.Set;
-import java.util.HashSet;
+
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 /**
  * Manages the display numbers in use.
@@ -61,4 +67,15 @@ public void blacklist(int badDisplay) {
         free(badDisplay);
         blacklistedNumbers.add(badDisplay);
     }
+
+    @Restricted(NoExternalUse.class)
+    /*package*/ static final class Property extends NodeProperty<Node> {
+
+        private transient final DisplayAllocator allocator = new DisplayAllocator();
+
+
+        /*package*/ DisplayAllocator getAllocator() {
+            return allocator;
+        }
+    }
 }
diff --git a/src/main/java/hudson/plugins/xvnc/Xvnc.java b/src/main/java/hudson/plugins/xvnc/Xvnc.java
@@ -22,6 +22,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.WeakHashMap;
+
 import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
 
@@ -83,6 +84,8 @@ private Environment doSetUp(AbstractBuild build, final Launcher launcher, final
             String cmd, int retries, int minDisplayNumber, int maxDisplayNumber)
                     throws IOException, InterruptedException {
 
+        final DisplayAllocator allocator = getAllocator(build);
+
         final int displayNumber = allocator.allocate(minDisplayNumber, maxDisplayNumber);
         final String actualCmd = Util.replaceMacro(cmd, Collections.singletonMap("DISPLAY_NUMBER",String.valueOf(displayNumber)));
 
@@ -156,10 +159,14 @@ public boolean tearDown(AbstractBuild build, BuildListener listener) throws IOEx
         };
     }
 
-    /**
-     * Manages display numbers in use.
-     */
-    private static final DisplayAllocator allocator = new DisplayAllocator();
+    private DisplayAllocator getAllocator(AbstractBuild<?, ?> build) throws IOException {
+        DisplayAllocator.Property property = build.getBuiltOn().getNodeProperties().get(DisplayAllocator.Property.class);
+        if (property == null) {
+            property = new DisplayAllocator.Property();
+            build.getBuiltOn().getNodeProperties().add(property);
+        }
+        return property.getAllocator();
+    }
 
     /**
      * Whether {@link #maybeCleanUp} has already been run on a given node.
@@ -218,6 +225,7 @@ public DescriptorImpl() {
             load();
         }
 
+        @Override
         public String getDisplayName() {
             return Messages.description();
         }
@@ -230,6 +238,7 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti
             return true;
         }
 
+        @Override
         public boolean isApplicable(AbstractProject<?, ?> item) {
             return true;
         }

diff --git a/src/test/java/hudson/plugins/xvnc/XvncTest.java b/src/test/java/hudson/plugins/xvnc/XvncTest.java
@@ -0,0 +1,147 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2014 Red Hat, Inc.
+ *
+ * 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.plugins.xvnc;
+
+import hudson.Launcher;
+import hudson.model.BuildListener;
+import hudson.model.FreeStyleBuild;
+import hudson.model.Result;
+import hudson.model.AbstractBuild;
+import hudson.model.FreeStyleProject;
+import hudson.model.Hudson;
+import hudson.plugins.xvnc.Xvnc.DescriptorImpl;
+import hudson.slaves.DumbSlave;
+import hudson.tasks.Builder;
+import hudson.util.OneShotEvent;
+
+import java.io.IOException;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.JenkinsRule;
+
+public class XvncTest {
+
+    @Rule public JenkinsRule j = new JenkinsRule();
+
+    @Test @Bug(24773)
+    public void distinctDisplaySpaceForSlaves() throws Exception {
+        DumbSlave slaveA = j.createOnlineSlave();
+        DumbSlave slaveB = j.createOnlineSlave();
+
+        FreeStyleProject jobA = j.jenkins.createProject(FreeStyleProject.class, "jobA");
+        jobA.setAssignedNode(slaveA);
+        FreeStyleProject jobB = j.jenkins.createProject(FreeStyleProject.class, "jobB");
+        jobB.setAssignedNode(slaveB);
+
+        fakeXvncRun(jobA);
+        fakeXvncRun(jobB);
+
+        jobA.getBuildersList().add(new Blocker());
+        jobA.scheduleBuild2(0);
+        Blocker.RUNNING.block(1000);
+
+        FreeStyleBuild build = jobB.scheduleBuild2(0).get();
+        j.assertBuildStatusSuccess(build);
+
+        Blocker.DONE.signal();
+    }
+
+    @Test // The number should not be allocated as builds are executed sequentially
+    public void reuseDisplayNumberOnSameSlave() throws Exception {
+        FreeStyleProject p = j.jenkins.createProject(FreeStyleProject.class, "project");
+
+        runXvnc(p).cleanUp = true;;
+
+        j.buildAndAssertSuccess(p);
+        j.buildAndAssertSuccess(p);
+    }
+
+    @Test
+    public void displayBlacklistedOnOneMachineShouldNobBeBlackistedOnAnother() throws Exception {
+        DumbSlave slaveA = j.createOnlineSlave();
+        DumbSlave slaveB = j.createOnlineSlave();
+
+        FreeStyleProject jobA = j.jenkins.createProject(FreeStyleProject.class, "jobA");
+        jobA.setAssignedNode(slaveA);
+        FreeStyleProject jobB = j.jenkins.createProject(FreeStyleProject.class, "jobB");
+        jobB.setAssignedNode(slaveB);
+
+        // blacklist :42 on slaveA
+        runXvnc(jobA).xvnc = "vncserver :$DISPLAY_NUMBER with wrong args so it wont start";
+        j.assertBuildStatus(Result.FAILURE, jobA.scheduleBuild2(0).get());
+
+        // use :42 on slaveB
+        runXvnc(jobB).cleanUp = true;
+        j.buildAndAssertSuccess(jobB);
+    }
+
+    @Test
+    public void blacklistedDisplayShouldStayBlacklistedBetweenBuilds() throws Exception {
+        FreeStyleProject p = j.jenkins.createProject(FreeStyleProject.class, "project");
+
+        // Blacklist
+        runXvnc(p).xvnc = "vncserver :$DISPLAY_NUMBER with wrong args so it wont start";
+        j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
+
+        // Should still fail
+        runXvnc(p).xvnc = "vncserver :$DISPLAY_NUMBER with wrong args so it wont start";
+        j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
+    }
+
+    private Xvnc fakeXvncRun(FreeStyleProject p) throws Exception {
+        final Xvnc xvnc = new Xvnc(false, false);
+        p.getBuildWrappersList().add(xvnc);
+        DescriptorImpl descriptor = Hudson.getInstance().getDescriptorByType(DescriptorImpl.class);
+        descriptor.maxDisplayNumber = descriptor.minDisplayNumber = 42;
+        // Do nothing so next build using the same display can succeed. This is poor man's simulation of distinct build machine
+        descriptor.xvnc = "true";
+        return xvnc;
+    }
+
+    private Xvnc.DescriptorImpl runXvnc(FreeStyleProject p) throws Exception {
+        final Xvnc xvnc = new Xvnc(false, false);
+        p.getBuildWrappersList().add(xvnc);
+        DescriptorImpl descriptor = Hudson.getInstance().getDescriptorByType(DescriptorImpl.class);
+        descriptor.maxDisplayNumber = descriptor.minDisplayNumber = 42;
+        descriptor.xvnc = null;
+        return descriptor;
+    }
+
+    private static class Blocker extends Builder {
+
+        private static final OneShotEvent RUNNING = new OneShotEvent();
+        private static final OneShotEvent DONE = new OneShotEvent();
+
+        public Blocker() {}
+
+        @Override
+        public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener) throws InterruptedException, IOException {
+            RUNNING.signal();
+            DONE.block(1000);
+            return true;
+        }
+    }
+}