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

Allow slave process to use the slave memory specified in the configuration #30

Closed
wants to merge 3 commits into from

Conversation

lost-a-tooth
Copy link

Jenkins Slave Memory wasn't used to start slave process.

@cloudbees-pull-request-builder

plugins » mesos-plugin #74 SUCCESS
This pull request looks good

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@@ -312,7 +312,7 @@ private void createMesosTask(Offer offer, Request request) {
CommandInfo
.newBuilder()
.setValue(
String.format(SLAVE_COMMAND_FORMAT, request.request.mem,
String.format(SLAVE_COMMAND_FORMAT, request.request.slaveJarMem,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. Why is the slave being started with "slaveJarMem" instead of "request.request.mem"? Don't we have to account for Jenkins executor's memory? Do Jenkins executors get their own JVM?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, when executors execute builds, they are spurned as new processes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks.

@vinodkone
Copy link
Contributor

Also, please squash the commits.

@vinodkone
Copy link
Contributor

Can you rebase and address comments? If you don't want to create and pass "Resources" object in this review, you could just pass "numExecutors, slaveCpus, slaveMem, execuorCpus and executorMem" around instead of "cpus, mem and slaveJarMem". I think thats cleaner.

@cloudbees-pull-request-builder

plugins » mesos-plugin #85 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

plugins » mesos-plugin #90 SUCCESS
This pull request looks good


public SlaveRequest(JenkinsSlave slave, double cpus, int mem, String label, String jvmArgs) {
public SlaveRequest(JenkinsSlave slave, double cpus, int mem, String label, String jvmArgs, int slaveMem) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets make 'slaveMem' the 4th argument (after "mem") instead of last.

@vinodkone
Copy link
Contributor

Hey Chris. Made some minor comments. Once you fix them and squash all the commits into one, I will merge it. Thanks!

@vinodkone
Copy link
Contributor

Chris, if you can rebase and squash the commits (after addressing the comments) i can merge it and release 0.3.

@vinodkone
Copy link
Contributor

This PR is quite stale at this point. Please send a new one if this fix is still needed.

@vinodkone vinodkone closed this Oct 12, 2014
jeschkies added a commit that referenced this pull request Nov 1, 2019
Summary:
This enables the plugin to actually run builds on Mesos. I validated the
behavior manually since a full integration test requires some work with
the Jenkins fixture. To get everything working I added a template for
pod specs that is configured by users and introduced the provisioning
strategy.

We also go back to Java 8 since some Jenkins components do not
support Java 11 yet.

JIRA issues: DCOS_OSS-5056
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants