From 12fd3e47e393fb1942cd5cb29766bcefe62a81f8 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Jul 2016 20:50:54 -0400 Subject: [PATCH] [FIXED JENKINS-27530] Jenkins.reload must also reload the Queue to ensure that every Queue.Item.task corresponds to a live Job, lest nextBuildNumber be bogus. --- core/src/main/java/hudson/model/Job.java | 30 ++++++---------- core/src/main/java/hudson/model/Queue.java | 12 ++++++- core/src/main/java/hudson/model/RunMap.java | 2 +- core/src/main/java/jenkins/model/Jenkins.java | 6 ++-- .../model/lazy/AbstractLazyLoadRunMap.java | 12 +++++-- .../jenkins/model/lazy/LazyBuildMixIn.java | 9 +++++ .../jenkins/model/lazy/SortedIntList.java | 4 +++ .../jenkins/model/lazy/SortedIntListTest.java | 11 ++++++ .../test/java/hudson/model/RunMapTest.java | 36 +++++++++++++++++++ 9 files changed, 97 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java index 1a61359972..ee6cb07a98 100644 --- a/core/src/main/java/hudson/model/Job.java +++ b/core/src/main/java/hudson/model/Job.java @@ -23,8 +23,6 @@ */ package hudson.model; -import com.google.common.base.Function; -import com.google.common.collect.Collections2; import com.infradna.tool.bridge_method_injector.WithBridgeMethods; import hudson.BulkChange; @@ -126,6 +124,8 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; public abstract class Job, RunT extends Run> extends AbstractItem implements ExtensionPoint, StaplerOverridable, ModelObjectWithChildren, OnMaster { + private static final Logger LOGGER = Logger.getLogger(Job.class.getName()); + /** * Next build number. Kept in a separate file because this is the only * information that gets updated often. This allows the rest of the @@ -206,24 +206,16 @@ public abstract class Job, RunT extends Run foldersInt = Collections2.transform(Arrays.asList(folders), new Function() { - public Integer apply(File file) { - return Integer.parseInt(file.getName()); - } - }); - this.nextBuildNumber = Collections.max(foldersInt) + 1; + RunT lB = getLastBuild(); + synchronized (this) { + this.nextBuildNumber = lB != null ? lB.getNumber() + 1 : 1; + } + saveNextBuildNumber(); } - saveNextBuildNumber(); } } else { // From the old Hudson, or doCreateItem. Create this file now. @@ -1247,7 +1239,7 @@ public abstract class Job, RunT extends Run parked = new HashMap(); @@ -2785,6 +2790,11 @@ public class Queue extends ResourceController implements Saveable { this.buildables = new ArrayList(buildables); this.pendings = new ArrayList(pendings); } + + @Override + public String toString() { + return "Queue.Snapshot{waitingList=" + waitingList + ";blockedProjects=" + blockedProjects + ";buildables=" + buildables + ";pendings=" + pendings + "}"; + } } private static class LockedRunnable implements Runnable { diff --git a/core/src/main/java/hudson/model/RunMap.java b/core/src/main/java/hudson/model/RunMap.java index 3142dc7643..33322839cc 100644 --- a/core/src/main/java/hudson/model/RunMap.java +++ b/core/src/main/java/hudson/model/RunMap.java @@ -185,7 +185,7 @@ public final class RunMap> extends AbstractLazyLoadRunMap // Defense against JENKINS-23152 and its ilk. File rootDir = r.getRootDir(); if (rootDir.isDirectory()) { - throw new IllegalStateException(rootDir + " already existed; will not overwrite with " + r); + throw new IllegalStateException("JENKINS-23152: " + rootDir + " already existed; will not overwrite with " + r); } if (!r.getClass().getName().equals("hudson.matrix.MatrixRun")) { // JENKINS-26739: grandfathered in proposeNewNumber(r.getNumber()); diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 3c298d44f6..61f22a4f90 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -44,7 +44,6 @@ import hudson.FilePath; import hudson.Functions; import hudson.Launcher; import hudson.Launcher.LocalLauncher; -import hudson.LocalPluginManager; import hudson.Lookup; import hudson.Main; import hudson.Plugin; @@ -239,7 +238,6 @@ import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; -import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.HttpResponses; @@ -311,6 +309,7 @@ import java.util.logging.Logger; import static hudson.Util.*; import static hudson.init.InitMilestone.*; +import hudson.init.Initializer; import hudson.util.LogTaskListener; import static java.util.logging.Level.*; import static javax.servlet.http.HttpServletResponse.*; @@ -3714,10 +3713,13 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve /** * Reloads the configuration synchronously. + * Beware that this calls neither {@link ItemListener#onLoaded} nor {@link Initializer}s. */ public void reload() throws IOException, InterruptedException, ReactorException { + queue.save(); executeReactor(null, loadTasks()); User.reload(); + queue.load(); servletContext.setAttribute("app", this); } diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java index 25ac5f4dfc..46bd2be841 100644 --- a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java +++ b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java @@ -368,9 +368,17 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i } } + /** + * @return the highest recorded build number, or 0 if there are none + */ + @Restricted(NoExternalUse.class) + public synchronized int maxNumberOnDisk() { + return numberOnDisk.max(); + } + protected final synchronized void proposeNewNumber(int number) throws IllegalStateException { - if (numberOnDisk.isInRange(numberOnDisk.ceil(number))) { - throw new IllegalStateException("cannot create a build with number " + number + " since that (or higher) is already in use among " + numberOnDisk); + if (number <= maxNumberOnDisk()) { + throw new IllegalStateException("JENKINS-27530: cannot create a build with number " + number + " since that (or higher) is already in use among " + numberOnDisk); } } diff --git a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java index b206035c38..6778edf7b2 100644 --- a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java +++ b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java @@ -25,6 +25,7 @@ package jenkins.model.lazy; import hudson.Extension; +import hudson.model.AbstractItem; import hudson.model.Item; import hudson.model.ItemGroup; import hudson.model.Job; @@ -32,6 +33,7 @@ import hudson.model.Queue; import hudson.model.Run; import hudson.model.RunMap; import hudson.model.listeners.ItemListener; +import hudson.model.queue.SubTask; import hudson.widgets.BuildHistoryWidget; import hudson.widgets.HistoryWidget; import java.io.File; @@ -100,6 +102,12 @@ public abstract class LazyBuildMixIn & Queue.Task & @SuppressWarnings("unchecked") public void onLoad(ItemGroup parent, String name) throws IOException { RunMap _builds = createBuildRunMap(); + int max = _builds.maxNumberOnDisk(); + int next = asJob().getNextBuildNumber(); + if (next <= max) { + LOGGER.log(Level.WARNING, "JENKINS-27530: improper nextBuildNumber {0} detected in {1} with highest build number {2}; adjusting", new Object[] {next, asJob(), max}); + asJob().updateNextBuildNumber(max + 1); + } RunMap currentBuilds = this.builds; if (parent != null) { // are we overwriting what currently exist? @@ -121,6 +129,7 @@ public abstract class LazyBuildMixIn & Queue.Task & if (r.isBuilding()) { // Do not use RunMap.put(Run): _builds.put(r.getNumber(), r); + LOGGER.log(Level.FINE, "keeping reloaded {0}", r); } } } diff --git a/core/src/main/java/jenkins/model/lazy/SortedIntList.java b/core/src/main/java/jenkins/model/lazy/SortedIntList.java index 27933fd47e..24f0835f7a 100644 --- a/core/src/main/java/jenkins/model/lazy/SortedIntList.java +++ b/core/src/main/java/jenkins/model/lazy/SortedIntList.java @@ -83,6 +83,10 @@ class SortedIntList extends AbstractList { return size; } + public int max() { + return size > 0 ? data[size - 1] : 0; + } + @Override public boolean add(Integer i) { return add(i.intValue()); diff --git a/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java b/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java index 2146852a5f..9748aeaa18 100644 --- a/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java +++ b/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java @@ -34,4 +34,15 @@ public class SortedIntListTest { assertFalse(l.isInRange(3)); } + @Test public void max() { + SortedIntList l = new SortedIntList(5); + assertEquals(0, l.max()); + l.add(1); + assertEquals(1, l.max()); + l.add(5); + assertEquals(5, l.max()); + l.add(10); + assertEquals(10, l.max()); + } + } diff --git a/test/src/test/java/hudson/model/RunMapTest.java b/test/src/test/java/hudson/model/RunMapTest.java index 13d49dcd60..3d555f2691 100644 --- a/test/src/test/java/hudson/model/RunMapTest.java +++ b/test/src/test/java/hudson/model/RunMapTest.java @@ -1,5 +1,6 @@ package hudson.model; +import hudson.model.queue.QueueTaskFuture; import javax.xml.transform.Source; import javax.xml.transform.stream.StreamSource; import static org.junit.Assert.*; @@ -12,6 +13,7 @@ import org.jvnet.hudson.test.SleepBuilder; public class RunMapTest { @Rule public JenkinsRule r = new JenkinsRule(); + // TODO https://github.com/jenkinsci/jenkins/pull/2438: @Rule public LoggerRule logs = new LoggerRule(); /** * Makes sure that reloading the project while a build is in progress won't clobber that in-progress build. @@ -42,6 +44,40 @@ public class RunMapTest { assertSame(b2.getPreviousBuild(), b1); } + @Issue("JENKINS-27530") + @Test public void reloadWhileBuildIsInQueue() throws Exception { + //logs.record(Queue.class, Level.FINE); + FreeStyleProject p = r.createFreeStyleProject("p"); + p.getBuildersList().add(new SleepBuilder(9999999)); + r.jenkins.setNumExecutors(1); + assertEquals(1, p.scheduleBuild2(0).waitForStart().number); + p.scheduleBuild2(0); + // Note that the bug does not reproduce simply from p.doReload(), since in that case Job identity remains intact: + r.jenkins.reload(); + p = r.jenkins.getItemByFullName("p", FreeStyleProject.class); + FreeStyleBuild b1 = p.getLastBuild(); + assertEquals(1, b1.getNumber()); + /* Currently fails since Run.project is final. But anyway that is not the problem: + assertEquals(p, b1.getParent()); + */ + Queue.Item[] items = Queue.getInstance().getItems(); + assertEquals(1, items.length); + assertEquals(p, items[0].task); // the real issue: assignBuildNumber was being called on the wrong Job + QueueTaskFuture b2f = items[0].getFuture(); + b1.getExecutor().interrupt(); + r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b1)); + FreeStyleBuild b2 = (FreeStyleBuild) b2f.waitForStart(); + assertEquals(2, b2.getNumber()); + assertEquals(p, b2.getParent()); + b2.getExecutor().interrupt(); + r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b2)); + FreeStyleBuild b3 = p.scheduleBuild2(0).waitForStart(); + assertEquals(3, b3.getNumber()); + assertEquals(p, b3.getParent()); + b3.getExecutor().interrupt(); + r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b3)); + } + /** * Testing if the lazy loading can gracefully tolerate a RuntimeException during unmarshalling. */