-
Notifications
You must be signed in to change notification settings - Fork 130
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-26398] More flexible versions of createSlave
#428
Conversation
} | ||
// TODO 2.216+ support WebSocket option | ||
DumbSlave s = new DumbSlave(name, new File(r.jenkins.getRootDir(), "agent-work-dirs/" + name).getAbsolutePath(), new JNLPLauncher(true)); | ||
s.setNumExecutors(1); // TODO pending 2.234+ |
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.
A common use case I have seen in plugins is the desire to create an agent with more than one executor for complex testing scenarios, for example in throttle-concurrent-builds
. While I recognize that this is not a use case you are intending to address in this PR, I think it would be worth thinking about how to accommodate such a use case in a future change and designing the API to support (or at least not preclude) such a future use case.
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.
Nothing precludes you from calling setNumExecutors
on the return value.
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.
But if I recall correctly that might be too late, since we would have already launched the agent with the wrong number of executors, and setNumExecutors
doesn't relaunch it. And it is awkward to launch with the wrong number and then relaunch with the correct number. So I think to accommodate such a future use case the API that launches the agent would need to support customizing the number of executors prior to launch. If the API you are proposing here can be extended to support that use case, great.
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.
Since this requirement is pretty rare, and in fact there are only a handful of known use cases where specifically inbound agent is required (regular createSlave
handles other cases), I am not inclined to complicate the API with an unused parameter. If and when somebody needs something like that, they could propose an API addition for it, or simply write their own agent launch code as was done previously.
"-Xmx512m", | ||
"-XX:+PrintCommandLineFlags", | ||
"-Djava.awt.headless=true")); | ||
if (JenkinsRule.SLAVE_DEBUG_PORT > 0) { |
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.
Note: untested; I am not even exactly sure how you arrange for this property to be set in the Surefire JVM. Adapted from
SLAVE_DEBUG_PORT>0 ? " -Xdebug -Xrunjdwp:transport=dt_socket,server=y,address="+(SLAVE_DEBUG_PORT+sz): "", |
jenkins-test-harness/src/main/java/org/jvnet/hudson/test/RealJenkinsRule.java
Lines 466 to 468 in 5ffb9b2
if (new DisableOnDebug(null).isDebugging()) { | |
argv.add("-agentlib:jdwp=transport=dt_socket,server=y"); | |
} |
Causes
|
…lave` (cherry picked from commit b888323)
JENKINS-26398
Inspired by changes being made in jenkinsci/workflow-durable-task-step-plugin#180. Cleans up some tech debt from jenkinsci/pipeline-plugin#34 & jenkinsci/credentials-binding-plugin#3.
createSlave
overloads to use a more stable agent workdir compatible withJenkinsSessionRule
.InboundAgentRule
which is better suited to simulating agent disconnections.