From 6d2e3ae5cbc57133ba2b46f19031957872ea74b0 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 21 Aug 2014 18:18:25 -0400 Subject: [PATCH 01/23] Initial sketch of replacement of Run.id by number. Does not yet deal with migrating historical records. --- .../main/java/hudson/model/AbstractBuild.java | 1 + core/src/main/java/hudson/model/Build.java | 1 + core/src/main/java/hudson/model/Run.java | 77 +--- core/src/main/java/hudson/model/RunMap.java | 44 -- .../jenkins/diagnostics/ooom/BuildPtr.java | 93 ----- .../ooom/OutOfOrderBuildDetector.java | 58 --- .../ooom/OutOfOrderBuildMonitor.java | 70 ---- .../jenkins/diagnostics/ooom/Problem.java | 302 -------------- .../model/lazy/AbstractLazyLoadRunMap.java | 376 ++++-------------- .../jenkins/model/lazy/LazyBuildMixIn.java | 15 - .../diagnostics/ooom/Messages.properties | 1 - .../OutOfOrderBuildMonitor/message.groovy | 70 ---- .../OutOfOrderBuildMonitor/message.properties | 5 - core/src/test/java/hudson/model/RunTest.java | 31 -- .../lazy/AbstractLazyLoadRunMapTest.java | 169 ++------ .../test/java/jenkins/model/lazy/FakeMap.java | 34 +- .../jenkins/model/lazy/FakeMapBuilder.java | 48 +-- .../ooom/OutOfOrderBuildDetectorTest.groovy | 144 ------- .../buildNumberClash.zip | Bin 4810 -> 0 bytes .../oneProblem.zip | Bin 15427 -> 0 bytes 20 files changed, 139 insertions(+), 1400 deletions(-) delete mode 100644 core/src/main/java/jenkins/diagnostics/ooom/BuildPtr.java delete mode 100644 core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildDetector.java delete mode 100644 core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor.java delete mode 100644 core/src/main/java/jenkins/diagnostics/ooom/Problem.java delete mode 100644 core/src/main/resources/jenkins/diagnostics/ooom/Messages.properties delete mode 100644 core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.groovy delete mode 100644 core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.properties delete mode 100644 test/src/test/groovy/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest.groovy delete mode 100644 test/src/test/resources/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest/buildNumberClash.zip delete mode 100644 test/src/test/resources/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest/oneProblem.zip diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java index d1e291d52e..520e721893 100644 --- a/core/src/main/java/hudson/model/AbstractBuild.java +++ b/core/src/main/java/hudson/model/AbstractBuild.java @@ -167,6 +167,7 @@ public abstract class AbstractBuild

,R extends Abs super(job); } + @Deprecated protected AbstractBuild(P job, Calendar timestamp) { super(job, timestamp); } diff --git a/core/src/main/java/hudson/model/Build.java b/core/src/main/java/hudson/model/Build.java index 99073f1b73..30057fd3e7 100644 --- a/core/src/main/java/hudson/model/Build.java +++ b/core/src/main/java/hudson/model/Build.java @@ -92,6 +92,7 @@ public abstract class Build

,B extends Build> super(project); } + @Deprecated protected Build(P job, Calendar timestamp) { super(job, timestamp); } diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index bbeba83a78..902e473f78 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -146,7 +146,7 @@ public abstract class Run ,RunT extends Run,RunT extends Run,RunT extends Run ID_FORMATTER = new IDFormatterProvider(); - private static final class IDFormatterProvider extends ThreadLocal { - @Override - protected SimpleDateFormat initialValue() { - synchronized (CANONICAL_ID_FORMATTER) { - return (SimpleDateFormat) CANONICAL_ID_FORMATTER.clone(); - } - } - }; + private static final SimpleDateFormat LEGACY_ID_FORMATTER = new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); + // TODO implement migration step; and write out a legacy ID -> number map for use from AbstractLazyLoadRunMap /** * Creates a new {@link Run}. * @param job Owner job */ protected Run(@Nonnull JobT job) throws IOException { - this(job, new GregorianCalendar()); + project = job; + timestamp = System.currentTimeMillis(); + state = State.NOT_STARTED; this.number = project.assignBuildNumber(); + getRootDir().mkdirs(); LOGGER.log(FINER, "new {0} @{1}", new Object[] {this, hashCode()}); } - /** - * Constructor for creating a {@link Run} object in - * an arbitrary state. - */ + @Deprecated protected Run(@Nonnull JobT job, @Nonnull Calendar timestamp) { this(job,timestamp.getTimeInMillis()); } + @Deprecated protected Run(@Nonnull JobT job, long timestamp) { this.project = job; this.timestamp = timestamp; @@ -306,8 +301,9 @@ public abstract class Run ,RunT extends Run,RunT extends Run,RunT extends Run,RunT extends Run,RunT extends Run,RunT extends Run> extends AbstractLazyLoadRunMap return r.getNumber(); } - @Override - protected final String getIdOf(R r) { - return r.getId(); - } - @Override public R put(R r) { return super._put(r); @@ -185,31 +177,6 @@ public final class RunMap> extends AbstractLazyLoadRunMap return r.createReference(); } - @Override - protected FilenameFilter createDirectoryFilter() { - final SimpleDateFormat formatter = Run.ID_FORMATTER.get(); - - return new FilenameFilter() { - @Override public boolean accept(File dir, String name) { - if (name.startsWith("0000")) { - // JENKINS-1461 sometimes create bogus data directories with impossible dates, such as year 0, April 31st, - // or August 0th. Date object doesn't roundtrip those, so we eventually fail to load this data. - // Don't even bother trying. - return false; - } - try { - if (formatter.format(formatter.parse(name)).equals(name)) { - return true; - } - } catch (ParseException e) { - // fall through - } - LOGGER.log(FINE, "Skipping {0} in {1}", new Object[] {name, dir}); - return false; - } - }; - } - @Override protected R retrieve(File d) throws IOException { if(new File(d,"build.xml").exists()) { @@ -221,17 +188,6 @@ public final class RunMap> extends AbstractLazyLoadRunMap LOGGER.log(FINEST, "Loaded " + b.getFullDisplayName() + " in " + Thread.currentThread().getName(), new ThisIsHowItsLoaded()); } return b; - } catch (Run.InvalidDirectoryNameException x) { - Level lvl; - try { - Integer.parseInt(d.getName()); - // JENKINS-15587: just an mangled symlink - lvl = Level.FINE; - } catch (NumberFormatException x2) { - // potentially a real build dir, maybe a bug - lvl = Level.WARNING; - } - LOGGER.log(lvl, "skipping non-build directory {0}", d); } catch (IOException e) { LOGGER.log(Level.WARNING, "could not load " + d, e); } catch (InstantiationError e) { diff --git a/core/src/main/java/jenkins/diagnostics/ooom/BuildPtr.java b/core/src/main/java/jenkins/diagnostics/ooom/BuildPtr.java deleted file mode 100644 index 8d269ea634..0000000000 --- a/core/src/main/java/jenkins/diagnostics/ooom/BuildPtr.java +++ /dev/null @@ -1,93 +0,0 @@ -package jenkins.diagnostics.ooom; - -import hudson.FilePath; -import hudson.model.Job; -import hudson.model.TaskListener; - -import java.io.File; -import java.io.IOException; - -/** - * ID and build number of one build. - */ -public final class BuildPtr implements Comparable { - final Job job; - - final File buildDir; - /** - * Timestamp build ID. - */ - public final String id; - /** - * Build number found from the disk. - */ - public final int n; - - /** - * Position of this build according to the ordering induced by {@link #n} - */ - int posByN; - - /** - * Position of this build according to the ordering induced by {@link #id} - */ - int posByID; - - - BuildPtr(Job job, File buildDir, int n) { - this.job = job; - this.n = n; - this.id = buildDir.getName(); - this.buildDir = buildDir; - } - - @Override - public String toString() { - return buildDir.toString()+":#"+n; - } - - - /** - * If this build and that build are inconsistent, in that - * their numbers and timestamps are ordering in the wrong direction. - */ - public boolean isInconsistentWith(BuildPtr that) { - return signOfCompare(this.posByN,that.posByN) * signOfCompare(this.posByID,that.posByID) < 0; - } - - /** - * sign of (a-b). - */ - private static int signOfCompare(int a, int b) { - if (a>b) return 1; - if (a "+dst); - if (!buildDir.renameTo(dst)) { - FilePath bd = new FilePath(buildDir); - bd.copyRecursiveTo(new FilePath(dst)); - bd.deleteRecursive(); - } - - // if there's a symlink delete it - new File(buildDir.getParentFile(),String.valueOf(n)).delete(); - } - - @Override - public int compareTo(BuildPtr that) { - return this.id.compareTo(that.id); - } -} diff --git a/core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildDetector.java b/core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildDetector.java deleted file mode 100644 index a42736146f..0000000000 --- a/core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildDetector.java +++ /dev/null @@ -1,58 +0,0 @@ -package jenkins.diagnostics.ooom; - -import hudson.Extension; -import hudson.model.AsyncPeriodicWork; -import hudson.model.Job; -import hudson.model.TaskListener; -import hudson.util.TimeUnit2; -import jenkins.model.Jenkins; - -import javax.inject.Inject; -import java.io.IOException; - -/** - * Discovers {@link Problem}s periodically in the background and - * pass them on to {@link OutOfOrderBuildMonitor}. - * - * @author Kohsuke Kawaguchi - */ -@Extension -public class OutOfOrderBuildDetector extends AsyncPeriodicWork { - @Inject - private OutOfOrderBuildMonitor monitor; - - - public OutOfOrderBuildDetector() { - super("Out of order build detection"); - } - - @Override - protected void execute(TaskListener listener) throws IOException, InterruptedException { - execute(listener, 10*1000); - } - - /** - * Performs the check synchronously. - * - * @param delay - * delay in the number of milli-seconds to reduce the load on I/O. - */ - public void execute(TaskListener listener, int delay) throws InterruptedException { - for (Job j : Jenkins.getInstance().getAllItems(Job.class)) { - listener.getLogger().println("Scanning " + j.getFullName()); - - Problem p = Problem.find(j); - if (p!=null) { - monitor.addProblem(p); - listener.getLogger().println(" found problems: "+p); - } - - Thread.sleep(delay); - } - } - - @Override - public long getRecurrencePeriod() { - return TimeUnit2.DAYS.toMillis(1); - } -} diff --git a/core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor.java b/core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor.java deleted file mode 100644 index abf500f190..0000000000 --- a/core/src/main/java/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor.java +++ /dev/null @@ -1,70 +0,0 @@ -package jenkins.diagnostics.ooom; - -import hudson.Extension; -import hudson.model.TaskListener; -import hudson.util.HttpResponses; -import jenkins.management.AsynchronousAdministrativeMonitor; -import org.kohsuke.stapler.HttpResponse; -import org.kohsuke.stapler.interceptor.RequirePOST; - -import java.io.File; -import java.util.Collections; -import java.util.LinkedHashSet; -import java.util.Set; - -/** - * Reports any {@link Problem}s found and report them in the "Manage Jenkins" page. - * - * @author Kohsuke Kawaguchi - */ -@Extension -public class OutOfOrderBuildMonitor extends AsynchronousAdministrativeMonitor { - private final Set problems = Collections.synchronizedSet(new LinkedHashSet()); - - @Override - public boolean isActivated() { - return !problems.isEmpty() || getLogFile().exists(); - } - - void addProblem(Problem p) { - problems.add(p); - } - - public Set getProblems() { - return Collections.unmodifiableSet(new LinkedHashSet(problems)); - } - - @RequirePOST - public HttpResponse doFix() { - start(false); - return HttpResponses.forwardToPreviousPage(); - } - - /** - * Discards the current log file so that the "stuff is completed" message will be gone. - */ - @RequirePOST - public HttpResponse doDismiss() { - getLogFile().delete(); - return HttpResponses.forwardToPreviousPage(); - } - - @Override - public String getDisplayName() { - return "Fix Out-of-order Builds"; - } - - @Override - public File getLogFile() { - return super.getLogFile(); - } - - @Override - protected void fix(TaskListener listener) throws Exception { - Set problems = getProblems(); - for (Problem problem : problems) { - problem.fix(listener); - } - this.problems.removeAll(problems); - } -} diff --git a/core/src/main/java/jenkins/diagnostics/ooom/Problem.java b/core/src/main/java/jenkins/diagnostics/ooom/Problem.java deleted file mode 100644 index 662328dff5..0000000000 --- a/core/src/main/java/jenkins/diagnostics/ooom/Problem.java +++ /dev/null @@ -1,302 +0,0 @@ -package jenkins.diagnostics.ooom; - -import hudson.AbortException; -import hudson.Util; -import hudson.model.AbstractProject; -import hudson.model.Job; -import hudson.model.Run; -import hudson.model.TaskListener; -import jenkins.model.lazy.AbstractLazyLoadRunMap; -import org.xml.sax.InputSource; - -import javax.xml.xpath.XPathConstants; -import javax.xml.xpath.XPathExpression; -import javax.xml.xpath.XPathExpressionException; -import javax.xml.xpath.XPathFactory; -import java.io.File; -import java.io.IOException; -import java.text.ParseException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.SortedMap; -import java.util.TreeMap; -import java.util.TreeSet; -import java.util.logging.Logger; - -import static java.util.logging.Level.*; - -/** - * Look at build numbers in build.xml and compare them with build IDs (timestamps). - * - * When they are inconsistent (newer build timestamp-wise has an older build number), - * it'll confuse the binary search in {@link AbstractLazyLoadRunMap}, so detect them and report them. - * - * @author Kohsuke Kawaguchi - */ -public final class Problem { - public final Job job; - - /** - * A smallest set of builds whose removals would correct the order - * inconsistency problem. - */ - private final Set offenders = new TreeSet(); - - /** - * Scans the inconsistencies and return the information about it. - * - * @return null if no problems were found. - */ - public static Problem find(Job j) { - Problem p = new Problem(j); - if (p.countInconsistencies()==0) return null; - return p; - } - - private Problem(Job j) { - this.job = j; - new Inspector().inspect(); - } - - public Set getOffenders() { - return Collections.unmodifiableSet(offenders); - } - - /** - * Number of inconsistencies, which is the number of builds whose IDs - * have to be messed around on disk to collect problems. - */ - public int countInconsistencies() { - return offenders.size(); - } - - /** - * Equality is based on the job. - */ - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - - Problem problem = (Problem) o; - - return job.equals(problem.job); - } - - @Override - public int hashCode() { - return job.hashCode(); - } - - @Override - public String toString() { - return job.getFullName() + " " + Util.join(offenders); - } - - public void fix(TaskListener listener) throws IOException, InterruptedException { - listener.getLogger().println("Fixing problems in " + job.getFullName()); - for (BuildPtr o : offenders) { - o.fix(listener); - } - - if (job instanceof AbstractProject) { - // let all the current references go and build a new one - AbstractProject a = (AbstractProject) job; - a._getRuns().purgeCache(); - } - } - - /** - * Finds the problems and builds up the data model of {@link Problem}. - */ - class Inspector { - /** - * All the builds sorted by their {@link BuildPtr#n} - */ - private List byN; - /** - * All the builds sorted by their {@link BuildPtr#id} - */ - private List byId; - - private final XPathExpression xpath; - - Inspector() { - try { - xpath = XPathFactory.newInstance().newXPath().compile("/*/number/text()"); - } catch (XPathExpressionException e) { - throw new AssertionError(e); - } - } - - /** - * Simply report inconsistencies in the ordering. - */ - protected void inspect() { - Map builds = scan(); - - byN = new ArrayList(builds.values()); - // this is already sorted by BuildPtr.n - int i=0; - for (BuildPtr b : byN) { - b.posByN = i++; - } - - byId = new ArrayList(byN); - Collections.sort(byId); - i=0; - for (BuildPtr b : byId) { - b.posByID = i++; - } - - while (true) { - BuildPtr b = pick(); - if (b==null) - break; - offenders.add(b); - } - } - - /** - * Find the most inconsistent build, a build whose removal - * would reduce the # of inconsistencies by the most. - * - * This process takes {@link #offenders} into account. - * - * @return null if there's no more build to remove. - */ - private BuildPtr pick() { - BuildPtr worst=null; - int worstScore=0; - - for (BuildPtr b : byN) { - if (offenders.contains(b)) - continue; - - int score = score(b); - if (score>worstScore) { - worst = b; - worstScore = score; - } - } - - return worst; - } - - /** - * Count the number of other builds the given build is inconsistent with, - * excluding inconsistencies with {@link #offenders} (since those inconsistencies - * are already effectively resolved by fixing offenders.) - */ - private int score(BuildPtr b) { - int i=0; - for (BuildPtr a : byN) { - if (offenders.contains(a)) - continue; - if (a.isInconsistentWith(b)) - i++; - } - return i; - } - - /** - * Looks at the builds directory of the given job and builds up the full map of build number to its ID. - */ - protected SortedMap scan() { - LOGGER.fine("Inspecting "+job); - - SortedMap builds = new TreeMap(); - - File[] files = job.getBuildDir().listFiles(); - if (files==null) return builds; - - for (File build : files) { - try { - LOGGER.finer("Inspecting " + build); - - if (isInt(build.getName())) { - // if this is a number, then it must be a build number - String s = loadBuildNumberFromBuildXml(build); - if (!s.equals(build.getName())) { - LOGGER.warning(build+" contains build number "+s); - // this index is invalid. - if (build.delete()) { - // index should be a symlink, and if so we can just delete it without losing data. - LOGGER.info("Removed problematic index "+build); - } else { - // the deltion will fail if 'build' isn't just a symlink but an actual directory. - // That is good, as we don't want to delete any real data. - LOGGER.warning("Couldn't delete " + build); - } - } - continue; - } - - - if (isID(build.getName())) { - String bn = loadBuildNumberFromBuildXml(build); - if (bn==null) { - LOGGER.log(WARNING, "Failed to parse "+build); - continue; - } - - int n; - try { - n = Integer.parseInt(bn); - } catch (NumberFormatException e) { - LOGGER.log(WARNING, "Expected number in " + build + " but found " + bn, e); - continue; - } - - BuildPtr b = new BuildPtr(job,build,n); - - BuildPtr o = builds.put(n, b); - if (o != null) { - LOGGER.log(WARNING, "Multiple builds have the same number: {0} vs. {1}", new Object[] {o, b}); - offenders.add(b.compareTo(o) > 0 ? o : b); - } - } - } catch (XPathExpressionException e) { - LOGGER.log(WARNING, "Failed to inspect "+build, e); - } catch (AbortException e) { - LOGGER.log(WARNING, "Failed to inspect "+build+": "+e.getMessage()); - } catch (IOException e) { - LOGGER.log(WARNING, "Failed to inspect "+build, e); - } - } - - return builds; - } - - private boolean isInt(String s) { - try { - Integer.parseInt(s); - return true; - } catch (NumberFormatException e) { - return false; - } - } - - private boolean isID(String s) { - try { - Run.ID_FORMATTER.get().parse(s); - return true; - } catch (ParseException e) { - return false; - } - } - - private String loadBuildNumberFromBuildXml(File dir) throws XPathExpressionException, IOException { - File buildXml = new File(dir, "build.xml"); - if (!buildXml.exists()) - throw new AbortException(buildXml+" doesn't exist"); - String systemId = buildXml.toURI().toURL().toExternalForm(); - return (String)xpath.evaluate(new InputSource(systemId), XPathConstants.STRING); - } - } - - private static final Logger LOGGER = Logger.getLogger(Problem.class.getName()); -} diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java index 33ba98c598..b1c12800fc 100644 --- a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java +++ b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java @@ -27,7 +27,6 @@ import hudson.model.Job; import hudson.model.Run; import hudson.model.RunMap; import java.io.File; -import java.io.FilenameFilter; import java.io.IOException; import java.util.AbstractMap; import java.util.ArrayList; @@ -44,8 +43,6 @@ import java.util.logging.Logger; import javax.annotation.CheckForNull; import static jenkins.model.lazy.AbstractLazyLoadRunMap.Direction.*; -import static jenkins.model.lazy.Boundary.*; -import org.apache.commons.collections.keyvalue.DefaultMapEntry; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -60,26 +57,16 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; * requested, this class {@link #retrieve(File) retrieves them} on demand, one by one. * *

