[FIX JENKINS-30909] Make sure queue is persisted reliably (#3244)

* [FIX JENKINS-30909] Make sure queue is persisted reliably * Fix tests on Windows * Ensure Saver is thread-safe (cherry picked from commit 72a7529)
jenkinsci · Mar 23, 2018 · 8c975f1 · 8c975f1
1 parent 96c52d9
commit 8c975f1
Show file tree

Hide file tree

Showing 17 changed files with 625 additions and 2 deletions.
diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java
@@ -24,10 +24,12 @@
  */
 package hudson.model;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
 import hudson.BulkChange;
+import hudson.Extension;
 import hudson.ExtensionList;
 import hudson.ExtensionPoint;
 import hudson.Util;
@@ -64,7 +66,10 @@
 import hudson.security.ACL;
 import hudson.security.AccessControlled;
 import java.nio.file.Files;
+
+import hudson.util.Futures;
 import jenkins.security.QueueItemAuthenticatorProvider;
+import jenkins.util.SystemProperties;
 import jenkins.util.Timer;
 import hudson.triggers.SafeTimerTask;
 import java.util.concurrent.TimeUnit;
@@ -93,7 +98,6 @@
 import java.util.Set;
 import java.util.TreeSet;
 import java.util.concurrent.Callable;
-import java.util.concurrent.TimeUnit;
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.Condition;
@@ -102,6 +106,7 @@
 import java.util.logging.Logger;
 
 import javax.annotation.Nonnull;
+import javax.annotation.concurrent.GuardedBy;
 import javax.servlet.ServletException;
 
 import jenkins.model.Jenkins;
@@ -3009,4 +3014,75 @@ public static Queue getInstance() {
     public static void init(Jenkins h) {
         h.getQueue().load();
     }
+
+    /**
+     * Schedule <tt>Queue.save()</tt> call for near future once items change. Ignore all changes until the time the save
+     * takes place.
+     *
+     * Once queue is restored after a crash, items stages might not be accurate until the next #maintain() - this is not
+     * a problem as the items will be reshuffled first and then scheduled during the next maintainance cycle.
+     *
+     * Implementation note: Queue.load() calls QueueListener hooks for every item deserialized that can hammer the persistance
+     * on load. The problem is avoided by delaying the actual save for the time long enough for queue to load so the save
+     * operations will collapse into one. Also, items are persisted as buildable or blocked in vast majority of cases and
+     * those stages does not trigger the save here.
+     */
+    @Extension
+    @Restricted(NoExternalUse.class)
+    public static final class Saver extends QueueListener implements Runnable {
+
+        /**
+         * All negative values will disable periodic saving.
+         */
+        @VisibleForTesting
+        /*package*/ static /*final*/ int DELAY_SECONDS = SystemProperties.getInteger("hudson.model.Queue.Saver.DELAY_SECONDS", 60);
+
+        private final Object lock = new Object();
+        @GuardedBy("lock")
+        private Future<?> nextSave;
+
+        @Override
+        public void onEnterWaiting(WaitingItem wi) {
+            push();
+        }
+
+        @Override
+        public void onLeft(Queue.LeftItem li) {
+            push();
+        }
+
+        private void push() {
+            if (DELAY_SECONDS < 0) return;
+
+            synchronized (lock) {
+                // Can be done or canceled in case of a bug or external intervention - do not allow it to hang there forever
+                if (nextSave != null && !(nextSave.isDone() || nextSave.isCancelled())) return;
+                nextSave = Timer.get().schedule(this, DELAY_SECONDS, TimeUnit.SECONDS);
+            }
+        }
+
+        @Override
+        public void run() {
+            try {
+                Jenkins j = Jenkins.getInstanceOrNull();
+                if (j != null) {
+                    j.getQueue().save();
+                }
+            } finally {
+                synchronized (lock) {
+                    nextSave = null;
+                }
+            }
+        }
+
+        @VisibleForTesting @Restricted(NoExternalUse.class)
+        /*package*/ @Nonnull Future<?> getNextSave() {
+            synchronized (lock) {
+                return nextSave == null
+                        ? Futures.precomputed(null)
+                        : nextSave
+                ;
+            }
+        }
+    }
 }
diff --git a/test/pom.xml b/test/pom.xml
@@ -53,7 +53,7 @@ THE SOFTWARE.
     <dependency>
       <groupId>${project.groupId}</groupId>
       <artifactId>jenkins-test-harness</artifactId>
-      <version>2.32</version>
+      <version>2.33</version>
       <scope>test</scope>
       <exclusions>
         <exclusion>

diff --git a/test/src/test/java/hudson/model/QueueRestartTest.java b/test/src/test/java/hudson/model/QueueRestartTest.java
@@ -0,0 +1,89 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 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.model;
+
+import hudson.ExtensionList;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.RestartableJenkinsRule;
+
+import java.io.IOException;
+import java.util.concurrent.TimeUnit;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class QueueRestartTest {
+
+    @Rule
+    public RestartableJenkinsRule j = new RestartableJenkinsRule();
+
+    @Test
+    public void persistQueueOnRestart() {
+        j.addStep(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                Queue.Saver.DELAY_SECONDS = 24 * 60 * 60; // Avoid period save as we are after the explicit one
+                scheduleSomeBuild();
+                assertBuildIsScheduled();
+            }
+        });
+        j.addStep(new Statement() {
+            @Override public void evaluate() {
+                assertBuildIsScheduled();
+            }
+        });
+    }
+
+    @Test
+    public void persistQueueOnCrash() {
+        j.addStepWithDirtyShutdown(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                Queue.Saver.DELAY_SECONDS = 0; // Call Queue#save() on every queue modification simulating time has passed before crash
+                scheduleSomeBuild();
+                assertBuildIsScheduled();
+
+                // Save have no delay though is not synchronous
+                ExtensionList.lookup(Queue.Saver.class).get(0).getNextSave().get(3, TimeUnit.SECONDS);
+
+                assertTrue("queue.xml does not exist", j.j.jenkins.getQueue().getXMLQueueFile().exists());
+            }
+        });
+        j.addStep(new Statement() {
+            @Override public void evaluate() {
+                assertBuildIsScheduled();
+            }
+        });
+    }
+
+    private void assertBuildIsScheduled() {
+        assertEquals(1, j.j.jenkins.getQueue().getItems().length);
+    }
+
+    private void scheduleSomeBuild() throws IOException {
+        FreeStyleProject p = j.j.createFreeStyleProject();
+        p.setAssignedLabel(Label.get("waitforit"));
+        p.scheduleBuild2(0);
+    }
+}
diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java
@@ -46,6 +46,7 @@
 import hudson.model.Queue.Executable;
 import hudson.model.Queue.WaitingItem;
 import hudson.model.labels.LabelExpression;
