-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Let the CI pass the appropriate forkCount #7110
Conversation
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.
I guess
Lines 46 to 60 in e80100a
def mavenOptions = [ | |
'-Pdebug', | |
'-Penable-jacoco', | |
'--update-snapshots', | |
"-Dmaven.repo.local=$m2repo", | |
'-Dmaven.test.failure.ignore', | |
'-Dspotbugs.failOnError=false', | |
'-Dcheckstyle.failOnViolation=false', | |
'-Dset.changelist', | |
'help:evaluate', | |
'-Dexpression=changelist', | |
"-Doutput=$changelistF", | |
'clean', | |
'install', | |
] |
-DforkCount=2
to match?
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 in its current form this PR is not setting the forkCount
for the test/
module to any value, and since the default forkCount
is 1, I think this will have the practical effect of doubling the test time for the test/
module, which will likely cause this PR build to time out and fail.
While I do not understand the motivation for this change, I am going to assume (rather than applying the needs-justification
label) that this is because it is undesirable to make an assumption about the processing power of the execution environment in the build. Rather it seems desirable to allow the execution environment to set this value, e.g. in its Jenkinsfile
(for CI builds) or in the options passed to mvn
(for interactive builds).
If that is the motivation, then why not take it to its logical conclusion by also removing forkCount
from the core/
module as well? Then, we would need to define forkCount
in the Jenkinsfile
to something reasonable. Since it is currently 0.5C for the core/
module and 2 for the test/
, I think using a value of 2 in the Jenkinsfile
would be reasonable, and would also make test runs more deterministic (a plus!).
I missed the fact that it was also set in core. I agree that it should rather be set in the |
Jenkinsfile
Outdated
@@ -49,6 +49,7 @@ for (i = 0; i < buildTypes.size(); i++) { | |||
'--update-snapshots', | |||
"-Dmaven.repo.local=$m2repo", | |||
'-Dmaven.test.failure.ignore', | |||
'-DforkCount=0.5C', |
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.
I do not know how many cores our container test environments have, so I am unable to assess whether this is reasonable or not. I do know for sure that setting this to 2 would retain existing behavior for the test/
module (no regressions) and would likely not impact the core/
module, which only takes a few minutes to run anyway, (no regressions). So while I would be a +1 for a value of 2, I am undecided on a value of 0.5C pending an explanation of what that would translate to in practice on our container agents.
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.
In practice they seem equivalent (2 tests in parallel). https://github.com/jenkins-infra/documentation/blob/main/ci.adoc#jenkins-on-jenkins doesn't say anything about specs of default containers (unlike VMs), would probably be valuable.
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 it could easily change in the future and cause unexpected breakage. I think an explicit value of 2 would be less fragile.
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.
https://github.com/jenkins-infra/documentation/blob/main/ci.adoc#jenkins-on-jenkins doesn't say anything about specs of default containers (unlike VMs), would probably be valuable.
This page needs an update indeed, issue opened.
Here and below you can see the number of CPU and mem allocated to the kubernetes agents.
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
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.
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!
cf. jenkinsci/plugin-pom#269 (comment)
See JENKINS-XXXXX.
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgrade@Restricted
or have@since TODO
Javadoc, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
if applicable.eval
to ease future introduction of Content-Security-Policy directives (see documentation on jenkins.io).Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are accurate, human-readable, and in the imperative moodupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).