- * The lookup is primarily done by using the build number as the key (hence the key type is {@link Integer}), - * but this class also provides look up based on {@linkplain #getIdOf(Object) the build ID}. + * The lookup is done by using the build number as the key (hence the key type is {@link Integer}). * *

* This class makes the following assumption about the on-disk layout of the data: * *

* *

- * On certain platforms, there are symbolic links named after build numbers that link to the build ID. - * If these are available, they are used as a hint to speed up the lookup. Otherwise - * we rely on the assumption above and perform a binary search to locate the build. - * (notice that we'll have to do linear search if we don't have the consistent ordering assumption, - * which robs the whole point of doing lazy loading.) - * - *

* Some of the {@link SortedMap} operations are weakly implemented. For example, * {@link #size()} may be inaccurate because we only count the number of directories that look like * build records, without checking if they are loadable. But these weaknesses aren't distinguishable @@ -93,7 +80,7 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; *

* Object lock of {@code this} is used to make sure mutation occurs sequentially. * That is, ensure that only one thread is actually calling {@link #retrieve(File)} and - * updating {@link jenkins.model.lazy.AbstractLazyLoadRunMap.Index#byNumber} and {@link jenkins.model.lazy.AbstractLazyLoadRunMap.Index#byId}. + * updating {@link jenkins.model.lazy.AbstractLazyLoadRunMap.Index#byNumber}. * * @author Kohsuke Kawaguchi * @since 1.485 @@ -112,8 +99,10 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i private LazyLoadRunMapEntrySet entrySet = new LazyLoadRunMapEntrySet(this); /** - * Pair of two maps into a single class, so that the changes can be made visible atomically, - * and updates can happen concurrently to read. + * Historical holder for map. + * + * TODO all this mess including {@link #numberOnDisk} could probably be simplified to a single {@code TreeMap>} + * where a null value means not yet loaded and a broken entry just uses {@code NoHolder}. * * The idiom is that you put yourself in a synchronized block, {@linkplain #copy() make a copy of this}, * update the copy, then set it to {@link #index}. @@ -121,63 +110,24 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i private class Index { /** * Stores the mapping from build number to build, for builds that are already loaded. - */ - private final TreeMap> byNumber; - - /** - * Stores the build ID to build number for builds that we already know. * * If we have known load failure of the given ID, we record that in the map * by using the null value (not to be confused with a non-null {@link BuildReference} * with null referent, which just means the record was GCed.) */ - private final TreeMap> byId; + private final TreeMap> byNumber; private Index() { - byId = new TreeMap>(); byNumber = new TreeMap>(COMPARATOR); } private Index(Index rhs) { - byId = new TreeMap>(rhs.byId); byNumber = new TreeMap>(rhs.byNumber); } - - /** - * Returns the build record #M (<=n) - */ - private Map.Entry> ceilingEntry(int n) { -// switch to this once we depend on JDK6 -// return byNumber.ceilingEntry(n); - - Set>> s = byNumber.tailMap(n).entrySet(); - if (s.isEmpty()) return null; - else return s.iterator().next(); - } - - /** - * Returns the build record #M (>=n) - */ - // >= and not <= because byNumber is in the descending order - private Map.Entry> floorEntry(int n) { -// switch to this once we depend on JDK6 -// return byNumber.floorEntry(n); - - SortedMap> sub = byNumber.headMap(n); - if (sub.isEmpty()) return null; - Integer k = sub.lastKey(); - return new DefaultMapEntry(k,sub.get(k)); - } } /** - * Build IDs found as directories, in the ascending order. - */ - // copy on write - private volatile SortedList idOnDisk = new SortedList(Collections.emptyList()); - - /** - * Build number shortcuts found on disk, in the ascending order. + * Build numbers found on disk, in the ascending order. */ // copy on write private volatile SortedIntList numberOnDisk = new SortedIntList(0); @@ -199,7 +149,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i assert this.dir==null; this.dir = dir; if (dir!=null) - loadIdOnDisk(); + loadNumberOnDisk(); } /** @@ -230,31 +180,23 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i public synchronized void purgeCache() { index = new Index(); fullyLoaded = false; - loadIdOnDisk(); + loadNumberOnDisk(); } - private void loadIdOnDisk() { + private void loadNumberOnDisk() { String[] kids = dir.list(); if (kids == null) { // the job may have just been created kids = EMPTY_STRING_ARRAY; } - List buildDirs = new ArrayList(); - FilenameFilter buildDirFilter = createDirectoryFilter(); SortedIntList list = new SortedIntList(kids.length / 2); for (String s : kids) { - if (buildDirFilter.accept(dir, s)) { - buildDirs.add(s); - } else { - try { - list.add(Integer.parseInt(s)); - } catch (NumberFormatException e) { - // this isn't a shortcut - } + try { + list.add(Integer.parseInt(s)); + } catch (NumberFormatException e) { + // this isn't a build dir } } - Collections.sort(buildDirs); - idOnDisk = new SortedList(buildDirs); list.sort(); numberOnDisk = list; } @@ -263,13 +205,9 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i return COMPARATOR; } - /** - * If we have non-zero R in memory, we can return false right away. - * If we have zero R in memory, try loading one and see if we can find something. - */ @Override public boolean isEmpty() { - return index.byId.isEmpty() && search(Integer.MAX_VALUE, DESC)==null; + return search(Integer.MAX_VALUE, DESC)==null; } @Override @@ -349,7 +287,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i } public R get(int n) { - return search(n, Direction.EXACT); + return getByNumber(n); } /** @@ -367,184 +305,55 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i * If DESC, finds the closest #M that satisfies M<=N. */ public @CheckForNull R search(final int n, final Direction d) { - Entry> c = index.ceilingEntry(n); - if (c!=null && c.getKey()== n) { - R r = c.getValue().get(); - if (r!=null) - return r; // found the exact #n - } - - // at this point we know that we don't have #n loaded yet - - {// check numberOnDisk as a cache to see if we can find it there - int npos = numberOnDisk.find(n); - if (npos>=0) {// found exact match - R r = load(numberOnDisk.get(npos), null); - if (r!=null) - return r; - } - - switch (d) { - case ASC: - case DESC: - // didn't find the exact match, but what's the nearest ascending value in the cache? - int neighbor = (d==ASC?HIGHER:LOWER).apply(npos); - if (numberOnDisk.isInRange(neighbor)) { - R r = getByNumber(numberOnDisk.get(neighbor)); - if (r!=null) { - // make sure that the cache is accurate by looking at the previous ID - // and it actually satisfies the constraint - int prev = (d==ASC?LOWER:HIGHER).apply(idOnDisk.find(getIdOf(r))); - if (idOnDisk.isInRange(prev)) { - R pr = getById(idOnDisk.get(prev)); - // sign*sign is making sure that #pr and #r sandwiches #n. - if (pr!=null && signOfCompare(getNumberOf(pr),n)*signOfCompare(n,getNumberOf(r))>0) - return r; - else { - // cache is lying. there's something fishy. - // ignore the cache and do the slow search - } - } else { - // r is the build with youngest ID - return r; - } - } else { - // cache says we should have a build but we didn't. - // ignore the cache and do the slow search - } - } - break; - case EXACT: - // fall through - } - - // didn't find it in the cache, but don't give up yet - // maybe the cache just doesn't exist. - // so fall back to the slow search - } - - // capture the snapshot and work off with it since it can be overwritten by other threads - SortedList idOnDisk = this.idOnDisk; - boolean clonedIdOnDisk = false; // if we modify idOnDisk we need to write it back. this flag is set to true when we overwrit idOnDisk local var - - // slow path: we have to find the build from idOnDisk by guessing ID of the build. - // first, narrow down the candidate IDs to try by using two known number-to-ID mapping - if (idOnDisk.isEmpty()) return null; - - Entry> f = index.floorEntry(n); - - // if bound is null, use a sentinel value - String cid = c==null ? "\u0000" : c.getValue().id; - String fid = f==null ? "\uFFFF" : f.getValue().id; - // at this point, #n must be in (cid,fid) - - // We know that the build we are looking for exists in [lo,hi) --- it's "hi)" and not "hi]" because we do +1. - // we will narrow this down via binary search - final int initialSize = idOnDisk.size(); - int lo = idOnDisk.higher(cid); - int hi = idOnDisk.lower(fid)+1; - - final int initialLo = lo, initialHi = hi; - - if (!(0<=lo && lo<=hi && hi<=idOnDisk.size())) { - // assertion error, but we are so far unable to get to the bottom of this bug. - // but don't let this kill the loading the hard way - String msg = String.format( - "JENKINS-15652 Assertion error #1: failing to load %s #%d %s: lo=%d,hi=%d,size=%d,size2=%d", - dir, n, d, lo, hi, idOnDisk.size(), initialSize); - LOGGER.log(Level.WARNING, msg); - return null; - } - - while (lo(idOnDisk); - clonedIdOnDisk = true; - } - idOnDisk.remove(pivot); - continue; - } - - int found = getNumberOf(r); - if (found==n) - return r; // exact match - - if (found=idOnDisk.size()) { - // assertion error, but we are so far unable to get to the bottom of this bug. - // but don't let this kill the loading the hard way - LOGGER.log(Level.WARNING, String.format( - "JENKINS-15652 Assertion error #3: failing to load %s #%d %s: lo=%d,hi=%d,size=%d (initial:lo=%d,hi=%d,size=%d)", - dir, n,d,lo,hi,idOnDisk.size(), initialLo,initialHi,initialSize)); - return null; - } - return getById(idOnDisk.get(lo-1)); case EXACT: - if (hi<=0) return null; - R r = getById(idOnDisk.get(hi-1)); - if (r==null) return null; - - int found = getNumberOf(r); - if (found==n) - return r; // exact match + return getByNumber(n); + case ASC: + for (int m : numberOnDisk) { + if (m < n) { + // TODO could be made more efficient with numberOnDisk.find + continue; + } + R r = getByNumber(m); + if (r != null) { + return r; + } + } + return null; + case DESC: + // TODO again could be made more efficient + List reversed = new ArrayList(numberOnDisk); + Collections.reverse(reversed); + for (int m : reversed) { + if (m > n) { + continue; + } + R r = getByNumber(m); + if (r != null) { + return r; + } + } return null; default: throw new AssertionError(); } } - /** - * sign of (a-b). - */ - private static int signOfCompare(int a, int b) { - if (a>b) return 1; - if (a ref = snapshot.byId.get(id); + if (snapshot.byNumber.containsKey(n)) { + BuildReference ref = snapshot.byNumber.get(n); if (ref==null) return null; // known failure R v = unwrap(ref); if (v!=null) return v; // already in memory // otherwise fall through to load } - return load(id,null); - } - - public R getByNumber(int n) { - return search(n, Direction.EXACT); + return load(n, null); } public R put(R value) { @@ -557,27 +366,13 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i @Override public synchronized R put(Integer key, R r) { - String id = getIdOf(r); int n = getNumberOf(r); Index copy = copy(); BuildReference ref = createReference(r); - BuildReference old = copy.byId.put(id,ref); - copy.byNumber.put(n,ref); + BuildReference old = copy.byNumber.put(n,ref); index = copy; - /* - search relies on the fact that every object added via - put() method be available in the xyzOnDisk index, so I'm adding them here - however, this is awfully inefficient. I wonder if there's any better way to do this? - */ - if (!idOnDisk.contains(id)) { - ArrayList a = new ArrayList(idOnDisk); - a.add(id); - Collections.sort(a); - idOnDisk = new SortedList(a); - } - if (!numberOnDisk.contains(n)) { SortedIntList a = new SortedIntList(numberOnDisk); a.add(n); @@ -596,9 +391,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i public synchronized void putAll(Map rhs) { Index copy = copy(); for (R r : rhs.values()) { - String id = getIdOf(r); BuildReference ref = createReference(r); - copy.byId.put(id,ref); copy.byNumber.put(getNumberOf(r),ref); } index = copy; @@ -618,9 +411,9 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i synchronized (this) { if (!fullyLoaded) { Index copy = copy(); - for (String id : idOnDisk) { - if (!copy.byId.containsKey(id)) - load(id,copy); + for (Integer number : numberOnDisk) { + if (!copy.byNumber.containsKey(number)) + load(number, copy); } index = copy; fullyLoaded = true; @@ -638,42 +431,18 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i } /** - * Tries to load the record #N by using the shortcut. + * Tries to load the record #N. * * @return null if the data failed to load. */ protected R load(int n, Index editInPlace) { - R r = null; - File shortcut = new File(dir,String.valueOf(n)); - if (shortcut.isDirectory()) { - synchronized (this) { - r = load(shortcut,editInPlace); - - // make sure what we actually loaded is #n, - // because the shortcuts can lie. - if (r!=null && getNumberOf(r)!=n) - r = null; - - if (r==null) { - // if failed to locate, record that fact - SortedIntList update = new SortedIntList(numberOnDisk); - update.removeValue(n); - numberOnDisk = update; - } - } - } - return r; - } - - - protected R load(String id, Index editInPlace) { assert dir != null; - R v = load(new File(dir, id), editInPlace); + R v = load(new File(dir, String.valueOf(n)), editInPlace); if (v==null && editInPlace!=null) { // remember the failure. // if editInPlace==null, we can create a new copy for this, but not sure if it's worth doing, - // given that we also update idOnDisk anyway. - editInPlace.byId.put(id,null); + // TODO should we also update numberOnDisk? + editInPlace.byNumber.put(n, null); } return v; } @@ -690,9 +459,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i Index copy = editInPlace!=null ? editInPlace : new Index(index); - String id = getIdOf(r); BuildReference ref = createReference(r); - copy.byId.put(id,ref); copy.byNumber.put(getNumberOf(r),ref); if (editInPlace==null) index = copy; @@ -708,10 +475,11 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i * Subtype to provide {@link Run#getNumber()} so that this class doesn't have to depend on it. */ protected abstract int getNumberOf(R r); - /** - * Subtype to provide {@link Run#getId()} so that this class doesn't have to depend on it. - */ - protected abstract String getIdOf(R r); + + private String getIdOf(R r) { + // TODO see comment in getById + return String.valueOf(getNumberOf(r)); + } /** * Allow subtype to capture a reference. @@ -735,11 +503,10 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i public synchronized boolean removeValue(R run) { Index copy = copy(); int n = getNumberOf(run); - copy.byNumber.remove(n); + BuildReference old = copy.byNumber.remove(n); SortedIntList a = new SortedIntList(numberOnDisk); a.removeValue(n); numberOnDisk = a; - BuildReference old = copy.byId.remove(getIdOf(run)); this.index = copy; return unwrap(old)!=null; @@ -751,9 +518,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i public synchronized void reset(TreeMap builds) { Index index = new Index(); for (R r : builds.values()) { - String id = getIdOf(r); BuildReference ref = createReference(r); - index.byId.put(id,ref); index.byNumber.put(getNumberOf(r),ref); } @@ -770,14 +535,9 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i return o==this; } - /** - * Lists the actual data directory - */ - protected abstract FilenameFilter createDirectoryFilter(); - - private static final Comparator COMPARATOR = new Comparator() { - public int compare(Comparable o1, Comparable o2) { - return -o1.compareTo(o2); + private static final Comparator COMPARATOR = new Comparator() { + @Override public int compare(Integer o1, Integer o2) { + return o2 - o1; } }; diff --git a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java index 095a7358cb..333ae269d4 100644 --- a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java +++ b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java @@ -63,9 +63,6 @@ public abstract class LazyBuildMixIn & Queue.Task & @SuppressWarnings("deprecation") // [JENKINS-15156] builds accessed before onLoad or onCreatedFromScratch called private @Nonnull RunMap builds = new RunMap(); - // keep track of the previous time we started a build - private long lastBuildStartTime; - /** * Initializes this mixin. * Call this from a constructor and {@link AbstractItem#onLoad} to make sure it is always initialized. @@ -169,19 +166,7 @@ public abstract class LazyBuildMixIn & Queue.Task & * Calls the ({@link Job}) constructor of {@link #getBuildClass}. * Suitable for {@link SubTask#createExecutable}. */ - @SuppressWarnings("SleepWhileHoldingLock") - @edu.umd.cs.findbugs.annotations.SuppressWarnings("SWL_SLEEP_WITH_LOCK_HELD") public final synchronized RunT newBuild() throws IOException { - // make sure we don't start two builds in the same second - // so the build directories will be different too - long timeSinceLast = System.currentTimeMillis() - lastBuildStartTime; - if (timeSinceLast < 1000) { - try { - Thread.sleep(1000 - timeSinceLast); - } catch (InterruptedException e) { - } - } - lastBuildStartTime = System.currentTimeMillis(); try { RunT lastBuild = getBuildClass().getConstructor(asJob().getClass()).newInstance(asJob()); builds.put(lastBuild); diff --git a/core/src/main/resources/jenkins/diagnostics/ooom/Messages.properties b/core/src/main/resources/jenkins/diagnostics/ooom/Messages.properties deleted file mode 100644 index 94f27977b1..0000000000 --- a/core/src/main/resources/jenkins/diagnostics/ooom/Messages.properties +++ /dev/null @@ -1 +0,0 @@ -Problem.DisplayName={0} builds have inconsistent timestamps in {1} \ No newline at end of file diff --git a/core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.groovy b/core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.groovy deleted file mode 100644 index 27f62dde93..0000000000 --- a/core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.groovy +++ /dev/null @@ -1,70 +0,0 @@ -/* -The MIT License - -Copyright (c) 2013, CloudBees, 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 jenkins.diagnostics.ooom.OutOfOrderBuildMonitor - -import jenkins.diagnostics.ooom.BuildPtr -import jenkins.diagnostics.ooom.Problem; - -def f = namespace(lib.FormTagLib) - -if (my.isFixingActive()) { - div(class:"info") { - raw _("inProgress",my.url) - } -} else if (my.logFile.exists()) { - form(method:"POST",action:"${my.url}/dismiss",name:"dismissOutOfOrderBuilds") { - raw _("completed",my.url) - f.submit(name:"dismiss",value:_("Dismiss this message")) - } -} - -if (!my.problems.isEmpty()) { - form(method:"POST",action:"${my.url}/fix",name:"fixOutOfOrderBuilds") { - div(class:"warning") { - raw _("buildsAreOutOfOrder") - } - ul { - my.problems.each { Problem p -> - li { - - raw(_("problem", - p.countInconsistencies(), - p.job.fullDisplayName, - rootURL+'/'+p.job.url)) - - text(" : ") - p.offenders.each { BuildPtr o -> - a(href:rootURL+'/'+p.job.url+'/'+o.n, "#${o.n}") - - raw(" ") - } - } - } - } - - div(align:"right") { - f.submit(name:"fix",value:_("Correct those problems by moving offending records to a backup folder")) - } - } -} diff --git a/core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.properties b/core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.properties deleted file mode 100644 index e401c26551..0000000000 --- a/core/src/main/resources/jenkins/diagnostics/ooom/OutOfOrderBuildMonitor/message.properties +++ /dev/null @@ -1,5 +0,0 @@ -problem={0} builds in {1} -buildsAreOutOfOrder=Some projects have builds whose timestamps are inconsistent. \ - These will confuse Jenkins when it tries to look up build records. -inProgress=Out-of-order builds are being swept under the carpet. You can check the log. -completed=Out-of-order builds were swept under the carpet. You can check the log. \ No newline at end of file diff --git a/core/src/test/java/hudson/model/RunTest.java b/core/src/test/java/hudson/model/RunTest.java index 8ee207cbd3..95a436d16e 100644 --- a/core/src/test/java/hudson/model/RunTest.java +++ b/core/src/test/java/hudson/model/RunTest.java @@ -24,12 +24,7 @@ package hudson.model; -import hudson.Util; import hudson.model.Run.Artifact; -import hudson.util.StreamTaskListener; - -import java.io.File; -import java.util.Date; import java.util.List; import java.util.TimeZone; import java.util.concurrent.Callable; @@ -87,32 +82,6 @@ public class RunTest { } - @Bug(15587) - @Test - public void testParseTimestampFromBuildDir() throws Exception { - //Assume.assumeTrue(!Functions.isWindows() || (NTFS && JAVA7) || ...); - - String buildDateTime = "2012-12-21_04-02-28"; - int buildNumber = 155; - - StreamTaskListener l = StreamTaskListener.fromStdout(); - - File tempDir = Util.createTempDir(); - File buildDir = new File(tempDir, buildDateTime); - assertEquals(true, buildDir.mkdir()); - File buildDirSymLink = new File(tempDir, Integer.toString(buildNumber)); - - try { - buildDir.mkdir(); - - Util.createSymlink(tempDir, buildDir.getAbsolutePath(), buildDirSymLink.getName(), l); - long time = Run.parseTimestampFromBuildDir(buildDirSymLink); - assertEquals(buildDateTime, Run.ID_FORMATTER.get().format(new Date(time))); - } finally { - Util.deleteRecursive(tempDir); - } - } - private List.Artifact> createArtifactList(String... paths) throws Exception { Run r = new Run(new StubJob(), 0) {}; Run.ArtifactList list = r.new ArtifactList(); diff --git a/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java b/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java index 55bd0bdfde..bb336880ea 100644 --- a/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java +++ b/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java @@ -66,7 +66,7 @@ public class AbstractLazyLoadRunMapTest extends Assert { return new FakeMap(getDir()) { @Override protected BuildReference createReference(Build r) { - return new BuildReference(getIdOf(r), /* pretend referent expired */ null); + return new BuildReference(Integer.toString(r.n), /* pretend referent expired */ null); } }; } @@ -80,7 +80,7 @@ public class AbstractLazyLoadRunMapTest extends Assert { @Before public void setUp() throws Exception { - a = aBuilder.add(1, "A").add(3, "B").add(5, "C").make(); + a = aBuilder.add(1).add(3).add(5).make(); b = bBuilder.make(); } @@ -88,11 +88,11 @@ public class AbstractLazyLoadRunMapTest extends Assert { @Test public void lookup() { assertNull(a.get(0)); - a.get(1).asserts(1, "A"); + a.get(1).asserts(1); assertNull(a.get(2)); - a.get(3).asserts(3, "B"); + a.get(3).asserts(3); assertNull(a.get(4)); - a.get(5).asserts(5, "C"); + a.get(5).asserts(5); assertNull(a.get(6)); assertNull(b.get(1)); @@ -108,8 +108,8 @@ public class AbstractLazyLoadRunMapTest extends Assert { @Test public void idempotentLookup() { for (int i=0; i<5; i++) { - a.get(1).asserts(1,"A"); - a.get((Object)1).asserts(1, "A"); + a.get(1).asserts(1); + a.get((Object)1).asserts(1); } } @@ -153,24 +153,24 @@ public class AbstractLazyLoadRunMapTest extends Assert { @Bug(19418) @Test public void searchExactWhenIndexedButSoftReferenceExpired() throws IOException { - final FakeMap m = localExpiredBuilder.add(1, "A").add(2, "B").make(); + final FakeMap m = localExpiredBuilder.add(1).add(2).make(); // force index creation m.entrySet(); - m.search(1, Direction.EXACT).asserts(1, "A"); + m.search(1, Direction.EXACT).asserts(1); assertNull(m.search(3, Direction.EXACT)); assertNull(m.search(0, Direction.EXACT)); } @Bug(22681) @Test public void exactSearchShouldNotReload() throws Exception { - FakeMap m = localBuilder.add(1, "A").add(2, "B").make(); + FakeMap m = localBuilder.add(1).add(2).make(); assertNull(m.search(0, Direction.EXACT)); Build a = m.search(1, Direction.EXACT); - a.asserts(1, "A"); + a.asserts(1); Build b = m.search(2, Direction.EXACT); - b.asserts(2, "B"); + b.asserts(2); assertNull(m.search(0, Direction.EXACT)); assertSame(a, m.search(1, Direction.EXACT)); assertSame(b, m.search(2, Direction.EXACT)); @@ -186,82 +186,19 @@ public class AbstractLazyLoadRunMapTest extends Assert { */ @Test public void unloadableData() throws IOException { - FakeMap m = localBuilder.add(1, "A").addUnloadable("B").add(5, "C").make(); + FakeMap m = localBuilder.add(1).addUnloadable(3).add(5).make(); assertNull(m.search(3, Direction.EXACT)); - m.search(3,Direction.DESC).asserts(1, "A"); - m.search(3, Direction.ASC ).asserts(5, "C"); + m.search(3,Direction.DESC).asserts(1); + m.search(3, Direction.ASC ).asserts(5); } @Test public void eagerLoading() throws IOException { Map.Entry[] b = a.entrySet().toArray(new Map.Entry[3]); - ((Build)b[0].getValue()).asserts(5, "C"); - ((Build)b[1].getValue()).asserts(3, "B"); - ((Build)b[2].getValue()).asserts(1, "A"); - } - - @Test - public void fastLookup() throws IOException { - FakeMap a = localBuilder.addBoth(1, "A").addBoth(3, "B").addBoth(5, "C"). make(); - - a.get(1).asserts(1,"A"); - assertNull(a.get(2)); - a.get(3).asserts(3, "B"); - assertNull(a.get(4)); - a.get(5).asserts(5, "C"); - } - - @Test - public void fastSearch() throws IOException { - FakeMap a = localBuilder.addBoth(1, "A").addBoth(3, "B").addBoth(5, "C").addBoth(7, "D").make(); - - // we should be using the cache to find the entry efficiently - a.search(6, Direction.ASC).asserts(7, "D"); - a.search(2, Direction.DESC).asserts(1, "A"); - } - - @Test - public void bogusCache() throws IOException { - FakeMap a = localBuilder.addUnloadableCache(1).make(); - assertNull(a.get(1)); - } - - @Test - public void bogusCacheAndHiddenRealData() throws IOException { - FakeMap a = localBuilder.addUnloadableCache(1).add(1, "A").make(); - a.get(1).asserts(1, "A"); - } - - @Test - public void bogusCache2() throws IOException { - FakeMap a = localBuilder.addBogusCache(1, 3, "A").make(); - assertNull(a.get(1)); - a.get(3).asserts(3,"A"); - } - - @Test - public void incompleteCache() throws IOException { - FakeMapBuilder setup = localBuilder.addBoth(1, "A").add(3, "B").addBoth(5, "C"); - - // each test uses a fresh map since cache lookup causes additional loads - // to verify the results - - // if we just rely on cache, - // it'll pick up 5:C as the first ascending value, - // but we should be then verifying this by loading B, so in the end we should - // find the correct value - setup.make().search(2, Direction.ASC).asserts(3,"B"); - setup.make().search(4, Direction.DESC).asserts(3,"B"); - - // variation of the cache based search where we find the outer-most value via cache - setup.make().search(0, Direction.ASC).asserts(1,"A"); - setup.make().search(6, Direction.DESC).asserts(5,"C"); - - // variation of the cache search where the cache tells us that we are searching - // in the direction that doesn't have any records - assertNull(setup.make().search(0, Direction.DESC)); - assertNull(setup.make().search(6, Direction.ASC)); + ((Build)b[0].getValue()).asserts(5); + ((Build)b[1].getValue()).asserts(3); + ((Build)b[2].getValue()).asserts(1); } @Test @@ -271,8 +208,8 @@ public class AbstractLazyLoadRunMapTest extends Assert { Build[] b = m.values().toArray(new Build[2]); assertEquals(2, b.length); - b[0].asserts(5, "C"); - b[1].asserts(3, "B"); + b[0].asserts(5); + b[1].asserts(3); } @Test @@ -287,77 +224,55 @@ public class AbstractLazyLoadRunMapTest extends Assert { @Test public void indexOutOfBounds() throws Exception { FakeMapBuilder f = localBuilder; - f.add(100,"A") - .addUnloadable("B") - .addUnloadable("C") - .addUnloadable("D") - .addUnloadable("E") - .addUnloadable("F") - .addUnloadable("G") - .add(200,"H") - .add(201, "I"); + f.add(100) + .addUnloadable(150) + .addUnloadable(151) + .addUnloadable(152) + .addUnloadable(153) + .addUnloadable(154) + .addUnloadable(155) + .add(200) + .add(201); FakeMap map = f.make(); Build x = map.search(Integer.MAX_VALUE, Direction.DESC); assert x.n==201; } - @Bug(15652) - @Test public void outOfOrder() throws Exception { - FakeMap map = localBuilder - .add( 4, "2012-A") - .add( 5, "2012-B") - .add( 6, "2012-C") - .add( 7, "2012-D") - .add( 8, "2012-E") - .add( 9, "2012-F") - .add(10, "2012-G") - .add(11, "2012-H") - .add(12, "2012-I") - .add( 1, "2013-A") - .add( 7, "2013-B") - .add( 9, "2013-C") - .add(10, "2013-D") - .add(11, "2013-E") - .make(); - map.entrySet(); // forces Index to be populated - assertNull(map.search(3, Direction.DESC)); - } - @Bug(18065) @Test public void all() throws Exception { assertEquals("[]", a.getLoadedBuilds().keySet().toString()); Set> entries = a.entrySet(); assertEquals("[]", a.getLoadedBuilds().keySet().toString()); assertFalse(entries.isEmpty()); - assertEquals("5 since it is the latest, and 3 because, well, .search and pivot is weird", "[5, 3]", a.getLoadedBuilds().keySet().toString()); - assertEquals(5, a.getById("C").n); - assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString()); - assertEquals("A", a.getByNumber(1).id); - assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString()); + assertEquals("5 since it is the latest", "[5]", a.getLoadedBuilds().keySet().toString()); + assertEquals(5, a.getById("5").n); + assertEquals("[5]", a.getLoadedBuilds().keySet().toString()); + assertEquals(1, a.getByNumber(1).n); + assertEquals("[5, 1]", a.getLoadedBuilds().keySet().toString()); a.purgeCache(); assertEquals("[]", a.getLoadedBuilds().keySet().toString()); Iterator> iterator = entries.iterator(); - assertEquals("iterator starts off checking for newest build, so for this crazy logic see above", "[5, 3]", a.getLoadedBuilds().keySet().toString()); + assertEquals("[5]", a.getLoadedBuilds().keySet().toString()); assertTrue(iterator.hasNext()); - assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString()); + assertEquals("[5]", a.getLoadedBuilds().keySet().toString()); Map.Entry entry = iterator.next(); assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString()); assertEquals(5, entry.getKey().intValue()); assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString()); - assertEquals("C", entry.getValue().id); + assertEquals(5, entry.getValue().n); assertEquals("[5, 3]", a.getLoadedBuilds().keySet().toString()); assertTrue(iterator.hasNext()); entry = iterator.next(); assertEquals(3, entry.getKey().intValue()); assertEquals(".next() precomputes the one after that too", "[5, 3, 1]", a.getLoadedBuilds().keySet().toString()); - assertEquals("B", entry.getValue().id); + assertEquals(3, entry.getValue().n); assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString()); assertTrue(iterator.hasNext()); entry = iterator.next(); assertEquals(1, entry.getKey().intValue()); assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString()); - assertEquals("A", entry.getValue().id); + assertEquals(1, entry.getValue().n); assertEquals("[5, 3, 1]", a.getLoadedBuilds().keySet().toString()); assertFalse(iterator.hasNext()); } @@ -375,7 +290,7 @@ public class AbstractLazyLoadRunMapTest extends Assert { assertTrue(itr.hasNext()); Entry e = itr.next(); assertEquals((Integer)5,e.getKey()); - e.getValue().asserts(5, "C"); + e.getValue().asserts(5); // now that the first entry is returned, we expect there to be two loaded assertTrue(a.getLoadedBuilds().size() < 3); @@ -384,7 +299,7 @@ public class AbstractLazyLoadRunMapTest extends Assert { assertTrue(itr.hasNext()); e = itr.next(); assertEquals((Integer)3, e.getKey()); - e.getValue().asserts(3,"B"); + e.getValue().asserts(3); // repeat the process for the third one assertTrue(a.getLoadedBuilds().size() <= 3); @@ -393,7 +308,7 @@ public class AbstractLazyLoadRunMapTest extends Assert { assertTrue(itr.hasNext()); e = itr.next(); assertEquals((Integer) 1, e.getKey()); - e.getValue().asserts(1,"A"); + e.getValue().asserts(1); assertFalse(itr.hasNext()); assertEquals(3, a.getLoadedBuilds().size()); diff --git a/core/src/test/java/jenkins/model/lazy/FakeMap.java b/core/src/test/java/jenkins/model/lazy/FakeMap.java index ecb0648c17..a74c24733f 100644 --- a/core/src/test/java/jenkins/model/lazy/FakeMap.java +++ b/core/src/test/java/jenkins/model/lazy/FakeMap.java @@ -26,7 +26,6 @@ package jenkins.model.lazy; import org.apache.commons.io.FileUtils; import java.io.File; -import java.io.FilenameFilter; import java.io.IOException; /** @@ -42,50 +41,27 @@ public class FakeMap extends AbstractLazyLoadRunMap { return build.n; } - @Override - protected String getIdOf(Build build) { - return build.id; - } - - @Override - protected FilenameFilter createDirectoryFilter() { - return new FilenameFilter() { - public boolean accept(File dir, String name) { - try { - Integer.parseInt(name); - return false; - } catch (NumberFormatException e) { - return true; - } - } - }; - } - @Override protected Build retrieve(File dir) throws IOException { String n = FileUtils.readFileToString(new File(dir, "n")).trim(); - String id = FileUtils.readFileToString(new File(dir, "id")).trim(); - //new Exception("loading " + id + " #" + n).printStackTrace(); - return new Build(Integer.parseInt(n),id); + //new Exception("loading #" + n).printStackTrace(); + return new Build(Integer.parseInt(n)); } } class Build { final int n; - final String id; - Build(int n, String id) { + Build(int n) { this.n = n; - this.id = id; } - public void asserts(int n, String id) { + public void asserts(int n) { assert this.n==n; - assert this.id.equals(id); } @Override public String toString() { - return "Build #" + n + " [" + id + "] @" + hashCode(); + return "Build #" + n + " @" + hashCode(); } } \ No newline at end of file diff --git a/core/src/test/java/jenkins/model/lazy/FakeMapBuilder.java b/core/src/test/java/jenkins/model/lazy/FakeMapBuilder.java index 870d0cf2d5..4013736cb8 100644 --- a/core/src/test/java/jenkins/model/lazy/FakeMapBuilder.java +++ b/core/src/test/java/jenkins/model/lazy/FakeMapBuilder.java @@ -46,59 +46,19 @@ public class FakeMapBuilder implements TestRule { public FakeMapBuilder() { } - public FakeMapBuilder add(int n, String id) throws IOException { - verifyId(id); - File build = new File(dir,id); - build.mkdir(); + public FakeMapBuilder add(int n) throws IOException { + File build = new File(dir, Integer.toString(n)); FileUtils.writeStringToFile(new File(build, "n"), Integer.toString(n)); - FileUtils.writeStringToFile(new File(build,"id"),id); - return this; - } - - /** - * Adds a symlink from n to build id. - * - * (in test we should ideally create a symlink, but we fake the test - * by actually making it a directory and staging the same data.) - */ - public FakeMapBuilder addCache(int n, String id) throws IOException { - return addBogusCache(n,n,id); - } - - public FakeMapBuilder addBogusCache(int label, int actual, String id) throws IOException { - verifyId(id); - File build = new File(dir,Integer.toString(label)); build.mkdir(); - FileUtils.writeStringToFile(new File(build, "n"), Integer.toString(actual)); - FileUtils.writeStringToFile(new File(build,"id"),id); return this; } - public FakeMapBuilder addBoth(int n, String id) throws IOException { - return add(n,id).addCache(n,id); - } - - private void verifyId(String id) { - try { - Integer.parseInt(id); - throw new IllegalMonitorStateException("ID cannot be a number"); - } catch (NumberFormatException e) { - // OK - } - } - /** * Adds a build record under the givn ID but make it unloadable, * which will cause a failure when a load is attempted on this build ID. */ - public FakeMapBuilder addUnloadable(String id) throws IOException { - File build = new File(dir,id); - build.mkdir(); - return this; - } - - public FakeMapBuilder addUnloadableCache(int n) throws IOException { - File build = new File(dir,String.valueOf(n)); + public FakeMapBuilder addUnloadable(int n) throws IOException { + File build = new File(dir, Integer.toString(n)); build.mkdir(); return this; } diff --git a/test/src/test/groovy/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest.groovy b/test/src/test/groovy/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest.groovy deleted file mode 100644 index 00b0bd97b9..0000000000 --- a/test/src/test/groovy/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest.groovy +++ /dev/null @@ -1,144 +0,0 @@ -package jenkins.diagnostics.ooom - -import com.gargoylesoftware.htmlunit.html.HtmlPage -import hudson.model.FreeStyleProject -import hudson.model.Job -import hudson.model.TaskListener -import hudson.util.StreamTaskListener -import static org.junit.Assert.*; -import org.junit.Rule -import org.junit.Test -import org.jvnet.hudson.test.Bug -import org.jvnet.hudson.test.JenkinsRule -import org.jvnet.hudson.test.recipes.LocalData - -import javax.inject.Inject - -/** - * - * - * @author Kohsuke Kawaguchi - */ -class OutOfOrderBuildDetectorTest { - @Rule - public JenkinsRule j = new JenkinsRule(); - - @Inject - OutOfOrderBuildDetector oobd; - - @Inject - OutOfOrderBuildMonitor oobm; - - /** - * The test case looks like [#3,#1,#2,#4] and we should find #3 to be the problem. - */ - @Test @LocalData - public void oneProblem() { - j.jenkins.injector.injectMembers(this); - - Job dt = j.jenkins.getItem("deletion-test"); - - def p = Problem.find(dt); - assert p.offenders.size()==1; - def b = (p.offenders as List)[0]; - - assert b.id=="2013-07-29_17-09-09" - assert b.n==3 - - def l = StreamTaskListener.fromStdout() - - // find all the problems now - oobd.execute(l,0); - - // verify that the monitor is activated - assert oobm.isActivated(); - assert oobm.problems.size()==1; - - def wc = j.createWebClient() - - // at this point there should be a link to start a fix but not the button to dismiss - def manage = wc.goTo("manage") - assertNoForm(manage,"dismissOutOfOrderBuilds") - j.submit(manage.getFormByName("fixOutOfOrderBuilds")); - - // give it a break until the fix is complete - while (oobm.isFixingActive()) - Thread.sleep(100); - - // there should be a log file now - oobm.getLogFile().exists() - wc.goTo("administrativeMonitor/${oobm.class.name}/log") - - // at this point the UI should change to show a dismiss action - manage = wc.goTo("manage") - assertNoForm(manage,"fixOutOfOrderBuilds") - j.submit(manage.getFormByName("dismissOutOfOrderBuilds")); - - // that should stop the alarm and there should be no more forms - assert !oobm.isActivated(); - manage = wc.goTo("manage") - assertNoForm(manage,"fixOutOfOrderBuilds") - assertNoForm(manage,"dismissOutOfOrderBuilds") - - // verify that the problem is actually fixed - assert dt.getBuildByNumber(1)!=null; - assert dt.getBuildByNumber(2)!=null; - assert dt.getBuildByNumber(3)==null; - assert dt.getBuildByNumber(4)!=null; - assert Problem.find(dt)==null; - - // and there should be a backup - assert new File(dt.rootDir,"outOfOrderBuilds/2013-07-29_17-09-09/build.xml").exists() - } - - def assertNoForm(HtmlPage p, String name) { - def forms = p.documentElement.getElementsByAttribute("form", "name", name); - assert forms.size()==0; - } - - /** - * If there's no problem, it shouldn't find it. - */ - @Test - public void thereShouldBeNoFailure() { - def f = j.createFreeStyleProject() - j.assertBuildStatusSuccess(f.scheduleBuild2(0)); - j.assertBuildStatusSuccess(f.scheduleBuild2(0)); - j.assertBuildStatusSuccess(f.scheduleBuild2(0)); - assert Problem.find(f)==null; - } - - @Bug(22631) - @LocalData - @Test public void buildNumberClash() throws Exception { - j.jenkins.injector.injectMembers(this); - FreeStyleProject p = j.jenkins.getItemByFullName("problematic", FreeStyleProject.class); - assertNotNull(p); - ByteArrayOutputStream baos = new ByteArrayOutputStream(); - TaskListener l = new StreamTaskListener(baos); - oobd.execute(l, 0); - String log = baos.toString(); - String idA = "2014-04-15_11-22-11"; - String idB = "2014-04-15_11-22-14"; - assertEquals(idB, p.getBuildByNumber(2).getId()); - assertEquals(3, p.getLastBuild().getNumber()); - File dir = p.getBuildDir(); - File buildDirA = new File(dir, idA); - File buildDirB = new File(dir, idB); - BuildPtr b2A = new BuildPtr(p, buildDirA, 2); - BuildPtr b2B = new BuildPtr(p, buildDirB, 2); - String expected = "[" + b2A + "]"; - assertTrue("Should see " + expected + " in:\n" + log, log.contains(expected)); - baos = new ByteArrayOutputStream(); - l = new StreamTaskListener(baos); - oobm.fix(l); - log = baos.toString(); - assertTrue(buildDirB.isDirectory()); - assertTrue("Should see " + buildDirA + " in:\n" + log, log.contains(buildDirA.toString())); - File dest = new File(new File(p.getRootDir(), "outOfOrderBuilds"), idA); - assertTrue("Should see " + dest + " in:\n" + log, log.contains(dest.toString())); - assertFalse(buildDirA.isDirectory()); - assertTrue(dest.isDirectory()); - } - -} diff --git a/test/src/test/resources/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest/buildNumberClash.zip b/test/src/test/resources/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest/buildNumberClash.zip deleted file mode 100644 index 64751bbe30b8205f7d74a760931f1c201d13fa34..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 4810 zcmeHL&u<$=6rQvRiVRfZQla)@^-v$cUax;NiL}|4+9|36sTDUuqDW=EJF&Oj-C51d zw&{((fE-$JfitQo2#Et1#HB}$RsV=e+&F;*h&N-eXX_n1*tw;(wbss$_syG`Z@ziY zo$b@h1%y@+di>d88)ZK4pk;K2^q3J-(hG3taNjll{SBec*yQP_hwUx8_VD=UA7Ar40s2-+%6Q0 z)?JL_HjZ)RK~Enud*2Bdwv1f8=nRH2rX0E%(wMnn(G48NHZKekkCCVXHI$9~O8tHxjuCr8wf!tJEdfk(a8a2Vy2;!#L>$J{ye7JvmS6#IU>q z+Y+RZd!)$lE%4{;>cw-XfCSc|k`gP|`wN!Y`*QBd}eCiwTR&R#mg3}y9X!s`7Q zR<)|xC|4V0vw7Pz%e7kBtY(n@x^>w8{iogU&VcjcY0ilxU7LsI(m?XmaXFj_Cf;EW zM^p`ohl5JXNf`b>whx=3gDdJry{A~*F7Gi+uY1y5l0kMfvZj_XYb49acx6R$yKK`k zMsI>Mj60Oy^h0c$^^LXWTC-lSURqzX4AmgIQOuHn+uglZYpdI}j8R!Mc?ktsA(6cS z$m4=&aR&}%nA=hnEF&$7CUNvXBI5&=7Dba-Qhp<{EhBvswbNywHE^On2C$A8ji!>2 z1TpnFOSQu#VWm$i5T@ zs+pnc-$2!izkGFVF{+w!al8{w)!I>1H8<8b7NKhW6`<-ZAlTPmJ`3SaN8%~X&_cLh zc~|oPs<>VuWwJCcMz*MzvX8i1483CtaI=JPuWB*)U=S zkY4OYUA(D=a8~9SLPVd+d0qUeke|$ntaV-& zfv6$gg8wUWY#6Q2L_Cu9TNjWh(A^oJ`9&AuiCPg|fTEDUnG;#NBDyO|4e<{g=xHmG zb6#Fp%6Z+DrI0^-b3SCP^E%N|Lp*+a`VewEUzV`W_R1T=_zZlXE+g~@m?QKb9>YYY diff --git a/test/src/test/resources/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest/oneProblem.zip b/test/src/test/resources/jenkins/diagnostics/ooom/OutOfOrderBuildDetectorTest/oneProblem.zip deleted file mode 100644 index 86e801dd38a0af3048e6eee0736e08506c03d0ad..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 15427 zcmeHO2{@G78z07!NJX-QYO)@J-_$--uJxcogF$zy2S|awJv;IW8#;o zAM^-bgtNOfR&tji69Os2Hlhzcp8J^*G;|-42*kvX9b`YwKK*f9I~O~gle?QZ&JK&4 z<6u0b2ebBca)N0Z4#}Co9rv4Uo9=FnN~OZGW@JbW|3H6r&3A1P4Tg0ghPgE=xpAdj_j_KvQsF!H z2diGFhJSHGc*{CorudvU=-x4v#mqVU@lT@GjQa1pT(eeDVc4o8d)4!AqlG03lGn#@ z@lhk_u=cP(&%t+=7dYQZJ;}0+DwmVA*N#8(y9eor{ z#cZ$pX4NaEQ8KWSz>9i)6|E0+xe-~iwwN`Z14M=Li6I~R8c2<~@Z2cFdwfPjzx?Nua(*9@z2^ZyN@Ir78*Lc>}6Y*l|D3B+=r6^{Fy zYdIdQI8nK+ise~Wme6+Jjow(Shr38J<}KYlq@zM+>CiR9J-PaQ=VIErx+F_C8AXK) zSz8zLKWV}~N1chRQ`?$RV({_X_83NF?C|;{*{1h;1}-1s7jS&N#X#0WI&bKL?Vh#1 zR-bxwTeXyr=xjCq{ar&Y^ZgYT>zo6mnw)|PR|Pjbysst#Zr_Y&~G5WQ+4v z#5k#3^QhM1*I{kiUi$+55{qO%{}|`}=D^e5#4g_|Q9aiYu97#pSsonvn*)R74<^f} zY+JOQ=2Bu%e*RCP6}WVXQDa;=;&22Tc6W9Nn*Ib|pyCz!q@N9%@&|{ko?5fAn_8=L z9b$`S9_@(fVG>j>f5>obOMR$R+;_wb;e^T|0n3Aol(ek46k1$H(Lx$6E~O|gJy$J3 zQ;oBrF8?@nQKH%jewp4#Qw_bKK>wQxBtOe+Mg$0Ly7!I>ot?^Ns8t|&NvHy?Czsz; zxxJQJR@AZnm}wJ#N@Uw#PRz0DmMN}BH!`g$toOgN@j6PCMfLrcKAkSBjvYhyE?&I& zrhP-fn7HO(P=yyJW2I)4B%7fr`l7ROaPJ45ulx4>F8Sc8cXAS^wg#7XQcZ7gJFwKj?O*J zeXCR8rL~V4hjIJWYhSaAP+)?i-hKSiuvhV&w7s@BElsA!`j7eZI(z;0rSvy3~Fty6yYE;@Iv5eJj30>}C7uw#G;<6rF#afih z1jv0@+vRcF@PX}8^OM8zsVmE~5|YI~ux)-; zwCG0MMNQMH^xE4TKzhTVQm?VbyWyg8zS_vn1C?I1dtG@q%HMwFkeiuw1TVb-eE>hb zp;mz8C5Z(H^aBzZ31PwmLgObq)C!QihnT(q7amk4MuLnQ}3-IQ=bmgcXu{FZHS5S%vF zp=W1q&aS{L{UmX??1giodCM*izi_S8FDHhPoI13^Y#etSDv~c1n_2aRZ%+MP`TO%F zhwRHxey28zV!HS_1f`O=RGh0DkgZ$Z6>vBvRr_`;9lLa#)6gvKXayg4KRG^Rfh6v@K5zo%L ztD!sdA60g88JumlkYJaH&P%Hr*ts@BW&heb-zz*UC5MAppO^o>5VtpX&zupW;sg%GMmkHtNXchn^G{ai$Ov zh76kuSIntgku1*pwFe7!OJpskby$0aJJ(zR<67Tccnu{rC{o*;eb|zFQ^FR*(Rl9Q z(_g%f7i;lcW*z7^Ko7+*1=9)fTy{+LH9U9snDx+K+Z~pMtUKOelz3dXFQaeS>6PX? zJTo?xXxXp%P#)a8XVD!?Q&CSxzPPVvWP2D{RDbYG=mxt;Tcp&M_Zkeh?mK&MQ}mZe z#ciMWieRHVG-KMv>JP>r{F$d+xl2{AGCFqXheynxrRip7r_V1A?D9fHipwG>W0DcdYQesZgloEk9IuETlT=rOL*z0mf~~GIT!D4T@~ngT;dCNdRpg^ z8EpXx$JZA4+Kns;B>_pFg_3|k_(}p_yOI4{H^>aXpd0@Y-H@B5ZV;v^@Ks+q0^L|h zO8)yOp2@iaYmz1ylGVy`0@sfgG5TIWzK$FFg+LPlNalRFaEkI90HBb=YBOMlO z+}-S*W_lUIkt6y;#1+<1eKmvfH7i=aiRMehZ0q@OCdIoEn-qhzpn%I zV%(1tSCy26G4}r0Ti1i+6%8=UOLJe(AQEDz;8oi>Jn$j2apXeXrq8TpBdaPJnUAU* zr9bx~_2^f0CR3yRePJvM=1h$*P>fwc57WZyckoW%zMi??Cq$_Xp=9e zwX3>zc+hB~dQ>>G?)${h{3gcB?LM^1(Ru=XN1w$6eLMF&;f`m+!yi|j+qY^qr|t{s z`*tH%eR&*msKM~S;p#Dl&RjZIuH0m&(mg6lZoQGt7X~$3W-1C`zHY3jf&_Yh5a5F% zy=rcD`*9O0t?TJ(ZRau3rvawyB$iHo@yzJ+Y(0=toaMzIVId4YC=j(c065T~uuvNN zzb}o!#{^O?KpYduRI>=ryoALv_?SQndO9YM0bDo*Q+W!eq2))CYYXK9R1SPCL)=vd z-=~^H=_i{+i5f)755Ide53>-pw;0eJ3n@1^+PpHDYkn!Kbjt@3} z0SI6tDaoaoY4wA)kxniR5QY)Cej9ZePTQ5(PSY z;h+fiI2_!JWza%6K3oI^xQTrY2Qq0BAwd!lf_UiRY$B4AiGDrmDML?TPEzJiH6~sF zrED-EiAoUgKt^f}o;R)1kO|N(qILqmgy=XX>R`dH1j?VpfdHx}FikXB*xBr8K31yio@hUzOf=!G z4s=H!ry3Kwqrq$+hG-BR|7H@pqfafP)?i|y$xdLBXu^vJ=#HjaPVEGyi6#p>Ni<>E z3mRMF+2CNoYkq)MlHW@K(~e2Q2+uZx5UCoXfB+>C%r9X`1flRr6oo*w1M?v-6-1ZK zfMsOiW-KEgTXi|$;id^93o;vm>Q=&mfcL4C2_oEgP=Ypa!XrQlqTp?Wy9`PYKQ}zo zy!X^B?~bMOBLR{$b;%LmX8`k=S`(J z#Of}pz~@LTG^;(jnxee~b^E!H3ti)nRAbJIU}y~NSxc?K^ClS@107J*PJklOlR8dZ zdBxAMXaq>Y;@4pTjRotlFp&uqK4_T|9|A2%5`w5`3E)r=Blwe8Bx#%pC&5Y*CCd}# zO8}F?;h8YNLw$CfYD}olQV5TPAs_0q2oaJC3u7>`&yt-0=_CN^v%XYMVA^L%!qS6Z Scm%%j2*gbwZ{iRG|N0jdT1aL9 From c0d48fc116ee6357b35a4e3f408623b1533f364c Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 10:22:10 -0400 Subject: [PATCH 02/23] Fixed compatibility with MatrixRun. The Run constructors taking a timestamp must not be deprecated. And the root directory must be created in RunMap.put, not the constructor, in case the number (and hence directory name) is changed after the constructor completes. --- core/src/main/java/hudson/model/Run.java | 15 +++++++++++---- core/src/main/java/hudson/model/RunMap.java | 1 + 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 0c9614fb33..824ae12a81 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -47,6 +47,7 @@ import hudson.model.Descriptor.FormException; import hudson.model.Run.RunExecution; import hudson.model.listeners.RunListener; import hudson.model.listeners.SaveableListener; +import hudson.model.queue.SubTask; import hudson.search.SearchIndexBuilder; import hudson.security.ACL; import hudson.security.AccessControlled; @@ -108,6 +109,7 @@ import jenkins.model.PeepholePermalink; import jenkins.model.RunAction2; import jenkins.model.StandardArtifactManager; import jenkins.model.lazy.BuildReference; +import jenkins.model.lazy.LazyBuildMixIn; import jenkins.util.VirtualFile; import jenkins.util.io.OnMaster; import net.sf.json.JSONObject; @@ -280,21 +282,26 @@ public abstract class Run ,RunT extends RunMay be used in a {@link SubTask#createExecutable} (instead of calling {@link LazyBuildMixIn#newBuild}). + * For example, {@code MatrixConfiguration.newBuild} does this + * so that the {@link #timestamp} as well as {@link #number} are shared with the parent build. + */ protected Run(@Nonnull JobT job, @Nonnull Calendar timestamp) { this(job,timestamp.getTimeInMillis()); } - @Deprecated + /** @see #Run(Job, Calendar) */ protected Run(@Nonnull JobT job, long timestamp) { this.project = job; this.timestamp = timestamp; this.state = State.NOT_STARTED; - getRootDir().mkdirs(); } /** diff --git a/core/src/main/java/hudson/model/RunMap.java b/core/src/main/java/hudson/model/RunMap.java index 3bebd0ece1..1c73f1b0e7 100644 --- a/core/src/main/java/hudson/model/RunMap.java +++ b/core/src/main/java/hudson/model/RunMap.java @@ -163,6 +163,7 @@ public final class RunMap> extends AbstractLazyLoadRunMap @Override public R put(R r) { + r.getRootDir().mkdirs(); return super._put(r); } From 2ca95503eba72dd0d3e0232d76509576aeea3d5f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 10:33:54 -0400 Subject: [PATCH 03/23] =?UTF-8?q?Implemented=20@daniel-beck=E2=80=99s=20su?= =?UTF-8?q?ggestion=20for=20defense=20against=20build=20number=20duplicati?= =?UTF-8?q?on.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/src/main/java/hudson/model/RunMap.java | 11 ++++++++++- .../main/java/jenkins/model/lazy/LazyBuildMixIn.java | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/model/RunMap.java b/core/src/main/java/hudson/model/RunMap.java index 1c73f1b0e7..262c6355e9 100644 --- a/core/src/main/java/hudson/model/RunMap.java +++ b/core/src/main/java/hudson/model/RunMap.java @@ -161,9 +161,18 @@ public final class RunMap> extends AbstractLazyLoadRunMap return r.getNumber(); } + /** + * Add a new build to the map. + * Do not use when loading existing builds (use {@link #put(Integer, Object)}). + */ @Override public R put(R r) { - r.getRootDir().mkdirs(); + // Defense against JENKINS-23152 and its ilk. + File rootDir = r.getRootDir(); + if (rootDir.isDirectory()) { + throw new IllegalStateException(rootDir + " already existed; will not overwite with " + r); + } + rootDir.mkdirs(); return super._put(r); } diff --git a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java index 333ae269d4..e188f193a9 100644 --- a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java +++ b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java @@ -120,7 +120,8 @@ public abstract class LazyBuildMixIn & Queue.Task & // if we are reloading, keep all those that are still building intact for (RunT r : currentBuilds.getLoadedBuilds().values()) { if (r.isBuilding()) { - _builds.put(r); + // Do not use RunMap.put(Run): + _builds.put(r.getNumber(), r); } } } From 1ede3e983386830118bd4e891c76c64f119317f7 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 11:00:39 -0400 Subject: [PATCH 04/23] Defend against out-of-order builds even if nextBuildNumber is wrong. --- core/src/main/java/hudson/model/RunMap.java | 1 + .../model/lazy/AbstractLazyLoadRunMap.java | 6 +++++ .../jenkins/model/lazy/SortedIntListTest.java | 22 ++++++++++++++++++- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/RunMap.java b/core/src/main/java/hudson/model/RunMap.java index 262c6355e9..00cba471c1 100644 --- a/core/src/main/java/hudson/model/RunMap.java +++ b/core/src/main/java/hudson/model/RunMap.java @@ -172,6 +172,7 @@ public final class RunMap> extends AbstractLazyLoadRunMap if (rootDir.isDirectory()) { throw new IllegalStateException(rootDir + " already existed; will not overwite with " + r); } + proposeNewNumber(r.getNumber()); rootDir.mkdirs(); return super._put(r); } diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java index b1c12800fc..27b87c6ae9 100644 --- a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java +++ b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java @@ -356,6 +356,12 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i return load(n, null); } + 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); + } + } + public R put(R value) { return _put(value); } diff --git a/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java b/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java index 8fcd92354b..2146852a5f 100644 --- a/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java +++ b/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java @@ -1,5 +1,6 @@ package jenkins.model.lazy; +import static org.junit.Assert.*; import org.junit.Test; /** @@ -12,6 +13,25 @@ public class SortedIntListTest { l.add(0); l.add(5); l.add(10); - System.out.println(l.lower(Integer.MAX_VALUE)); + assertEquals(2, l.lower(Integer.MAX_VALUE)); } + + @Test public void ceil() { + SortedIntList l = new SortedIntList(5); + l.add(1); + l.add(3); + l.add(5); + assertEquals(0, l.ceil(0)); + assertEquals(0, l.ceil(1)); + assertEquals(1, l.ceil(2)); + assertEquals(1, l.ceil(3)); + assertEquals(2, l.ceil(4)); + assertEquals(2, l.ceil(5)); + assertEquals(3, l.ceil(6)); + assertTrue(l.isInRange(0)); + assertTrue(l.isInRange(1)); + assertTrue(l.isInRange(2)); + assertFalse(l.isInRange(3)); + } + } From 25075826b5d26bb8be11789a89b28fd9d11d1c2e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 11:16:52 -0400 Subject: [PATCH 05/23] Making various changes to ALLRM incompatible to subclasses, so be explicit that this is distinct from RunMap only for purposes of clarity and testing. --- .../src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java index 27b87c6ae9..9211f8d046 100644 --- a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java +++ b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java @@ -140,6 +140,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i */ private File dir; + @Restricted(NoExternalUse.class) // subclassing other than by RunMap does not guarantee compatibility protected AbstractLazyLoadRunMap(File dir) { initBaseDir(dir); } From 843d2387234080efc2fa86acdd52b3053cbd08dd Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 15:04:45 -0400 Subject: [PATCH 06/23] Added RunIdMigrator. --- core/src/main/java/hudson/model/Job.java | 18 +- core/src/main/java/hudson/model/Run.java | 9 +- core/src/main/java/hudson/model/RunMap.java | 24 ++ .../java/jenkins/model/RunIdMigrator.java | 244 ++++++++++++++++++ .../model/lazy/AbstractLazyLoadRunMap.java | 13 +- .../jenkins/model/lazy/LazyBuildMixIn.java | 11 +- .../java/jenkins/model/RunIdMigratorTest.java | 129 +++++++++ 7 files changed, 432 insertions(+), 16 deletions(-) create mode 100644 core/src/main/java/jenkins/model/RunIdMigrator.java create mode 100644 core/src/test/java/jenkins/model/RunIdMigratorTest.java diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java index 9f5bb63c72..9b83cd0e35 100644 --- a/core/src/main/java/hudson/model/Job.java +++ b/core/src/main/java/hudson/model/Job.java @@ -101,7 +101,10 @@ import javax.annotation.Nonnull; import static javax.servlet.http.HttpServletResponse.*; import jenkins.model.ModelObjectWithChildren; +import jenkins.model.RunIdMigrator; import jenkins.model.lazy.LazyBuildMixIn; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; /** * A job is an runnable entity under the monitoring of Hudson. @@ -158,6 +161,9 @@ public abstract class Job, RunT extends Run> properties = new CopyOnWriteList>(); + @Restricted(NoExternalUse.class) + public transient RunIdMigrator runIdMigrator; + protected Job(ItemGroup parent, String name) { super(parent, name); } @@ -168,11 +174,21 @@ public abstract class Job, RunT extends Run parent, String name) throws IOException { super.onLoad(parent, name); + File buildDir = getBuildDir(); + runIdMigrator = new RunIdMigrator(); + runIdMigrator.migrate(buildDir); + TextFile f = getNextBuildNumberFile(); if (f.exists()) { // starting 1.28, we store nextBuildNumber in a separate file. @@ -184,7 +200,7 @@ public abstract class Job, RunT extends Run,RunT extends Run,RunT extends Run number map for use from AbstractLazyLoadRunMap - /** * Creates a new {@link Run}. * @param job Owner job diff --git a/core/src/main/java/hudson/model/RunMap.java b/core/src/main/java/hudson/model/RunMap.java index 00cba471c1..f5897bb5e8 100644 --- a/core/src/main/java/hudson/model/RunMap.java +++ b/core/src/main/java/hudson/model/RunMap.java @@ -35,10 +35,14 @@ import java.util.logging.Level; import static java.util.logging.Level.*; import java.util.logging.Logger; +import jenkins.model.RunIdMigrator; import jenkins.model.lazy.AbstractLazyLoadRunMap; import static jenkins.model.lazy.AbstractLazyLoadRunMap.Direction.*; import jenkins.model.lazy.BuildReference; +import jenkins.model.lazy.LazyBuildMixIn; import org.apache.commons.collections.comparators.ReverseComparator; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; /** * {@link Map} from build number to {@link Run}. @@ -58,6 +62,10 @@ public final class RunMap> extends AbstractLazyLoadRunMap private Constructor cons; + /** Normally overwritten by {@link LazyBuildMixIn#onLoad} or {@link LazyBuildMixIn#onCreatedFromScratch}, in turn created during {@link Job#onLoad}. */ + @Restricted(NoExternalUse.class) + public RunIdMigrator runIdMigrator = new RunIdMigrator(); + // TODO: before first complete build // patch up next/previous build link @@ -115,6 +123,7 @@ public final class RunMap> extends AbstractLazyLoadRunMap @Override public boolean removeValue(R run) { run.dropLinks(); + runIdMigrator.delete(dir, run.getId()); return super.removeValue(run); } @@ -161,6 +170,11 @@ public final class RunMap> extends AbstractLazyLoadRunMap return r.getNumber(); } + @Override + protected final String getIdOf(R r) { + return r.getId(); + } + /** * Add a new build to the map. * Do not use when loading existing builds (use {@link #put(Integer, Object)}). @@ -177,6 +191,16 @@ public final class RunMap> extends AbstractLazyLoadRunMap return super._put(r); } + @Override public R getById(String id) { + int n; + try { + n = Integer.parseInt(id); + } catch (NumberFormatException x) { + n = runIdMigrator.findNumber(id); + } + return getByNumber(n); + } + /** * Reuses the same reference as much as we can. *

diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java new file mode 100644 index 0000000000..9652a90669 --- /dev/null +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -0,0 +1,244 @@ +/* + * The MIT License + * + * Copyright 2014 Jesse Glick. + * + * 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 jenkins.model; + +import hudson.Util; +import hudson.model.Job; +import hudson.model.Run; +import java.io.File; +import java.io.IOException; +import java.io.PrintWriter; +import java.text.ParseException; +import java.text.SimpleDateFormat; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import javax.annotation.Nonnull; +import org.apache.commons.io.Charsets; +import org.apache.commons.io.FileUtils; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +/** + * Converts legacy {@code builds} directories to the current format. + * There would be one instance associated with each {@link Job}. + * The {@link Job#getBuildDir} is passed to every method call (rather than being cached) in case it is moved. + */ +@Restricted(NoExternalUse.class) +public final class RunIdMigrator { + + static final Logger LOGGER = Logger.getLogger(RunIdMigrator.class.getName()); + private static final SimpleDateFormat LEGACY_ID_FORMATTER = new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); + private static final String MAP_FILE = "legacyIds"; + /** avoids wasting a map for new jobs */ + private static final Map EMPTY = new TreeMap(); + + private @Nonnull Map idToNumber = EMPTY; + + public RunIdMigrator() {} + + /** + * @return whether there was a file to load + */ + private boolean load(File dir) { + File f = new File(dir, MAP_FILE); + if (!f.isFile()) { + return false; + } + if (f.length() == 0) { + return true; + } + idToNumber = new TreeMap(); + try { + for (String line : FileUtils.readLines(f)) { + int i = line.indexOf(' '); + idToNumber.put(line.substring(0, i), Integer.parseInt(line.substring(i + 1))); + } + } catch (Exception x) { // IOException, IndexOutOfBoundsException, NumberFormatException + LOGGER.log(Level.WARNING, "could not read from " + f, x); + } + return true; + } + + private void save(File dir) { + dir.mkdirs(); + File f = new File(dir, MAP_FILE); + try { + PrintWriter w = new PrintWriter(f); + try { + for (Map.Entry entry : idToNumber.entrySet()) { + w.println(entry.getKey() + ' ' + entry.getValue()); + } + } finally { + w.close(); + } + } catch (IOException x) { + LOGGER.log(Level.WARNING, "could not save changes to " + f, x); + } + } + + /** + * Called when a job is first created. + * Just saves an empty marker indicating that this job needs no migration. + * @param dir as in {@link Job#getBuildDir} + */ + public void created(File dir) { + save(dir); + } + + /** + * Perform one-time migration if this has not been done already. + * Where previously there would be a {@code 2014-01-02_03-04-05/build.xml} specifying {@code 99} plus a symlink {@code 99 → 2014-01-02_03-04-05}, + * after migration there will be just {@code 99/build.xml} specifying {@code 2014-01-02_03-04-05} and {@code …} according to local time zone at time of migration. + * Newly created builds are untouched. + * Does not throw {@link IOException} since we make a best effort to migrate but do not consider it fatal to job loading if we cannot. + * @param dir as in {@link Job#getBuildDir} + * @return true if migration was performed + */ + public synchronized boolean migrate(File dir) { + if (load(dir)) { + LOGGER.log(Level.FINER, "migration already performed for {0}", dir); + return false; + } + LOGGER.log(Level.INFO, "Migrating build records in {0}", dir); + doMigrate(dir); + save(dir); + return true; + } + + private static final Pattern NUMBER_ELT = Pattern.compile("(?m)^ (\\d+)(\r?\n)"); + private void doMigrate(File dir) { + idToNumber = new TreeMap(); + File[] kids = dir.listFiles(); + if (kids == null) { + LOGGER.warning("dir was unexpectedly missing"); + return; + } + // Need to process symlinks first so we can rename to them. + List kidsList = new ArrayList(Arrays.asList(kids)); + Iterator it = kidsList.iterator(); + while (it.hasNext()) { + File kid = it.next(); + String name = kid.getName(); + try { + String link = Util.resolveSymlink(kid); + if (link == null) { + continue; + } + try { + Integer.parseInt(name); + if (kid.delete()) { + LOGGER.log(Level.FINE, "deleted build number symlink {0} → {1}", new Object[] {name, link}); + } else { + LOGGER.log(Level.WARNING, "could not delete build number symlink {0} → {1}", new Object[] {name, link}); + } + } catch (NumberFormatException x) { + LOGGER.log(Level.FINE, "skipping other symlink {0} → {1}", new Object[] {name, link}); + } + it.remove(); + } catch (Exception x) { + LOGGER.log(Level.WARNING, "failed to process " + kid, x); + } + } + it = kidsList.iterator(); + while (it.hasNext()) { + File kid = it.next(); + try { + String name = kid.getName(); + try { + Integer.parseInt(name); + LOGGER.log(Level.FINE, "skipping new build dir {0}", name); + continue; + } catch (NumberFormatException x) { + // OK, next… + } + if (!kid.isDirectory()) { + LOGGER.log(Level.FINE, "skipping non-directory {0}", name); + continue; + } + long timestamp; + try { + timestamp = LEGACY_ID_FORMATTER.parse(name).getTime(); + } catch (ParseException x) { + LOGGER.log(Level.WARNING, "found unexpected dir {0}", name); + continue; + } + File buildXml = new File(kid, "build.xml"); + if (!buildXml.isFile()) { + LOGGER.log(Level.WARNING, "found no build.xml in {0}", name); + continue; + } + String xml = FileUtils.readFileToString(buildXml, Charsets.UTF_8); + Matcher m = NUMBER_ELT.matcher(xml); + if (!m.find()) { + LOGGER.log(Level.WARNING, "could not find in {0}/build.xml", name); + continue; + } + int number = Integer.parseInt(m.group(1)); + String nl = m.group(2); + xml = m.replaceFirst(" " + name + "" + nl + " " + timestamp + "" + nl); + File newKid = new File(dir, Integer.toString(number)); + if (!kid.renameTo(newKid)) { + LOGGER.log(Level.WARNING, "failed to rename {0} to {1}", new Object[] {name, number}); + continue; + } + FileUtils.writeStringToFile(new File(newKid, "build.xml"), xml, Charsets.UTF_8); + LOGGER.log(Level.FINE, "fully processed {0} → {1}", new Object[] {name, number}); + idToNumber.put(name, number); + } catch (Exception x) { + LOGGER.log(Level.WARNING, "failed to process " + kid, x); + } + } + } + + /** + * Look up a historical run by ID. + * @param id a nonnumeric ID which may be a valid {@link Run#getId} + * @return the corresponding {@link Run#number}, or 0 if unknown + */ + public synchronized int findNumber(@Nonnull String id) { + Integer number = idToNumber.get(id); + return number != null ? number : 0; + } + + /** + * Delete the record of a build. + * @param dir as in {@link Job#getBuildDir} + * @param id a {@link Run#getId} + */ + public synchronized void delete(File dir, String id) { + if (idToNumber.remove(id) != null) { + save(dir); + } + } + +} diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java index 9211f8d046..fa6f53f143 100644 --- a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java +++ b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java @@ -138,7 +138,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i * because the compatibility requires that we make it settable * in the first call after the constructor. */ - private File dir; + protected File dir; @Restricted(NoExternalUse.class) // subclassing other than by RunMap does not guarantee compatibility protected AbstractLazyLoadRunMap(File dir) { @@ -170,7 +170,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i public final void updateBaseDir(File dir) { this.dir = dir; } - + /** * Let go of all the loaded references. * @@ -341,8 +341,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i } public R getById(String id) { - int n = Integer.parseInt(id); // TODO also need an index of historical IDs → number - return getByNumber(n); + return getByNumber(Integer.parseInt(id)); } public R getByNumber(int n) { @@ -483,8 +482,10 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i */ protected abstract int getNumberOf(R r); - private String getIdOf(R r) { - // TODO see comment in getById + /** + * Subtype to provide {@link Run#getId()} so that this class doesn't have to depend on it. + */ + protected String getIdOf(R r) { return String.valueOf(getNumberOf(r)); } diff --git a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java index e188f193a9..99c48f820b 100644 --- a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java +++ b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java @@ -47,6 +47,7 @@ import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.DoNotUse; import static java.util.logging.Level.FINER; +import jenkins.model.RunIdMigrator; /** * Makes it easier to use a lazy {@link RunMap} from a {@link Job} implementation. @@ -89,14 +90,14 @@ public abstract class LazyBuildMixIn & Queue.Task & } /** - * Something to be called from {@link AbstractItem#onCreatedFromScratch}. + * Something to be called from {@link Job#onCreatedFromScratch}. */ public final void onCreatedFromScratch() { builds = createBuildRunMap(); } /** - * Something to be called from {@link AbstractItem#onLoad}. + * Something to be called from {@link Job#onLoad}. */ @SuppressWarnings("unchecked") public void onLoad(ItemGroup parent, String name) throws IOException { @@ -129,11 +130,15 @@ public abstract class LazyBuildMixIn & Queue.Task & } private RunMap createBuildRunMap() { - return new RunMap(asJob().getBuildDir(), new RunMap.Constructor() { + RunMap r = new RunMap(asJob().getBuildDir(), new RunMap.Constructor() { @Override public RunT create(File dir) throws IOException { return loadBuild(dir); } }); + RunIdMigrator runIdMigrator = asJob().runIdMigrator; + assert runIdMigrator != null; + r.runIdMigrator = runIdMigrator; + return r; } /** diff --git a/core/src/test/java/jenkins/model/RunIdMigratorTest.java b/core/src/test/java/jenkins/model/RunIdMigratorTest.java new file mode 100644 index 0000000000..4a75a1377b --- /dev/null +++ b/core/src/test/java/jenkins/model/RunIdMigratorTest.java @@ -0,0 +1,129 @@ +/* + * The MIT License + * + * Copyright 2014 Jesse Glick. + * + * 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 jenkins.model; + +import hudson.Util; +import hudson.util.StreamTaskListener; +import java.io.File; +import java.nio.charset.Charset; +import java.util.Map; +import java.util.TimeZone; +import java.util.TreeMap; +import java.util.logging.ConsoleHandler; +import java.util.logging.Handler; +import java.util.logging.Level; +import org.apache.commons.io.FileUtils; +import org.junit.Test; +import static org.junit.Assert.*; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.rules.TemporaryFolder; + +public class RunIdMigratorTest { + + @Rule public TemporaryFolder tmp = new TemporaryFolder(); + + /** Ensures that legacy timestamps are interpreted in a predictable time zone. */ + @BeforeClass public static void timezone() { + TimeZone.setDefault(TimeZone.getTimeZone("EST")); + } + + @BeforeClass public static void logging() { + RunIdMigrator.LOGGER.setLevel(Level.ALL); + Handler handler = new ConsoleHandler(); + handler.setLevel(Level.ALL); + RunIdMigrator.LOGGER.addHandler(handler); + } + + private RunIdMigrator migrator; + private File dir; + + @Before public void init() { + migrator = new RunIdMigrator(); + dir = tmp.getRoot(); + } + + @Test public void newJob() throws Exception { + migrator.created(dir); + assertEquals("{legacyIds=''}", summarize()); + assertEquals(0, migrator.findNumber("whatever")); + migrator.delete(dir, "1"); + migrator = new RunIdMigrator(); + assertFalse(migrator.migrate(dir)); + assertEquals("{legacyIds=''}", summarize()); + } + + @Test public void legacy() throws Exception { + write("2014-01-02_03-04-05/build.xml", "\n\n ok\n 99\n ok\n"); + link("99", "2014-01-02_03-04-05"); + link("lastFailedBuild", "-1"); + link("lastSuccessfulBuild", "99"); + assertTrue(migrator.migrate(dir)); + assertEquals("{99={build.xml='\n\n ok\n 2014-01-02_03-04-05\n 1388649845000\n ok\n'}, lastFailedBuild=→-1, lastSuccessfulBuild=→99, legacyIds='2014-01-02_03-04-05 99\n'}", summarize()); + assertEquals(99, migrator.findNumber("2014-01-02_03-04-05")); + migrator = new RunIdMigrator(); + assertFalse(migrator.migrate(dir)); + assertEquals(99, migrator.findNumber("2014-01-02_03-04-05")); + migrator.delete(dir, "2014-01-02_03-04-05"); + FileUtils.deleteDirectory(new File(dir, "99")); + new File(dir, "lastSuccessfulBuild").delete(); + assertEquals("{lastFailedBuild=→-1, legacyIds=''}", summarize()); + } + + // TODO test sane recovery from various error conditions + + private void write(String file, String text) throws Exception { + FileUtils.write(new File(dir, file), text); + } + + private void link(String symlink, String dest) throws Exception { + Util.createSymlink(dir, dest, symlink, new StreamTaskListener(System.out, Charset.defaultCharset())); + } + + private String summarize() throws Exception { + return summarize(dir); + } + private static String summarize(File dir) throws Exception { + File[] kids = dir.listFiles(); + Map m = new TreeMap(); + for (File kid : kids) { + String notation; + String symlink = Util.resolveSymlink(kid); + if (symlink != null) { + notation = "→" + symlink; + } else if (kid.isFile()) { + notation = "'" + FileUtils.readFileToString(kid) + "'"; + } else if (kid.isDirectory()) { + notation = summarize(kid); + } else { + notation = "?"; + } + m.put(kid.getName(), notation); + } + return m.toString(); + } + +} \ No newline at end of file From 268d20c34135dc05ab0685957238a21047817de3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 15:13:51 -0400 Subject: [PATCH 07/23] Be consistent and use Integer.toString rather than String.valueOf. --- core/src/main/java/hudson/model/Run.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 45ed064cac..395b7698e3 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -978,7 +978,7 @@ public abstract class Run ,RunT extends Run Date: Wed, 27 Aug 2014 15:15:02 -0400 Subject: [PATCH 08/23] Need to synchronize access to LEGACY_ID_FORMATTER. --- core/src/main/java/jenkins/model/RunIdMigrator.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java index 9652a90669..e794515e14 100644 --- a/core/src/main/java/jenkins/model/RunIdMigrator.java +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -30,6 +30,7 @@ import hudson.model.Run; import java.io.File; import java.io.IOException; import java.io.PrintWriter; +import java.text.DateFormat; import java.text.ParseException; import java.text.SimpleDateFormat; import java.util.ArrayList; @@ -57,7 +58,7 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; public final class RunIdMigrator { static final Logger LOGGER = Logger.getLogger(RunIdMigrator.class.getName()); - private static final SimpleDateFormat LEGACY_ID_FORMATTER = new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); + private static final DateFormat LEGACY_ID_FORMATTER = new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); private static final String MAP_FILE = "legacyIds"; /** avoids wasting a map for new jobs */ private static final Map EMPTY = new TreeMap(); @@ -187,7 +188,9 @@ public final class RunIdMigrator { } long timestamp; try { - timestamp = LEGACY_ID_FORMATTER.parse(name).getTime(); + synchronized (LEGACY_ID_FORMATTER) { + timestamp = LEGACY_ID_FORMATTER.parse(name).getTime(); + } } catch (ParseException x) { LOGGER.log(Level.WARNING, "found unexpected dir {0}", name); continue; From d5680873472669b764597a0316f59c2f9bcf505f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 15:15:21 -0400 Subject: [PATCH 09/23] Obsolete logic to delete build number symlinks. --- core/src/main/java/hudson/model/Run.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 395b7698e3..b2753ea4ff 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -1453,10 +1453,6 @@ public abstract class Run ,RunT extends Run Date: Wed, 27 Aug 2014 15:17:18 -0400 Subject: [PATCH 10/23] Minimizing diff to master. --- .../main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java index fa6f53f143..147c79d5ae 100644 --- a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java +++ b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java @@ -170,7 +170,7 @@ public abstract class AbstractLazyLoadRunMap extends AbstractMap i public final void updateBaseDir(File dir) { this.dir = dir; } - + /** * Let go of all the loaded references. * From a73ba0ac91bce9d0eb6de0e41095847a80851cff Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 15:18:00 -0400 Subject: [PATCH 11/23] Integer.toString here too. --- core/src/main/java/hudson/model/Run.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index b2753ea4ff..2232930c65 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -963,7 +963,7 @@ public abstract class Run ,RunT extends Run Date: Wed, 27 Aug 2014 15:19:31 -0400 Subject: [PATCH 12/23] A couple of methods that should no longer be @Deprecated. --- core/src/main/java/hudson/model/AbstractBuild.java | 1 - core/src/main/java/hudson/model/Build.java | 1 - 2 files changed, 2 deletions(-) diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/core/src/main/java/hudson/model/AbstractBuild.java index 520e721893..d1e291d52e 100644 --- a/core/src/main/java/hudson/model/AbstractBuild.java +++ b/core/src/main/java/hudson/model/AbstractBuild.java @@ -167,7 +167,6 @@ public abstract class AbstractBuild

,R extends Abs super(job); } - @Deprecated protected AbstractBuild(P job, Calendar timestamp) { super(job, timestamp); } diff --git a/core/src/main/java/hudson/model/Build.java b/core/src/main/java/hudson/model/Build.java index 30057fd3e7..99073f1b73 100644 --- a/core/src/main/java/hudson/model/Build.java +++ b/core/src/main/java/hudson/model/Build.java @@ -92,7 +92,6 @@ public abstract class Build

,B extends Build> super(project); } - @Deprecated protected Build(P job, Calendar timestamp) { super(job, timestamp); } From a1cf988bfc273c2134d82b760367190276eeccc6 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Aug 2014 15:22:26 -0400 Subject: [PATCH 13/23] Simplified constructor. --- core/src/main/java/hudson/model/Run.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 2232930c65..c9f8777200 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -275,9 +275,7 @@ public abstract class Run ,RunT extends Run Date: Fri, 12 Dec 2014 14:59:03 -0500 Subject: [PATCH 14/23] Compilability fix. --- .../java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java b/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java index 5a9ad3be1b..247653e501 100644 --- a/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java +++ b/core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java @@ -332,7 +332,7 @@ public class AbstractLazyLoadRunMapTest extends Assert { @Issue("JENKINS-25655") @Test public void entrySetChanges() { assertEquals(3, a.entrySet().size()); - a.put(new Build(7, "D")); + a.put(new Build(7)); assertEquals(4, a.entrySet().size()); } From 507412e0dbe9533be0f89c87157590b9cc61b5bd Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 12 Dec 2014 15:43:38 -0500 Subject: [PATCH 15/23] Safer to use AtomicFileWriter. --- core/src/main/java/jenkins/model/RunIdMigrator.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java index e794515e14..70b2af51b1 100644 --- a/core/src/main/java/jenkins/model/RunIdMigrator.java +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -27,9 +27,9 @@ package jenkins.model; import hudson.Util; import hudson.model.Job; import hudson.model.Run; +import hudson.util.AtomicFileWriter; import java.io.File; import java.io.IOException; -import java.io.PrintWriter; import java.text.DateFormat; import java.text.ParseException; import java.text.SimpleDateFormat; @@ -91,16 +91,16 @@ public final class RunIdMigrator { } private void save(File dir) { - dir.mkdirs(); File f = new File(dir, MAP_FILE); try { - PrintWriter w = new PrintWriter(f); + AtomicFileWriter w = new AtomicFileWriter(f); try { for (Map.Entry entry : idToNumber.entrySet()) { - w.println(entry.getKey() + ' ' + entry.getValue()); + w.write(entry.getKey() + ' ' + entry.getValue() + '\n'); } + w.commit(); } finally { - w.close(); + w.abort(); } } catch (IOException x) { LOGGER.log(Level.WARNING, "could not save changes to " + f, x); From 0a9b63828062782643728b0a0f4f86dc20a8e7f9 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 15 Dec 2014 14:26:36 -0500 Subject: [PATCH 16/23] Added a tool to reverse the build directory migration if required. --- core/src/main/java/hudson/model/Job.java | 2 +- .../java/jenkins/model/RunIdMigrator.java | 112 +++++++++++++++++- .../java/jenkins/model/RunIdMigratorTest.java | 29 ++++- 3 files changed, 138 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java index e6f996987d..21ff3716c7 100644 --- a/core/src/main/java/hudson/model/Job.java +++ b/core/src/main/java/hudson/model/Job.java @@ -191,7 +191,7 @@ public abstract class Job, RunT extends Run EMPTY = new TreeMap(); + private static final Set offeredToUnmigrate = Collections.synchronizedSet(new HashSet()); private @Nonnull Map idToNumber = EMPTY; @@ -123,9 +136,10 @@ public final class RunIdMigrator { * Newly created builds are untouched. * Does not throw {@link IOException} since we make a best effort to migrate but do not consider it fatal to job loading if we cannot. * @param dir as in {@link Job#getBuildDir} + * @param jenkinsHome root directory of Jenkins (for logging only) * @return true if migration was performed */ - public synchronized boolean migrate(File dir) { + public synchronized boolean migrate(File dir, @CheckForNull File jenkinsHome) { if (load(dir)) { LOGGER.log(Level.FINER, "migration already performed for {0}", dir); return false; @@ -133,6 +147,25 @@ public final class RunIdMigrator { LOGGER.log(Level.INFO, "Migrating build records in {0}", dir); doMigrate(dir); save(dir); + if (jenkinsHome != null && offeredToUnmigrate.add(jenkinsHome)) { + StringBuilder cp = new StringBuilder(); + for (Class c : new Class[] {RunIdMigrator.class, /* TODO how to calculate transitive dependencies automatically? */Charsets.class, WriterOutputStream.class, BuildException.class, FastDateFormat.class}) { + URL location = c.getProtectionDomain().getCodeSource().getLocation(); + String locationS = location.toString(); + if (location.getProtocol().equals("file")) { + try { + locationS = new File(location.toURI()).getAbsolutePath(); + } catch (URISyntaxException x) { + // never mind + } + } + if (cp.length() > 0) { + cp.append(File.pathSeparator); + } + cp.append(locationS); + } + LOGGER.log(Level.WARNING, "Build record migration is one-way. If you need to downgrade Jenkins, run: java -classpath {0} {1} {2}", new Object[] {cp, RunIdMigrator.class.getName(), jenkinsHome}); + } return true; } @@ -244,4 +277,81 @@ public final class RunIdMigrator { } } + /** + * Reverses the migration, in case you want to revert to the older format. + * @param args one parameter, {@code $JENKINS_HOME} + */ + public static void main(String... args) throws Exception { + if (args.length != 1) { + throw new Exception("pass one parameter, $JENKINS_HOME"); + } + File root = new File(args[0]); + File jobs = new File(root, "jobs"); + if (!jobs.isDirectory()) { + throw new FileNotFoundException("no such $JENKINS_HOME " + root); + } + unmigrateJobsDir(jobs); + } + private static void unmigrateJobsDir(File jobs) throws Exception { + System.err.println(jobs + " will be inspected for build directories to restore"); + for (File kid : jobs.listFiles()) { + File builds = new File(kid, "builds"); + if (builds.isDirectory()) { // kid is a job + unmigrateBuildsDir(builds); + } + File jobs2 = new File(kid, "jobs"); + if (jobs2.isDirectory()) { // kid is a folder + unmigrateJobsDir(jobs2); + } + } + } + private static final Pattern ID_ELT = Pattern.compile("(?m)^ ([0-9_-]+)(\r?\n)"); + private static final Pattern TIMESTAMP_ELT = Pattern.compile("(?m)^ (\\d+)(\r?\n)"); + /** Inverse of {@link #doMigrate}. */ + private static void unmigrateBuildsDir(File builds) throws Exception { + File mapFile = new File(builds, MAP_FILE); + if (!mapFile.isFile()) { + System.err.println(builds + " does not look to have been migrated yet; skipping"); + return; + } + for (File build : builds.listFiles()) { + int number; + try { + number = Integer.parseInt(build.getName()); + } catch (NumberFormatException x) { + continue; + } + File buildXml = new File(build, "build.xml"); + if (!buildXml.isFile()) { + System.err.println(buildXml + " did not exist"); + continue; + } + String xml = FileUtils.readFileToString(buildXml, Charsets.UTF_8); + Matcher m = TIMESTAMP_ELT.matcher(xml); + if (!m.find()) { + System.err.println(buildXml + " did not contain as expected"); + continue; + } + long timestamp = Long.parseLong(m.group(1)); + String nl = m.group(2); + xml = m.replaceFirst(" " + number + "" + nl); + m = ID_ELT.matcher(xml); + String id; + if (m.find()) { + id = m.group(1); + xml = m.replaceFirst(""); + } else { + // Post-migration build. We give it a new ID based on its timestamp. + id = LEGACY_ID_FORMATTER.format(new Date(timestamp)); + } + FileUtils.write(buildXml, xml, Charsets.UTF_8); + if (!build.renameTo(new File(builds, id))) { + System.err.println(build + " could not be renamed"); + } + Util.createSymlink(builds, id, Integer.toString(number), StreamTaskListener.fromStderr()); + } + Util.deleteFile(mapFile); + System.err.println(builds + " has been restored to its original format"); + } + } diff --git a/core/src/test/java/jenkins/model/RunIdMigratorTest.java b/core/src/test/java/jenkins/model/RunIdMigratorTest.java index 4a75a1377b..43ac629a50 100644 --- a/core/src/test/java/jenkins/model/RunIdMigratorTest.java +++ b/core/src/test/java/jenkins/model/RunIdMigratorTest.java @@ -72,7 +72,7 @@ public class RunIdMigratorTest { assertEquals(0, migrator.findNumber("whatever")); migrator.delete(dir, "1"); migrator = new RunIdMigrator(); - assertFalse(migrator.migrate(dir)); + assertFalse(migrator.migrate(dir, null)); assertEquals("{legacyIds=''}", summarize()); } @@ -81,11 +81,12 @@ public class RunIdMigratorTest { link("99", "2014-01-02_03-04-05"); link("lastFailedBuild", "-1"); link("lastSuccessfulBuild", "99"); - assertTrue(migrator.migrate(dir)); + assertEquals("{2014-01-02_03-04-05={build.xml='\n\n ok\n 99\n ok\n'}, 99=→2014-01-02_03-04-05, lastFailedBuild=→-1, lastSuccessfulBuild=→99}", summarize()); + assertTrue(migrator.migrate(dir, null)); assertEquals("{99={build.xml='\n\n ok\n 2014-01-02_03-04-05\n 1388649845000\n ok\n'}, lastFailedBuild=→-1, lastSuccessfulBuild=→99, legacyIds='2014-01-02_03-04-05 99\n'}", summarize()); assertEquals(99, migrator.findNumber("2014-01-02_03-04-05")); migrator = new RunIdMigrator(); - assertFalse(migrator.migrate(dir)); + assertFalse(migrator.migrate(dir, null)); assertEquals(99, migrator.findNumber("2014-01-02_03-04-05")); migrator.delete(dir, "2014-01-02_03-04-05"); FileUtils.deleteDirectory(new File(dir, "99")); @@ -93,6 +94,28 @@ public class RunIdMigratorTest { assertEquals("{lastFailedBuild=→-1, legacyIds=''}", summarize()); } + @Test public void reverseImmediately() throws Exception { + File root = dir; + dir = new File(dir, "jobs/somefolder/jobs/someproject/builds"); + write("99/build.xml", "\n\n ok\n 2014-01-02_03-04-05\n 1388649845000\n ok\n"); + link("lastFailedBuild", "-1"); + link("lastSuccessfulBuild", "99"); + write("legacyIds", "2014-01-02_03-04-05 99\n"); + assertEquals("{99={build.xml='\n\n ok\n 2014-01-02_03-04-05\n 1388649845000\n ok\n'}, lastFailedBuild=→-1, lastSuccessfulBuild=→99, legacyIds='2014-01-02_03-04-05 99\n'}", summarize()); + RunIdMigrator.main(root.getAbsolutePath()); + assertEquals("{2014-01-02_03-04-05={build.xml='\n\n ok\n 99\n ok\n'}, 99=→2014-01-02_03-04-05, lastFailedBuild=→-1, lastSuccessfulBuild=→99}", summarize()); + } + + @Test public void reverseAfterNewBuilds() throws Exception { + File root = dir; + dir = new File(dir, "jobs/someproject/builds"); + write("1/build.xml", "\n\n ok\n 1388649845000\n ok\n"); + write("legacyIds", ""); + assertEquals("{1={build.xml='\n\n ok\n 1388649845000\n ok\n'}, legacyIds=''}", summarize()); + RunIdMigrator.main(root.getAbsolutePath()); + assertEquals("{1=→2014-01-02_03-04-05, 2014-01-02_03-04-05={build.xml='\n\n ok\n 1\n ok\n'}}", summarize()); + } + // TODO test sane recovery from various error conditions private void write(String file, String text) throws Exception { From 589d74a55e122e9d8f5507c5d6799b24cb5cb621 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 15 Dec 2014 15:51:45 -0500 Subject: [PATCH 17/23] Making reverse migration tool handle more kinds of item groups. --- .../java/jenkins/model/RunIdMigrator.java | 22 ++++++++++++------- .../java/jenkins/model/RunIdMigratorTest.java | 4 ++-- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java index f74d6db62e..eabab2694b 100644 --- a/core/src/main/java/jenkins/model/RunIdMigrator.java +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -293,15 +293,21 @@ public final class RunIdMigrator { unmigrateJobsDir(jobs); } private static void unmigrateJobsDir(File jobs) throws Exception { - System.err.println(jobs + " will be inspected for build directories to restore"); - for (File kid : jobs.listFiles()) { - File builds = new File(kid, "builds"); - if (builds.isDirectory()) { // kid is a job - unmigrateBuildsDir(builds); + for (File job : jobs.listFiles()) { + File[] kids = job.listFiles(); + if (kids == null) { + continue; } - File jobs2 = new File(kid, "jobs"); - if (jobs2.isDirectory()) { // kid is a folder - unmigrateJobsDir(jobs2); + for (File kid : kids) { + if (!kid.isDirectory()) { + continue; + } + if (kid.getName().equals("builds")) { + unmigrateBuildsDir(kid); + } else { + // Might be jobs, modules, promotions, etc.; we assume an ItemGroup.getRootDirFor implementation returns grandchildren. + unmigrateJobsDir(kid); + } } } } diff --git a/core/src/test/java/jenkins/model/RunIdMigratorTest.java b/core/src/test/java/jenkins/model/RunIdMigratorTest.java index 43ac629a50..17241b151c 100644 --- a/core/src/test/java/jenkins/model/RunIdMigratorTest.java +++ b/core/src/test/java/jenkins/model/RunIdMigratorTest.java @@ -96,7 +96,7 @@ public class RunIdMigratorTest { @Test public void reverseImmediately() throws Exception { File root = dir; - dir = new File(dir, "jobs/somefolder/jobs/someproject/builds"); + dir = new File(dir, "jobs/somefolder/jobs/someproject/promotions/OK/builds"); write("99/build.xml", "\n\n ok\n 2014-01-02_03-04-05\n 1388649845000\n ok\n"); link("lastFailedBuild", "-1"); link("lastSuccessfulBuild", "99"); @@ -108,7 +108,7 @@ public class RunIdMigratorTest { @Test public void reverseAfterNewBuilds() throws Exception { File root = dir; - dir = new File(dir, "jobs/someproject/builds"); + dir = new File(dir, "jobs/someproject/modules/test$test/builds"); write("1/build.xml", "\n\n ok\n 1388649845000\n ok\n"); write("legacyIds", ""); assertEquals("{1={build.xml='\n\n ok\n 1388649845000\n ok\n'}, legacyIds=''}", summarize()); From c6486e2b4d71d8e1830b48f43ac4c84d8b06b0fb Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 15 Dec 2014 16:16:29 -0500 Subject: [PATCH 18/23] #1406 silently skipped the call to moveDirectory in case newBuildDir.isDirectory(), rather than throwing an IOE as renameTo does. Also deleting apparently useless call to forceMkdir. --- core/src/main/java/hudson/model/Job.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java index 21ff3716c7..2addad98b0 100644 --- a/core/src/main/java/hudson/model/Job.java +++ b/core/src/main/java/hudson/model/Job.java @@ -654,8 +654,7 @@ public abstract class Job, RunT extends Run Date: Mon, 15 Dec 2014 16:17:35 -0500 Subject: [PATCH 19/23] ItemsTest.moveItem showed that if migrate is called on a nonexistent directory, we should do nothing. --- core/src/main/java/jenkins/model/RunIdMigrator.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java index eabab2694b..eb233d473c 100644 --- a/core/src/main/java/jenkins/model/RunIdMigrator.java +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -144,6 +144,10 @@ public final class RunIdMigrator { LOGGER.log(Level.FINER, "migration already performed for {0}", dir); return false; } + if (!dir.isDirectory()) { + LOGGER.log(/* normal during Job.movedTo */Level.FINE, "{0} was unexpectedly missing", dir); + return false; + } LOGGER.log(Level.INFO, "Migrating build records in {0}", dir); doMigrate(dir); save(dir); @@ -173,10 +177,6 @@ public final class RunIdMigrator { private void doMigrate(File dir) { idToNumber = new TreeMap(); File[] kids = dir.listFiles(); - if (kids == null) { - LOGGER.warning("dir was unexpectedly missing"); - return; - } // Need to process symlinks first so we can rename to them. List kidsList = new ArrayList(Arrays.asList(kids)); Iterator it = kidsList.iterator(); From ebe2f8cf0515e24a5552e964b94fa8b4db19ad94 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Mon, 22 Dec 2014 15:51:50 -0800 Subject: [PATCH 20/23] Formatting change: static import --- .../java/jenkins/model/RunIdMigrator.java | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java index eb233d473c..28de4897c6 100644 --- a/core/src/main/java/jenkins/model/RunIdMigrator.java +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -47,7 +47,6 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeMap; -import java.util.logging.Level; import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -61,6 +60,8 @@ import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.framework.io.WriterOutputStream; +import static java.util.logging.Level.*; + /** * Converts legacy {@code builds} directories to the current format. * There would be one instance associated with each {@link Job}. @@ -98,7 +99,7 @@ public final class RunIdMigrator { idToNumber.put(line.substring(0, i), Integer.parseInt(line.substring(i + 1))); } } catch (Exception x) { // IOException, IndexOutOfBoundsException, NumberFormatException - LOGGER.log(Level.WARNING, "could not read from " + f, x); + LOGGER.log(WARNING, "could not read from " + f, x); } return true; } @@ -116,7 +117,7 @@ public final class RunIdMigrator { w.abort(); } } catch (IOException x) { - LOGGER.log(Level.WARNING, "could not save changes to " + f, x); + LOGGER.log(WARNING, "could not save changes to " + f, x); } } @@ -141,14 +142,14 @@ public final class RunIdMigrator { */ public synchronized boolean migrate(File dir, @CheckForNull File jenkinsHome) { if (load(dir)) { - LOGGER.log(Level.FINER, "migration already performed for {0}", dir); + LOGGER.log(FINER, "migration already performed for {0}", dir); return false; } if (!dir.isDirectory()) { - LOGGER.log(/* normal during Job.movedTo */Level.FINE, "{0} was unexpectedly missing", dir); + LOGGER.log(/* normal during Job.movedTo */FINE, "{0} was unexpectedly missing", dir); return false; } - LOGGER.log(Level.INFO, "Migrating build records in {0}", dir); + LOGGER.log(INFO, "Migrating build records in {0}", dir); doMigrate(dir); save(dir); if (jenkinsHome != null && offeredToUnmigrate.add(jenkinsHome)) { @@ -168,7 +169,7 @@ public final class RunIdMigrator { } cp.append(locationS); } - LOGGER.log(Level.WARNING, "Build record migration is one-way. If you need to downgrade Jenkins, run: java -classpath {0} {1} {2}", new Object[] {cp, RunIdMigrator.class.getName(), jenkinsHome}); + LOGGER.log(WARNING, "Build record migration is one-way. If you need to downgrade Jenkins, run: java -classpath {0} {1} {2}", new Object[] {cp, RunIdMigrator.class.getName(), jenkinsHome}); } return true; } @@ -191,16 +192,16 @@ public final class RunIdMigrator { try { Integer.parseInt(name); if (kid.delete()) { - LOGGER.log(Level.FINE, "deleted build number symlink {0} → {1}", new Object[] {name, link}); + LOGGER.log(FINE, "deleted build number symlink {0} → {1}", new Object[] {name, link}); } else { - LOGGER.log(Level.WARNING, "could not delete build number symlink {0} → {1}", new Object[] {name, link}); + LOGGER.log(WARNING, "could not delete build number symlink {0} → {1}", new Object[] {name, link}); } } catch (NumberFormatException x) { - LOGGER.log(Level.FINE, "skipping other symlink {0} → {1}", new Object[] {name, link}); + LOGGER.log(FINE, "skipping other symlink {0} → {1}", new Object[] {name, link}); } it.remove(); } catch (Exception x) { - LOGGER.log(Level.WARNING, "failed to process " + kid, x); + LOGGER.log(WARNING, "failed to process " + kid, x); } } it = kidsList.iterator(); @@ -210,13 +211,13 @@ public final class RunIdMigrator { String name = kid.getName(); try { Integer.parseInt(name); - LOGGER.log(Level.FINE, "skipping new build dir {0}", name); + LOGGER.log(FINE, "skipping new build dir {0}", name); continue; } catch (NumberFormatException x) { // OK, next… } if (!kid.isDirectory()) { - LOGGER.log(Level.FINE, "skipping non-directory {0}", name); + LOGGER.log(FINE, "skipping non-directory {0}", name); continue; } long timestamp; @@ -225,18 +226,18 @@ public final class RunIdMigrator { timestamp = LEGACY_ID_FORMATTER.parse(name).getTime(); } } catch (ParseException x) { - LOGGER.log(Level.WARNING, "found unexpected dir {0}", name); + LOGGER.log(WARNING, "found unexpected dir {0}", name); continue; } File buildXml = new File(kid, "build.xml"); if (!buildXml.isFile()) { - LOGGER.log(Level.WARNING, "found no build.xml in {0}", name); + LOGGER.log(WARNING, "found no build.xml in {0}", name); continue; } String xml = FileUtils.readFileToString(buildXml, Charsets.UTF_8); Matcher m = NUMBER_ELT.matcher(xml); if (!m.find()) { - LOGGER.log(Level.WARNING, "could not find in {0}/build.xml", name); + LOGGER.log(WARNING, "could not find in {0}/build.xml", name); continue; } int number = Integer.parseInt(m.group(1)); @@ -244,14 +245,14 @@ public final class RunIdMigrator { xml = m.replaceFirst(" " + name + "" + nl + " " + timestamp + "" + nl); File newKid = new File(dir, Integer.toString(number)); if (!kid.renameTo(newKid)) { - LOGGER.log(Level.WARNING, "failed to rename {0} to {1}", new Object[] {name, number}); + LOGGER.log(WARNING, "failed to rename {0} to {1}", new Object[] {name, number}); continue; } FileUtils.writeStringToFile(new File(newKid, "build.xml"), xml, Charsets.UTF_8); - LOGGER.log(Level.FINE, "fully processed {0} → {1}", new Object[] {name, number}); + LOGGER.log(FINE, "fully processed {0} → {1}", new Object[] {name, number}); idToNumber.put(name, number); } catch (Exception x) { - LOGGER.log(Level.WARNING, "failed to process " + kid, x); + LOGGER.log(WARNING, "failed to process " + kid, x); } } } From 73208027269fd0d20b8ef5307e7f1e8998a98a4b Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Mon, 22 Dec 2014 15:58:39 -0800 Subject: [PATCH 21/23] minor doc improvements --- core/src/main/java/jenkins/model/RunIdMigrator.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java index 28de4897c6..71126c103c 100644 --- a/core/src/main/java/jenkins/model/RunIdMigrator.java +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -64,7 +64,9 @@ import static java.util.logging.Level.*; /** * Converts legacy {@code builds} directories to the current format. - * There would be one instance associated with each {@link Job}. + * + * There would be one instance associated with each {@link Job}, to retain ID -> build# mapping. + * * The {@link Job#getBuildDir} is passed to every method call (rather than being cached) in case it is moved. */ @Restricted(NoExternalUse.class) @@ -75,6 +77,10 @@ public final class RunIdMigrator { private static final String MAP_FILE = "legacyIds"; /** avoids wasting a map for new jobs */ private static final Map EMPTY = new TreeMap(); + + /** + * Did we record "unmigrate" instruction for this $JENKINS_HOME? Yes if it's in the set. + */ private static final Set offeredToUnmigrate = Collections.synchronizedSet(new HashSet()); private @Nonnull Map idToNumber = EMPTY; From 6f0eb9be595764b3cf3478546ed444401c9a3462 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Mon, 22 Dec 2014 15:59:32 -0800 Subject: [PATCH 22/23] Path could include whitespace, so double quote would be nice. --- core/src/main/java/jenkins/model/RunIdMigrator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/jenkins/model/RunIdMigrator.java b/core/src/main/java/jenkins/model/RunIdMigrator.java index 71126c103c..61fe4c48ae 100644 --- a/core/src/main/java/jenkins/model/RunIdMigrator.java +++ b/core/src/main/java/jenkins/model/RunIdMigrator.java @@ -175,7 +175,7 @@ public final class RunIdMigrator { } cp.append(locationS); } - LOGGER.log(WARNING, "Build record migration is one-way. If you need to downgrade Jenkins, run: java -classpath {0} {1} {2}", new Object[] {cp, RunIdMigrator.class.getName(), jenkinsHome}); + LOGGER.log(WARNING, "Build record migration is one-way. If you need to downgrade Jenkins, run: java -classpath {0} {1} \"{2}\"", new Object[] {cp, RunIdMigrator.class.getName(), jenkinsHome}); } return true; } From 711b3325adf629585d60aa5a2bcac1d49b8324ae Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Mon, 22 Dec 2014 17:19:04 -0800 Subject: [PATCH 23/23] [JENKINS-24380] - Recorded the fix in changelog. - Added a separate screen to show the unmigration instruction, which makes it easier for people to find it than search logs. --- changelog.html | 55 +++++++------- .../java/jenkins/model/RunIdMigrator.java | 75 ++++++++++++++----- .../UnmigrationInstruction/index.jelly | 47 ++++++++++++ 3 files changed, 133 insertions(+), 44 deletions(-) create mode 100644 core/src/main/resources/jenkins/model/RunIdMigrator/UnmigrationInstruction/index.jelly diff --git a/changelog.html b/changelog.html index 62718805e9..824da3ec75 100644 --- a/changelog.html +++ b/changelog.html @@ -55,6 +55,11 @@ Upcoming changes