+import hudson.model.listeners.SaveableListener;
 import hudson.model.queue.CauseOfBlockage;
 import hudson.model.queue.QueueTaskDispatcher;
 import hudson.model.queue.QueueTaskFuture;
@@ -1018,4 +1019,22 @@ public void testGetCauseOfBlockageForNonConcurrentFreestyle() throws Exception {
 
         assertEquals(expected.getShortDescription(), actual.getShortDescription());
     }
+
+    @Test @LocalData
+    public void load_queue_xml() {
+        Queue q = r.getInstance().getQueue();
+        Queue.Item[] items = q.getItems();
+        assertEquals(Arrays.asList(items).toString(), 11, items.length);
+        assertEquals("Loading the queue should not generate saves", 0, QueueSaveSniffer.count);
+    }
+
+    @TestExtension("load_queue_xml")
+    public static final class QueueSaveSniffer extends SaveableListener {
+        private static int count = 0;
+        @Override public void onChange(Saveable o, XmlFile file) {
+            if (o instanceof Queue) {
+                count++;
+            }
+        }
+    }
 }
diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/config.xml
@@ -0,0 +1,48 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<hudson>
+  <disabledAdministrativeMonitors/>
+  <version>2.103-SNAPSHOT (private-01/18/2018 15:13 GMT-ogondza)</version>
+  <installState class="jenkins.install.InstallState$5">
+    <isSetupComplete>true</isSetupComplete>
+    <name>RESTART</name>
+  </installState>
+  <numExecutors>2</numExecutors>
+  <mode>NORMAL</mode>
+  <useSecurity>true</useSecurity>
+  <authorizationStrategy class="hudson.security.AuthorizationStrategy$Unsecured"/>
+  <securityRealm class="hudson.security.HudsonPrivateSecurityRealm">
+    <disableSignup>true</disableSignup>
+    <enableCaptcha>false</enableCaptcha>
+  </securityRealm>
+  <disableRememberMe>false</disableRememberMe>
+  <projectNamingStrategy class="jenkins.model.ProjectNamingStrategy$DefaultProjectNamingStrategy"/>
+  <workspaceDir>${JENKINS_HOME}/workspace/${ITEM_FULL_NAME}</workspaceDir>
+  <buildsDir>${ITEM_ROOTDIR}/builds</buildsDir>
+  <markupFormatter class="hudson.markup.EscapedMarkupFormatter"/>
+  <jdks/>
+  <viewsTabBar class="hudson.views.DefaultViewsTabBar"/>
+  <myViewsTabBar class="hudson.views.DefaultMyViewsTabBar"/>
+  <clouds/>
+  <scmCheckoutRetryCount>0</scmCheckoutRetryCount>
+  <views>
+    <hudson.model.AllView>
+      <owner class="hudson" reference="../../.."/>
+      <name>all</name>
+      <filterExecutors>false</filterExecutors>
+      <filterQueue>false</filterQueue>
+      <properties class="hudson.model.View$PropertyList"/>
+    </hudson.model.AllView>
+  </views>
+  <primaryView>all</primaryView>
+  <slaveAgentPort>-1</slaveAgentPort>
+  <disabledAgentProtocols>
+    <string>JNLP-connect</string>
+    <string>JNLP2-connect</string>
+  </disabledAgentProtocols>
+  <label></label>
+  <crumbIssuer class="hudson.security.csrf.DefaultCrumbIssuer">
+    <excludeClientIPFromCrumb>false</excludeClientIPFromCrumb>
+  </crumbIssuer>
+  <nodeProperties/>
+  <globalNodeProperties/>
+</hudson>
diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/a/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/a/config.xml
@@ -0,0 +1,18 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<project>
+  <actions/>
+  <description></description>
+  <keepDependencies>false</keepDependencies>
+  <properties/>
+  <scm class="hudson.scm.NullSCM"/>
+  <canRoam>false</canRoam>
+  <assignedNode>!master</assignedNode>
+  <disabled>false</disabled>
+  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
+  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
+  <triggers/>
+  <concurrentBuild>false</concurrentBuild>
+  <builders/>
+  <publishers/>
+  <buildWrappers/>
+</project>
diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/b/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/b/config.xml
@@ -0,0 +1,18 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<project>
+  <actions/>
+  <description></description>
+  <keepDependencies>false</keepDependencies>
+  <properties/>
+  <scm class="hudson.scm.NullSCM"/>
+  <canRoam>false</canRoam>
+  <assignedNode>!master</assignedNode>
+  <disabled>false</disabled>
+  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
+  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
+  <triggers/>
+  <concurrentBuild>false</concurrentBuild>
+  <builders/>
+  <publishers/>
+  <buildWrappers/>
+</project>
diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/c/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/c/config.xml
@@ -0,0 +1,18 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<project>
+  <actions/>
+  <description></description>
+  <keepDependencies>false</keepDependencies>
+  <properties/>
+  <scm class="hudson.scm.NullSCM"/>
+  <canRoam>false</canRoam>
+  <assignedNode>!master</assignedNode>
+  <disabled>false</disabled>
+  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
+  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
+  <triggers/>
+  <concurrentBuild>false</concurrentBuild>
+  <builders/>
+  <publishers/>
+  <buildWrappers/>
+</project>
diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/d/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/d/config.xml
@@ -0,0 +1,18 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<project>
+  <actions/>
+  <description></description>
+  <keepDependencies>false</keepDependencies>
+  <properties/>
+  <scm class="hudson.scm.NullSCM"/>
+  <canRoam>false</canRoam>
+  <assignedNode>!master</assignedNode>
+  <disabled>false</disabled>
+  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
+  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
+  <triggers/>
+  <concurrentBuild>false</concurrentBuild>
+  <builders/>
+  <publishers/>
+  <buildWrappers/>
+</project>
diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/e/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/e/config.xml
@@ -0,0 +1,18 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<project>
+  <actions/>
+  <description></description>
+  <keepDependencies>false</keepDependencies>
+  <properties/>
+  <scm class="hudson.scm.NullSCM"/>
+  <canRoam>false</canRoam>
+  <assignedNode>!master</assignedNode>
+  <disabled>false</disabled>
+  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
+  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
+  <triggers/>
+  <concurrentBuild>false</concurrentBuild>
+  <builders/>
+  <publishers/>
+  <buildWrappers/>
+</project>