From be28d586b5db3cd3a5b7bbbfa265e73e86b6446b Mon Sep 17 00:00:00 2001 From: stellargo Date: Tue, 21 Jul 2020 09:50:47 +0530 Subject: [PATCH] [JENKINS-62755] Introduce Fingerprint Cleanup in External Storage API (#4817) * Create new FingerprintCleanupThread * Moved test to jenkins * Add license * Fix license * Fix some tests * Fix all tests * Remove redundant imports * Add documentation; add test * Fix errors in fingerprintertest * Remove default fingerprint cleanup implementation * Remove override from execute * Undo author deletion * Remove redundant import * Move fingerprint cleanup to storage * Fix spacing * Add new line * Minor fixes * Fix tests * Add documentation * Introduce fingerprint cleanup toggle * Link cleanup config to thread * Add file system cleanup * Add test * Introduce external fingerprint storage cleanup tests * Remove redundant statements * Fix test * Improve docstring * Introduce shouldNotCleanFingerprintsWhenDisabled test * Fix default cleanup value * Minor fixes * Enabled->Disabled * Fix tests * get -> is --- .../model/FingerprintCleanupThread.java | 90 ++------- .../fingerprints/FileFingerprintStorage.java | 79 ++++++++ .../fingerprints/FingerprintStorage.java | 63 ++++++ .../GlobalFingerprintConfiguration.java | 10 + .../config.jelly | 11 +- .../model/FingerprintCleanupThreadTest.java | 182 +++++++++++++++--- 6 files changed, 322 insertions(+), 113 deletions(-) diff --git a/core/src/main/java/hudson/model/FingerprintCleanupThread.java b/core/src/main/java/hudson/model/FingerprintCleanupThread.java index 89c21ea236..fea8bb2ac0 100644 --- a/core/src/main/java/hudson/model/FingerprintCleanupThread.java +++ b/core/src/main/java/hudson/model/FingerprintCleanupThread.java @@ -25,20 +25,14 @@ package hudson.model; import hudson.Extension; import hudson.ExtensionList; -import hudson.Functions; import jenkins.fingerprints.FileFingerprintStorage; import jenkins.fingerprints.FingerprintStorage; -import jenkins.model.FingerprintFacet; -import jenkins.model.Jenkins; +import jenkins.fingerprints.GlobalFingerprintConfiguration; import org.jenkinsci.Symbol; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; -import java.io.File; -import java.io.IOException; -import java.util.Date; import java.util.logging.Logger; -import java.util.regex.Pattern; /** * Scans the fingerprint database and remove old records @@ -54,8 +48,6 @@ import java.util.regex.Pattern; @Restricted(NoExternalUse.class) public class FingerprintCleanupThread extends AsyncPeriodicWork { - static final String FINGERPRINTS_DIR_NAME = "fingerprints"; - private static final Pattern FINGERPRINT_FILE_PATTERN = Pattern.compile("[0-9a-f]{28}\\.xml"); private static final Logger LOGGER = Logger.getLogger(FingerprintCleanupThread.class.getName()); public FingerprintCleanupThread() { @@ -74,81 +66,21 @@ public class FingerprintCleanupThread extends AsyncPeriodicWork { return ExtensionList.lookup(AsyncPeriodicWork.class).get(FingerprintCleanupThread.class); } + /** + * Initiates the cleanup of fingerprints IF enabled. + * In case of configured external storage, the file system based storage cleanup is also performed. + */ public void execute(TaskListener listener) { - Object fingerprintStorage = FingerprintStorage.get(); - if (!(fingerprintStorage instanceof FileFingerprintStorage)) { - LOGGER.fine("External fingerprint storage is configured. Skipping execution"); + if (GlobalFingerprintConfiguration.get().isFingerprintCleanupDisabled()) { + LOGGER.fine("Fingerprint cleanup is disabled. Skipping execution"); return; } + FingerprintStorage.get().iterateAndCleanupFingerprints(listener); - int numFiles = 0; - - File root = new File(getRootDir(), FINGERPRINTS_DIR_NAME); - File[] files1 = root.listFiles(f -> f.isDirectory() && f.getName().length()==2); - if(files1!=null) { - for (File file1 : files1) { - File[] files2 = file1.listFiles(f -> f.isDirectory() && f.getName().length()==2); - for(File file2 : files2) { - File[] files3 = file2.listFiles(f -> f.isFile() && FINGERPRINT_FILE_PATTERN.matcher(f.getName()).matches()); - for(File file3 : files3) { - if(check(file3, listener)) - numFiles++; - } - deleteIfEmpty(file2); - } - deleteIfEmpty(file1); - } + if (!(FingerprintStorage.get() instanceof FileFingerprintStorage) && + FingerprintStorage.getFileFingerprintStorage().isReady()) { + FileFingerprintStorage.getFileFingerprintStorage().iterateAndCleanupFingerprints(listener); } - - listener.getLogger().println("Cleaned up "+numFiles+" records"); - } - - /** - * Deletes a directory if it's empty. - */ - private void deleteIfEmpty(File dir) { - String[] r = dir.list(); - if(r==null) return; // can happen in a rare occasion - if(r.length==0) - dir.delete(); - } - - /** - * Examines the file and returns true if a file was deleted. - */ - private boolean check(File fingerprintFile, TaskListener listener) { - try { - Fingerprint fp = loadFingerprint(fingerprintFile); - if (fp == null || (!fp.isAlive() && fp.getFacetBlockingDeletion() == null) ) { - listener.getLogger().println("deleting obsolete " + fingerprintFile); - fingerprintFile.delete(); - return true; - } else { - if (!fp.isAlive()) { - FingerprintFacet deletionBlockerFacet = fp.getFacetBlockingDeletion(); - listener.getLogger().println(deletionBlockerFacet.getClass().getName() + " created on " + new Date(deletionBlockerFacet.getTimestamp()) + " blocked deletion of " + fingerprintFile); - } - // get the fingerprint in the official map so have the changes visible to Jenkins - // otherwise the mutation made in FingerprintMap can override our trimming. - fp = getFingerprint(fp); - return fp.trim(); - } - } catch (IOException e) { - Functions.printStackTrace(e, listener.error("Failed to process " + fingerprintFile)); - return false; - } - } - - protected Fingerprint loadFingerprint(File fingerprintFile) throws IOException { - return FileFingerprintStorage.load(fingerprintFile); - } - - protected Fingerprint getFingerprint(Fingerprint fp) throws IOException { - return Jenkins.get()._getFingerprint(fp.getHashString()); - } - - protected File getRootDir() { - return Jenkins.get().getRootDir(); } } diff --git a/core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java b/core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java index 3c7b1cb343..0bd947acd3 100644 --- a/core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java +++ b/core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java @@ -25,14 +25,17 @@ package jenkins.fingerprints; import com.thoughtworks.xstream.converters.basic.DateConverter; import hudson.Extension; +import hudson.Functions; import hudson.Util; import hudson.XmlFile; import hudson.model.Fingerprint; +import hudson.model.TaskListener; import hudson.model.listeners.SaveableListener; import hudson.util.AtomicFileWriter; import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; +import jenkins.model.FingerprintFacet; import jenkins.model.Jenkins; import org.jenkinsci.Symbol; import org.kohsuke.accmod.Restricted; @@ -44,9 +47,11 @@ import java.io.EOFException; import java.io.File; import java.io.IOException; import java.io.PrintWriter; +import java.util.Date; import java.util.Map; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Pattern; /** * Default file system storage implementation for fingerprints. @@ -60,6 +65,8 @@ public class FileFingerprintStorage extends FingerprintStorage { private static final Logger logger = Logger.getLogger(FileFingerprintStorage.class.getName()); private static final DateConverter DATE_CONVERTER = new DateConverter(); + public static final String FINGERPRINTS_DIR_NAME = "fingerprints"; + private static final Pattern FINGERPRINT_FILE_PATTERN = Pattern.compile("[0-9a-f]{28}\\.xml"); @DataBoundConstructor public FileFingerprintStorage () {} @@ -216,6 +223,56 @@ public class FileFingerprintStorage extends FingerprintStorage { return new File(Jenkins.get().getRootDir(),"fingerprints").exists(); } + /** + * Perform Fingerprint cleanup. + */ + @Override + public void iterateAndCleanupFingerprints(TaskListener taskListener) { + int numFiles = 0; + + File root = new File(getRootDir(), FINGERPRINTS_DIR_NAME); + File[] files1 = root.listFiles(f -> f.isDirectory() && f.getName().length()==2); + if(files1!=null) { + for (File file1 : files1) { + File[] files2 = file1.listFiles(f -> f.isDirectory() && f.getName().length()==2); + for(File file2 : files2) { + File[] files3 = file2.listFiles(f -> f.isFile() && FINGERPRINT_FILE_PATTERN.matcher(f.getName()).matches()); + for(File file3 : files3) { + if(cleanFingerprint(file3, taskListener)) + numFiles++; + } + deleteIfEmpty(file2); + } + deleteIfEmpty(file1); + } + } + + taskListener.getLogger().println("Cleaned up "+numFiles+" records"); + } + + private boolean cleanFingerprint(File fingerprintFile, TaskListener listener) { + try { + Fingerprint fp = loadFingerprint(fingerprintFile); + if (fp == null || (!fp.isAlive() && fp.getFacetBlockingDeletion() == null) ) { + listener.getLogger().println("deleting obsolete " + fingerprintFile); + fingerprintFile.delete(); + return true; + } else { + if (!fp.isAlive()) { + FingerprintFacet deletionBlockerFacet = fp.getFacetBlockingDeletion(); + listener.getLogger().println(deletionBlockerFacet.getClass().getName() + " created on " + new Date(deletionBlockerFacet.getTimestamp()) + " blocked deletion of " + fingerprintFile); + } + // get the fingerprint in the official map so have the changes visible to Jenkins + // otherwise the mutation made in FingerprintMap can override our trimming. + fp = getFingerprint(fp); + return fp.trim(); + } + } catch (IOException e) { + Functions.printStackTrace(e, listener.error("Failed to process " + fingerprintFile)); + return false; + } + } + /** * The file we save our configuration. */ @@ -261,6 +318,28 @@ public class FileFingerprintStorage extends FingerprintStorage { return buf.toString(); } + /** + * Deletes a directory if it's empty. + */ + private void deleteIfEmpty(File dir) { + String[] r = dir.list(); + if(r==null) return; // can happen in a rare occasion + if(r.length==0) + dir.delete(); + } + + protected Fingerprint loadFingerprint(File fingerprintFile) throws IOException { + return FileFingerprintStorage.load(fingerprintFile); + } + + protected Fingerprint getFingerprint(Fingerprint fp) throws IOException { + return Jenkins.get()._getFingerprint(fp.getHashString()); + } + + protected File getRootDir() { + return Jenkins.get().getRootDir(); + } + @Extension public static class DescriptorImpl extends FingerprintStorageDescriptor { diff --git a/core/src/main/java/jenkins/fingerprints/FingerprintStorage.java b/core/src/main/java/jenkins/fingerprints/FingerprintStorage.java index 221f0d41d9..531817e03c 100644 --- a/core/src/main/java/jenkins/fingerprints/FingerprintStorage.java +++ b/core/src/main/java/jenkins/fingerprints/FingerprintStorage.java @@ -24,13 +24,19 @@ package jenkins.fingerprints; import edu.umd.cs.findbugs.annotations.CheckForNull; +import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; import hudson.ExtensionPoint; import java.io.IOException; +import java.util.Date; +import hudson.Functions; import hudson.model.AbstractDescribableImpl; import hudson.model.Fingerprint; +import hudson.model.TaskListener; +import jenkins.model.FingerprintFacet; +import jenkins.model.Jenkins; import org.kohsuke.accmod.restrictions.Beta; import org.kohsuke.accmod.Restricted; @@ -49,6 +55,13 @@ public abstract class FingerprintStorage extends AbstractDescribableImpl - - + + + + + - - + + diff --git a/test/src/test/java/hudson/model/FingerprintCleanupThreadTest.java b/test/src/test/java/hudson/model/FingerprintCleanupThreadTest.java index fce710dbd8..408d509068 100644 --- a/test/src/test/java/hudson/model/FingerprintCleanupThreadTest.java +++ b/test/src/test/java/hudson/model/FingerprintCleanupThreadTest.java @@ -23,6 +23,12 @@ */ package hudson.model; +import hudson.Extension; +import hudson.Util; +import jenkins.fingerprints.FileFingerprintStorage; +import jenkins.fingerprints.FingerprintStorage; +import jenkins.fingerprints.FingerprintStorageDescriptor; +import jenkins.fingerprints.GlobalFingerprintConfiguration; import org.junit.Rule; import org.junit.Test; @@ -34,10 +40,13 @@ import java.io.PrintStream; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; import jenkins.model.FingerprintFacet; import org.jvnet.hudson.test.JenkinsRule; +import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.io.FileMatchers.aReadableFile; import static org.hamcrest.core.Is.is; import static org.hamcrest.core.IsNot.not; @@ -51,8 +60,8 @@ import static org.hamcrest.MatcherAssert.assertThat; public class FingerprintCleanupThreadTest { private static final Fingerprint.BuildPtr ptr = new Fingerprint.BuildPtr("fred", 23); - private Path tempDirectory; - private Path fpFile; + private static Path tempDirectory; + private static Path fpFile; @Rule public JenkinsRule jenkinsRule = new JenkinsRule(); @@ -61,7 +70,8 @@ public class FingerprintCleanupThreadTest { public void testDoesNotLogUnimportantExcessiveLogMessage() throws IOException { createFolderStructure(); TestTaskListener testTaskListener = new TestTaskListener(); - FingerprintCleanupThread cleanupThread = new TestFingerprintCleanupThread(new TestFingerprint(true)); + configureLocalTestStorage(new TestFingerprint(true)); + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); cleanupThread.execute(testTaskListener); String logOutput = testTaskListener.outputStream.toString(); assertFalse("Should not have logged unimportant, excessive message.", logOutput.contains("possibly trimming")); @@ -71,7 +81,8 @@ public class FingerprintCleanupThreadTest { public void testFingerprintFileIsEmpty() throws IOException { createFolderStructure(); TestTaskListener testTaskListener = new TestTaskListener(); - FingerprintCleanupThread cleanupThread = new TestFingerprintCleanupThread(new TestFingerprint(false)); + configureLocalTestStorage(new TestFingerprint(false)); + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); cleanupThread.execute(testTaskListener); String logOutput = testTaskListener.outputStream.toString(); assertFalse("Should have deleted obsolete file.", fpFile.toFile().exists()); @@ -79,7 +90,7 @@ public class FingerprintCleanupThreadTest { @Test public void testGetRecurrencePeriod() throws IOException { - FingerprintCleanupThread cleanupThread = new TestFingerprintCleanupThread(new TestFingerprint()); + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); assertEquals("Wrong recurrence period.", PeriodicWork.DAY, cleanupThread.getRecurrencePeriod()); } @@ -87,22 +98,13 @@ public class FingerprintCleanupThreadTest { public void testNoFingerprintsDir() throws IOException { createTestDir(); TestTaskListener testTaskListener = new TestTaskListener(); - FingerprintCleanupThread cleanupThread = new TestFingerprintCleanupThread(new TestFingerprint()); + configureLocalTestStorage(new TestFingerprint()); + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); cleanupThread.execute(testTaskListener); String logOutput = testTaskListener.outputStream.toString(); assertTrue("Should have done nothing.", logOutput.startsWith("Cleaned up 0 records")); } - @Test - public void testIOExceptionOnLoad() throws IOException { - createFolderStructure(); - TestTaskListener testTaskListener = new TestTaskListener(); - FingerprintCleanupThread cleanupThread = new TestFingerprintCleanupThreadThrowsExceptionOnLoad(new TestFingerprint()); - cleanupThread.execute(testTaskListener); - String logOutput = testTaskListener.outputStream.toString(); - assertTrue("Should have logged IOException.", logOutput.contains("ERROR: Failed to process")); - } - @Test public void testBlockingFacetBlocksDeletion() throws IOException { createFolderStructure(); @@ -111,7 +113,8 @@ public class FingerprintCleanupThreadTest { fp.facets.setOwner(Saveable.NOOP); TestFingperprintFacet facet = new TestFingperprintFacet(fp, System.currentTimeMillis(), true); fp.facets.add(facet); - FingerprintCleanupThread cleanupThread = new TestFingerprintCleanupThread(fp); + configureLocalTestStorage(fp); + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); cleanupThread.execute(testTaskListener); String logOutput = testTaskListener.outputStream.toString(); assertThat(logOutput, containsString("blocked deletion of")); @@ -125,14 +128,83 @@ public class FingerprintCleanupThreadTest { fp.facets.setOwner(Saveable.NOOP); TestFingperprintFacet facet = new TestFingperprintFacet(fp, System.currentTimeMillis(), false); fp.facets.add(facet); - FingerprintCleanupThread cleanupThread = new TestFingerprintCleanupThread(fp); + configureLocalTestStorage(fp); + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); cleanupThread.execute(testTaskListener); assertThat(fpFile.toFile(), is(not(aReadableFile()))); } + @Test + public void testExternalStorageCleanupWithoutLocalFingerprints() throws IOException { + createFolderStructure(); + TestTaskListener testTaskListener = new TestTaskListener(); + Fingerprint fingerprint = new TestFingerprint(false); + configureExternalTestStorage(); + String fingerprintId = fingerprint.getHashString(); + + fingerprint.save(); + assertThat(Fingerprint.load(fingerprintId), is(not(nullValue()))); + + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); + cleanupThread.execute(testTaskListener); + assertThat(Fingerprint.load(fingerprintId), is(nullValue())); + } + + @Test + public void testExternalStorageCleanupWithLocalFingerprints() throws IOException { + TestTaskListener testTaskListener = new TestTaskListener(); + + String localFingerprintId = Util.getDigestOf("local"); + new Fingerprint((Run) null, "foo.jar", Util.fromHexString(localFingerprintId)); + + configureExternalTestStorage(); + String externalFingerprintId = Util.getDigestOf("local"); + new Fingerprint((Run) null, "bar.jar", Util.fromHexString(externalFingerprintId)); + + assertThat(Fingerprint.load(localFingerprintId), is(not(nullValue()))); + assertThat(Fingerprint.load(externalFingerprintId), is(not(nullValue()))); + + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); + cleanupThread.execute(testTaskListener); + + assertThat(Fingerprint.load(localFingerprintId), is(nullValue())); + assertThat(Fingerprint.load(externalFingerprintId), is(nullValue())); + } + + @Test + public void shouldNotCleanFingerprintsWhenDisabled() throws IOException { + GlobalFingerprintConfiguration.get().setFingerprintCleanupDisabled(true); + + TestTaskListener testTaskListener = new TestTaskListener(); + + String localFingerprintId = Util.getDigestOf("local"); + new Fingerprint((Run) null, "foo.jar", Util.fromHexString(localFingerprintId)); + + configureExternalTestStorage(); + String externalFingerprintId = Util.getDigestOf("local"); + new Fingerprint((Run) null, "bar.jar", Util.fromHexString(externalFingerprintId)); + + assertThat(Fingerprint.load(localFingerprintId), is(not(nullValue()))); + assertThat(Fingerprint.load(externalFingerprintId), is(not(nullValue()))); + + FingerprintCleanupThread cleanupThread = new FingerprintCleanupThread(); + cleanupThread.execute(testTaskListener); + + assertThat(Fingerprint.load(localFingerprintId), is(not(nullValue()))); + assertThat(Fingerprint.load(externalFingerprintId), is(not(nullValue()))); + } + + private void configureLocalTestStorage(Fingerprint fingerprint) { + GlobalFingerprintConfiguration.get().setStorage(new TestFileFingerprintStorage(fingerprint)); + } + + private void configureExternalTestStorage() { + GlobalFingerprintConfiguration.get().setStorage(new TestExternalFingerprintStorage()); + } + private void createFolderStructure() throws IOException { createTestDir(); - Path fingerprintsPath = tempDirectory.resolve(FingerprintCleanupThread.FINGERPRINTS_DIR_NAME); + Path fingerprintsPath = tempDirectory.resolve(FileFingerprintStorage.FINGERPRINTS_DIR_NAME); Files.createDirectory(fingerprintsPath); Path aaDir = fingerprintsPath.resolve("aa"); Files.createDirectory(aaDir); @@ -160,17 +232,17 @@ public class FingerprintCleanupThreadTest { } - private class TestFingerprintCleanupThread extends FingerprintCleanupThread { + private static class TestFileFingerprintStorage extends FileFingerprintStorage { private Fingerprint fingerprintToLoad; - public TestFingerprintCleanupThread(Fingerprint fingerprintToLoad) throws IOException { + public TestFileFingerprintStorage(Fingerprint fingerprintToLoad) { this.fingerprintToLoad = fingerprintToLoad; } @Override - protected Fingerprint getFingerprint(Fingerprint fp) throws IOException { - return new Fingerprint(ptr, "file", new byte[0]); + protected Fingerprint getFingerprint(Fingerprint fp) { + return new Fingerprint(ptr, "foo", Util.fromHexString(Util.getDigestOf("foo"))); } @Override @@ -183,6 +255,21 @@ public class FingerprintCleanupThreadTest { return fingerprintToLoad; } + @Override + public boolean isReady() { + return fpFile.toFile().exists(); + } + + @Extension + public static class DescriptorImpl extends FingerprintStorageDescriptor { + + @Override + public String getDisplayName() { + return "TestFileFingerprintStorage"; + } + + } + } public static final class TestFingperprintFacet extends FingerprintFacet { @@ -205,11 +292,11 @@ public class FingerprintCleanupThreadTest { private boolean isAlive = true; public TestFingerprint() throws IOException { - super(ptr, "fred", new byte[0]); + super(ptr, "foo", Util.fromHexString(Util.getDigestOf("foo"))); } public TestFingerprint(boolean isAlive) throws IOException { - super(ptr, "fred", new byte[0]); + super(ptr, "foo", Util.fromHexString(Util.getDigestOf("foo"))); this.isAlive = isAlive; } @@ -219,15 +306,50 @@ public class FingerprintCleanupThreadTest { } } - private class TestFingerprintCleanupThreadThrowsExceptionOnLoad extends TestFingerprintCleanupThread { + public static class TestExternalFingerprintStorage extends FingerprintStorage { - public TestFingerprintCleanupThreadThrowsExceptionOnLoad(Fingerprint fingerprintToLoad) throws IOException { - super(fingerprintToLoad); + Map storage = new HashMap<>(); + + @Override + public void save(Fingerprint fp) { + storage.put(fp.getHashString(), fp); } @Override - protected Fingerprint loadFingerprint(File fingerprintFile) throws IOException { - throw new IOException("Test exception"); + public Fingerprint load(String id) { + return storage.get(id); + } + + @Override + public void delete(String id) { + storage.remove(id); + } + + @Override + public boolean isReady() { + return storage.size() != 0; + } + + @Override + public void iterateAndCleanupFingerprints(TaskListener taskListener) { + for (Fingerprint fingerprint : storage.values()) { + cleanFingerprint(fingerprint, taskListener); + } + } + + @Override + protected Fingerprint getFingerprint(Fingerprint fp) throws IOException { + return new Fingerprint(ptr, "foo", Util.fromHexString(Util.getDigestOf("foo"))); + } + + @Extension + public static class DescriptorImpl extends FingerprintStorageDescriptor { + + @Override + public String getDisplayName() { + return "TestExternalFingerprintStorage"; + } + } } }