From 8c975f11c5a366b1693ed2be04e184a714cf2f84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Fri, 23 Feb 2018 20:30:37 +0100 Subject: [PATCH] [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 72a7529a119b2be6b3ff93d63688ee6973404236) --- core/src/main/java/hudson/model/Queue.java | 78 ++++++- test/pom.xml | 2 +- .../java/hudson/model/QueueRestartTest.java | 89 ++++++++ .../src/test/java/hudson/model/QueueTest.java | 19 ++ .../model/QueueTest/load_queue_xml/config.xml | 48 +++++ .../load_queue_xml/jobs/a/config.xml | 18 ++ .../load_queue_xml/jobs/b/config.xml | 18 ++ .../load_queue_xml/jobs/c/config.xml | 18 ++ .../load_queue_xml/jobs/d/config.xml | 18 ++ .../load_queue_xml/jobs/e/config.xml | 18 ++ .../load_queue_xml/jobs/f/config.xml | 18 ++ .../load_queue_xml/jobs/g/config.xml | 18 ++ .../load_queue_xml/jobs/h/config.xml | 18 ++ .../load_queue_xml/jobs/i/config.xml | 18 ++ .../load_queue_xml/jobs/j/config.xml | 18 ++ .../load_queue_xml/jobs/k/config.xml | 18 ++ .../model/QueueTest/load_queue_xml/queue.xml | 193 ++++++++++++++++++ 17 files changed, 625 insertions(+), 2 deletions(-) create mode 100644 test/src/test/java/hudson/model/QueueRestartTest.java create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/a/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/b/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/c/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/d/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/e/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/f/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/g/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/h/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/i/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/j/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/k/config.xml create mode 100644 test/src/test/resources/hudson/model/QueueTest/load_queue_xml/queue.xml diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 6ad10d5f81..7e59e52117 100644 --- 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.model.queue.WorkUnitContext; 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.NoSuchElementException; 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.Level; 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 class Queue extends ResourceController implements Saveable { public static void init(Jenkins h) { h.getQueue().load(); } + + /** + * Schedule Queue.save() 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 index 2de0eea9ba..80167d7e3f 100644 --- a/test/pom.xml +++ b/test/pom.xml @@ -53,7 +53,7 @@ THE SOFTWARE. ${project.groupId} jenkins-test-harness - 2.32 + 2.33 test diff --git a/test/src/test/java/hudson/model/QueueRestartTest.java b/test/src/test/java/hudson/model/QueueRestartTest.java new file mode 100644 index 0000000000..2ba10b2ddd --- /dev/null +++ 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 index 689b63e5ae..bc18c08e5f 100644 --- 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.BlockedItem; 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 class QueueTest { 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 new file mode 100644 index 0000000000..d18cae81c5 --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/config.xml @@ -0,0 +1,48 @@ + + + + 2.103-SNAPSHOT (private-01/18/2018 15:13 GMT-ogondza) + + true + RESTART + + 2 + NORMAL + true + + + true + false + + false + + ${JENKINS_HOME}/workspace/${ITEM_FULL_NAME} + ${ITEM_ROOTDIR}/builds + + + + + + 0 + + + + all + false + false + + + + all + -1 + + JNLP-connect + JNLP2-connect + + + + false + + + + \ No newline at end of file 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 new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/a/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + 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 new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/b/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + 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 new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/c/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + 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 new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/d/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + 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 new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/e/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/f/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/f/config.xml new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/f/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/g/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/g/config.xml new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/g/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/h/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/h/config.xml new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/h/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/i/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/i/config.xml new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/i/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/j/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/j/config.xml new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/j/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/k/config.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/k/config.xml new file mode 100644 index 0000000000..bbb41344ac --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/jobs/k/config.xml @@ -0,0 +1,18 @@ + + + + + false + + + false + !master + false + false + false + + false + + + + diff --git a/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/queue.xml b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/queue.xml new file mode 100644 index 0000000000..cf854c0522 --- /dev/null +++ b/test/src/test/resources/hudson/model/QueueTest/load_queue_xml/queue.xml @@ -0,0 +1,193 @@ + + + 17 + + + + + + + + 1 + + + + + 17 + k + 1516354024440 + 1516354024440 + false + + + + + + + + 1 + + + + + 16 + j + 1516354003647 + 1516354003648 + false + + + + + + + + 1 + + + + + 15 + i + 1516354001887 + 1516354001888 + false + + + + + + + + 1 + + + + + 14 + h + 1516353999191 + 1516353999192 + false + + + + + + + + 1 + + + + + 13 + g + 1516353998031 + 1516353998032 + false + + + + + + + + 1 + + + + + 12 + f + 1516353996504 + 1516353996504 + false + + + + + + + + 1 + + + + + 11 + e + 1516353995471 + 1516353995472 + false + + + + + + + + 1 + + + + + 10 + d + 1516353993872 + 1516353993873 + false + + + + + + + + 1 + + + + + 9 + c + 1516353991671 + 1516353991672 + false + + + + + + + + 1 + + + + + 8 + b + 1516353989177 + 1516353989177 + false + + + + + + + + 1 + + + + + 7 + a + 1516353986148 + 1516353986151 + false + + + \ No newline at end of file -- GitLab