[FIX JENKINS-39607] Gather GC logs also with rotation enabled

jenkinsci · Nov 9, 2016 · 41fcbfd · 41fcbfd
1 parent 8711174
commit 41fcbfd
Show file tree

Hide file tree

Showing 2 changed files with 89 additions and 15 deletions.
diff --git a/src/main/java/com/cloudbees/jenkins/support/impl/GCLogs.java b/src/main/java/com/cloudbees/jenkins/support/impl/GCLogs.java
@@ -9,11 +9,14 @@
 import jenkins.model.Jenkins;
 
 import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 import java.io.File;
+import java.io.FilenameFilter;
 import java.lang.management.ManagementFactory;
 import java.util.Collections;
 import java.util.Set;
 import java.util.logging.Logger;
+import java.util.regex.Pattern;
 
 /**
  * GC Logs Retriever.
@@ -27,7 +30,11 @@
 @Extension(ordinal = 90.0) // probably big too, see JenkinsLogs
 public class GCLogs extends Component {
 
-    static final String GCLOG_JRE_SWITCH = "-Xloggc:";
+    static final String GCLOGS_JRE_SWITCH = "-Xloggc:";
+
+    static final String GCLOGS_ROTATION_SWITCH = "-XX:+UseGCLogFileRotation";
+
+    private static final String GCLOGS_BUNDLE_ROOT = "/nodes/master/logs/gc/";
 
     private static final Logger LOGGER = Logger.getLogger(GCLogs.class.getName());
 
@@ -71,26 +78,75 @@ public void addContents(@NonNull Container result) {
         String gcLogFileLocation = getGcLogFileLocation();
         assert gcLogFileLocation != null; // non nullable here 'cause isEnabled() already checks it
 
-        // TODO : log rotation improved logic
+        if (isGcLogRotationConfigured()) {
+            handleRotatedLogs(gcLogFileLocation, result);
+        } else {
+            File file = new File(gcLogFileLocation);
+            if (!file.exists()) {
+                LOGGER.warning("[Support Bundle] GC Logging apparently configured, " +
+                        "but file '" + gcLogFileLocation + "' not found");
+                return;
+            }
+            result.add(new FileContent(GCLOGS_BUNDLE_ROOT + "gc.log", file));
+        }
+    }
+
+    /**
+     * Two cases:
+     * <ul>
+     * <li>The file name contains <code>%t</code> or <code>%p</code> somewhere in the middle:
+     * then we are simply going to replace those by <code>.*</code> to find associated logs and match files by regex.
+     * This will match GC logs from the current JVM execution, or possibly previous ones.</li>
+     * <li>or that feature is not used, then we simply match by "starts with"</li>
+     * </ul>
+     *
+     * @param gcLogFileLocation the specified value after <code>-Xloggc:</code>
+     * @param result            the container where to add the found logs, if any.
+     * @see https://bugs.openjdk.java.net/browse/JDK-7164841
+     */
+    private void handleRotatedLogs(@Nonnull final String gcLogFileLocation, Container result) {
+        // always add .* in the end because this is where the numbering is going to happen
+        String regex = gcLogFileLocation.replaceAll("%[pt]", ".*") + ".*";
+        final Pattern gcLogFilesPattern = Pattern.compile(regex);
+
+        File parentDirectory = new File(gcLogFileLocation).getParentFile();
 
-        File file = new File(gcLogFileLocation);
-        if (!file.exists()) {
-            LOGGER.warning("[Support Bundle] GC Logging apparently configured, " +
-                    "but file '" + gcLogFileLocation + "' not found");
+        if (parentDirectory == null || !parentDirectory.exists()) {
+            LOGGER.warning("[Support Bundle] " + parentDirectory + " does not exist, cannot collect gc logging files.");
+            return;
+        }
+
+        File[] gcLogs = parentDirectory.listFiles(new FilenameFilter() {
+            @Override
+            public boolean accept(File dir, String name) {
+                return gcLogFilesPattern.matcher(dir + "/" + name).matches();
+            }
+        });
+        if (gcLogs == null || gcLogs.length == 0) {
+            LOGGER.warning("No GC logging files found, although the VM argument was found. This is probably a bug.");
             return;
         }
-        result.add(new FileContent("/nodes/master/logs/gc.log", file));
+
+        LOGGER.finest("Found " + gcLogs.length + " matching files in " + parentDirectory.getAbsolutePath());
+        for (File gcLog : gcLogs) {
+            LOGGER.finest("Adding '" + gcLog.getName() + "' file");
+            result.add(new FileContent(GCLOGS_BUNDLE_ROOT + gcLog.getName(), gcLog));
+        }
     }
 
     @CheckForNull
     private String getGcLogFileLocation() {
 
-        String gcLogSwitch = vmArgumentFinder.findVmArgument(GCLOG_JRE_SWITCH);
+        String gcLogSwitch = vmArgumentFinder.findVmArgument(GCLOGS_JRE_SWITCH);
         if (gcLogSwitch == null) {
-            LOGGER.fine("No GC Logging switch found, no GC logs will be gathered.");
+            LOGGER.fine("No GC Logging switch found, cannot collect gc logging files.");
             return null;
         }
-        return gcLogSwitch.substring(GCLOG_JRE_SWITCH.length());
+        return gcLogSwitch.substring(GCLOGS_JRE_SWITCH.length());
+    }
+
+    private boolean isGcLogRotationConfigured() {
+        return vmArgumentFinder.findVmArgument(GCLOGS_ROTATION_SWITCH) != null;
     }
 
     /**

diff --git a/src/test/java/com/cloudbees/jenkins/support/impl/GCLogsTest.java b/src/test/java/com/cloudbees/jenkins/support/impl/GCLogsTest.java
@@ -17,14 +17,12 @@
 public class GCLogsTest {
 
     @Test
-    public void testSimpleFile() throws Exception {
+    public void simpleFile() throws Exception {
         File tmpFile = File.createTempFile("gclogs", "");
         Files.touch(tmpFile);
 
         GCLogs.VmArgumentFinder finder = mock(GCLogs.VmArgumentFinder.class);
-        System.out.println(tmpFile.exists());
-        System.out.println(tmpFile);
-        when(finder.findVmArgument(GCLogs.GCLOG_JRE_SWITCH)).thenReturn(GCLogs.GCLOG_JRE_SWITCH + tmpFile.getAbsolutePath());
+        when(finder.findVmArgument(GCLogs.GCLOGS_JRE_SWITCH)).thenReturn(GCLogs.GCLOGS_JRE_SWITCH + tmpFile.getAbsolutePath());
 
         TestContainer container = new TestContainer();
 
@@ -33,6 +31,27 @@ public void testSimpleFile() throws Exception {
         assertEquals(1, container.getContents().size());
     }
 
+    @Test
+    public void rotatedFiles() throws Exception {
+        File tempDir = Files.createTempDir();
+        for (int count = 0; count < 10; count++) {
+            Files.touch(new File(tempDir, "gc5625.log" + count));
+        }
+        for (int count = 0; count < 5; count++) {
+            Files.touch(new File(tempDir, "gc3421.log" + count));
+        }
+
+        GCLogs.VmArgumentFinder finder = mock(GCLogs.VmArgumentFinder.class);
+        when(finder.findVmArgument(GCLogs.GCLOGS_JRE_SWITCH)).thenReturn(GCLogs.GCLOGS_JRE_SWITCH + new File(tempDir, "gc%p.log").getAbsolutePath());
+        when(finder.findVmArgument(GCLogs.GCLOGS_ROTATION_SWITCH)).thenReturn(GCLogs.GCLOGS_ROTATION_SWITCH);
+
+        TestContainer container = new TestContainer();
+
+        new GCLogs(finder).addContents(container);
+
+        assertEquals(15, container.getContents().size());
+    }
+
     private static class TestContainer extends Container {
         List<Content> contents = new ArrayList<Content>();
 
@@ -44,6 +63,5 @@ public List<Content> getContents() {
         public void add(@CheckForNull Content content) {
             contents.add(content);
         }
-
     }
 }