Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop agents before exiting test cases, and reliably close all_*.log when agents exit #624

Merged
merged 17 commits into from
Feb 19, 2025
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@
<!-- https://www.jenkins.io/doc/developer/plugin-development/choosing-jenkins-baseline/ -->
<jenkins.baseline>2.479</jenkins.baseline>
<jenkins.version>${jenkins.baseline}.1</jenkins.version>
<!-- TODO until in parent -->
<jenkins-test-harness.version>2403.v256947ecb_c8a_</jenkins-test-harness.version>
Copy link
Member Author

@jglick jglick Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jenkinsci/jenkins-test-harness#922

Previously, the agent JVM launched by SimpleCommandLauncher (JenkinsRule.createSlave / .createOnlineSlave) would be terminated when the Computer was disconnected as part of Jenkins.cleanUp, but asynchronously and possible slightly later, when TemporaryDirectoryAllocator was already trying to clean up.

<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
<spotless.check.skip>false</spotless.check.skip>
</properties>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ private void setFile(File file) throws FileNotFoundException {
parentFile.mkdirs();
}

StreamUtils.closeQuietly(null); // ensure class is loaded so close() can succeed
Copy link
Member Author

@jglick jglick Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does nothing but loads this class. Otherwise there can occasionally be a NoClassDefFoundError from close during agent shutdown, at which time it is too late to load new classes from RemoteClassLoader because the connection has already been closed.

boolean success = false;
FileOutputStream fos = null;
BufferedOutputStream bos = null;
Expand Down
13 changes: 9 additions & 4 deletions src/main/java/com/cloudbees/jenkins/support/SupportPlugin.java
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,15 @@
// avoid double installation of the handler. JNLP agents can reconnect to the controller multiple times
// and each connection gets a different RemoteClassLoader, so we need to evict them by class name,
// not by their identity.
closeAll();
Runtime.getRuntime().addShutdownHook(new Thread(LogInitializer::closeAll, "close log handlers"));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensures that the file handle is closed before the JVM exits. Likely irrelevant on Linux, but seems to matter on Windows, where the OS will release mandatory file locks “sometime” after the process exits but not necessarily quickly enough for a test.

LogHolder.AGENT_LOG_HANDLER.setLevel(level);
LogHolder.AGENT_LOG_HANDLER.setDirectory(new File(rootPath.getRemote(), SUPPORT_DIRECTORY_NAME), "all");
ROOT_LOGGER.addHandler(LogHolder.AGENT_LOG_HANDLER);
return null;

Check warning on line 870 in src/main/java/com/cloudbees/jenkins/support/SupportPlugin.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 865-870 are not covered by tests
}

private static void closeAll() {
for (Handler h : ROOT_LOGGER.getHandlers()) {
if (h.getClass()
.getName()
Expand All @@ -874,10 +883,6 @@
}
}
}
LogHolder.AGENT_LOG_HANDLER.setLevel(level);
LogHolder.AGENT_LOG_HANDLER.setDirectory(new File(rootPath.getRemote(), SUPPORT_DIRECTORY_NAME), "all");
ROOT_LOGGER.addHandler(LogHolder.AGENT_LOG_HANDLER);
return null;
}
}

Expand Down
Loading