-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
[JENKINS-39370,JENKINS-39369] - Support of work directories in Remoting #129
Changes from 1 commit
d0abdf6
c657661
48fe307
ece2848
8550a79
af01961
ecc845f
374f423
7bcbff4
aa10398
ac05a22
b465606
9dc3559
fe336a2
4b7ee11
813135f
ca68f83
85384eb
ddd23f3
0e73eeb
d7ef457
151c93e
21d8c80
658095b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,12 +27,15 @@ | |
import hudson.remoting.Channel.Mode; | ||
import java.io.FileInputStream; | ||
import java.io.UnsupportedEncodingException; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.security.KeyStore; | ||
import java.security.KeyStoreException; | ||
import java.security.NoSuchProviderException; | ||
import java.security.PrivilegedActionException; | ||
import java.security.cert.CertificateFactory; | ||
import javax.annotation.CheckForNull; | ||
import javax.annotation.Nonnull; | ||
import javax.net.ssl.SSLSocketFactory; | ||
import javax.net.ssl.TrustManagerFactory; | ||
import org.jenkinsci.remoting.util.IOUtils; | ||
|
@@ -111,7 +114,14 @@ public class Launcher { | |
@Option(name="-ping") | ||
public boolean ping = true; | ||
|
||
@Option(name="-slaveLog", usage="create local slave error log") | ||
//TODO: rename the option | ||
/** | ||
* Specifies a destination for error logs. | ||
* If specified, this option overrides the default destination within {@link #workDir}. | ||
* If both this options and {@link #workDir} is not set, the log will not be generated. | ||
*/ | ||
@Option(name="-slaveLog", usage="Local agent error log destination (overrides workDir)") | ||
@CheckForNull | ||
public File slaveLog = null; | ||
|
||
@Option(name="-text",usage="encode communication with the master with base64. " + | ||
|
@@ -210,6 +220,33 @@ public boolean verify(String s, SSLSession sslSession) { | |
usage = "Disable TCP socket keep alive on connection to the master.") | ||
public boolean noKeepAlive = false; | ||
|
||
/** | ||
* Specifies a default working directory of the remoting instance. | ||
* If specified, this directory will be used to store logs, JAR cache, etc. | ||
* <p> | ||
* In order to retain compatibility, the option is disabled by default. | ||
* <p> | ||
* Jenkins specifics: This working directory is expected to be equal to the agent root specified in Jenkins configuration. | ||
* @since TODO | ||
*/ | ||
@Option(name = "-workDir", | ||
usage = "Declares the working directory of the remoting instance (stores cache and logs by default)") | ||
@CheckForNull | ||
public File workDir = null; | ||
|
||
/** | ||
* Specifies a directory within {@link #workDir}, which stores all the remoting-internal files. | ||
* <p> | ||
* This option is not expected to be used frequently, but it allows remoting users to specify a custom | ||
* storage directory if the default {@code remoting} directory is consumed by other stuff. | ||
* @since TODO | ||
*/ | ||
@Option(name = "-internalDir", | ||
usage = "Specifies a name of the internal files within a working directory ('remoting' by default)", | ||
depends = "-workDir") | ||
@Nonnull | ||
public String internalDir = "remoting"; | ||
|
||
public static void main(String... args) throws Exception { | ||
Launcher launcher = new Launcher(); | ||
CmdLineParser parser = new CmdLineParser(launcher); | ||
|
@@ -226,9 +263,46 @@ public static void main(String... args) throws Exception { | |
|
||
@edu.umd.cs.findbugs.annotations.SuppressWarnings("DM_DEFAULT_ENCODING") // log file, just like console output, should be in platform default encoding | ||
public void run() throws Exception { | ||
if (slaveLog!=null) { | ||
|
||
// Create and verify working directory if required | ||
@CheckForNull Path internalDirPath = workDir != null ? workDir.toPath() : null; | ||
if (internalDirPath == null) { | ||
System.err.println("WARNING: Agent working directory is not specified. Some functionality introduced in Remoting 3 may be disabled"); | ||
} else { | ||
if (workDir.exists()) { | ||
if (!workDir.isDirectory()) { | ||
throw new IOException("The specified agent working directory path points to a non-directory file"); | ||
} | ||
if (!workDir.canWrite() || !workDir.canRead() || !workDir.canExecute()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should/couldn't it just (try to) create it? Like for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It creates it on-demand. This code is being executed only for existing files |
||
throw new IOException("The specified agent working directory should be fully accessible to the remoting executable (RWX)"); | ||
} | ||
} | ||
|
||
// Now we create a subdirectory for remoting operations | ||
internalDirPath = new File(workDir, internalDir).toPath(); | ||
Files.createDirectories(internalDirPath); | ||
System.out.println("Using " + internalDirPath + " as a remoting working files directory"); | ||
} | ||
|
||
if (slaveLog != null) { // Legacy behavior | ||
System.out.println("Using " + slaveLog + " as an agent Error log destination. 'Out' log won't be generated"); | ||
System.out.flush(); // Just in case the channel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing words/sentence end in the comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep |
||
System.err.flush(); | ||
System.setErr(new PrintStream(new TeeOutputStream(System.err,new FileOutputStream(slaveLog)))); | ||
} else if (internalDirPath != null) { // New behavior | ||
System.out.println("Both error and output logs will be printed to " + internalDirPath); | ||
System.out.flush(); | ||
System.err.flush(); | ||
|
||
// TODO: Log rotation by default? | ||
System.setErr(new PrintStream(new TeeOutputStream(System.err, | ||
new FileOutputStream(new File(internalDirPath.toFile(), "remoting.err.log"))))); | ||
System.setOut(new PrintStream(new TeeOutputStream(System.out, | ||
new FileOutputStream(new File(internalDirPath.toFile(), "remoting.out.log"))))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if this wouldn't be easier, for diagnostics purposes, to just output in a single file, and use JUL (I don't like it very much, but as it's already present in the JDK and Jenkins code already uses it in general...) or another logging API to distinguish between messages severities. E.g. here we don't have the dates of the logs, so it would make btw it hard or impossible to correlate .err logs to .out ones, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was considering it, but unfortunately SOME of executions on agents in plugin implementations write to STDOUT/STDERR instead of using JUL or whatever other logging. Hence we have to maintain such low-level API. Otherwise I would just implement JUL appender |
||
} else { | ||
System.err.println("WARNING: Log location is not specified (neither -workDir nor -slaveLog set)"); | ||
} | ||
|
||
if(auth!=null) { | ||
final int idx = auth.indexOf(':'); | ||
if(idx<0) throw new CmdLineException(null, "No ':' in the -auth option"); | ||
|
@@ -256,6 +330,10 @@ public void run() throws Exception { | |
if (this.noKeepAlive) { | ||
jnlpArgs.add("-noKeepAlive"); | ||
} | ||
if (this.workDir != null) { | ||
jnlpArgs.add("-workDir"); | ||
jnlpArgs.add(workDir.getPath()); | ||
} | ||
if (candidateCertificates != null && !candidateCertificates.isEmpty()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pass through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is https://issues.jenkins-ci.org/browse/JENKINS-39817 for it. I'm not a big fan of adding this option since we are going to have the |
||
for (String c: candidateCertificates) { | ||
jnlpArgs.add("-cert"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Option(name="-agentLog", aliases={"-slaveLog"}, ...
and be done with the renameThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering renaming.
My main concern about
agent
is that we will haveJenkins agent
andremoting agent
terms. And they won't be equal in general.