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

Disable daemon conditionally #6106

Merged
merged 2 commits into from Jul 27, 2018

Conversation

@blindpirate
Copy link
Member

blindpirate commented Jul 27, 2018

This is a follow-up of gradle/gradle-private#1352. The asciidoctor plugin
we're using has a serious issue that its native library sometimes cause the daemon JVM crash.
As a workaround, we identify the following builds need to run docs:docs task because they depend on
binZip task:

  • wrapper
  • toolingApi
  • distributions
  • performance test coordinators because they're running in the default pool

This commit disables daemon by removing --daemon from build parameter list. (TeamCity would automatically
add -Dorg.gradle.daemon=false at the end of the parameter list.)

Of course this is not a final solution but we need this to verify it actually works and to get rid of lots of daemon disappear issues.

@blindpirate blindpirate self-assigned this Jul 27, 2018
@blindpirate blindpirate force-pushed the blindpirate/disable-daemon branch from 64ba027 to 55f8929 Jul 27, 2018
@blindpirate blindpirate requested a review from wolfs Jul 27, 2018
@@ -38,3 +39,7 @@ class FunctionalTest(model: CIBuildModel, testCoverage: TestCoverage, subProject
}
}
})

fun requiresDaemon(subProjectName: String): Boolean {

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

Function can be private.

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

Sure.

@@ -29,7 +29,8 @@ class FunctionalTest(model: CIBuildModel, testCoverage: TestCoverage, subProject
+ buildScanTags.map { buildScanTag(it) }
+ buildScanValues.map { buildScanCustomValue(it.key, it.value) }
).joinToString(separator = " "),
timeout = testCoverage.testType.timeout)
timeout = testCoverage.testType.timeout,
daemon = requiresDaemon(subProject))

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

It seems to me that the decision whether or not run a functional test without the daemon belongs to the model (in this case I guess FunctionalTestProject would be the right place). WDYT?

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

I think the correct model seems to be GradleSubproject so I put requiresDaemon there.

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

Good call! I wonder if we should call the property requiresDaemon, though. How about useDaemon?

@@ -126,6 +126,7 @@ fun applyDefaults(model: CIBuildModel, buildType: BaseGradleBuildType, gradleTas
var gradleParameterString = gradleParameters.joinToString(separator = " ")
.replace(java7Homes[OS.linux]!!, java7HomeParameter)
.replace(java9Homes[OS.linux]!!, java9HomeParameter)
.let { if(daemon) it else it.replace("--daemon", "")}

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

I would prefer adding --no-daemon on our own, so we are not affected if Teamcity decides to change the default. WDYT?

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

I also considered this way, the only disadvantage I can think of is that --daemon --no-daemon would confuse users.

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

Not to mention --daemon --no-daemon -Dorg.gradle.daemon=false... these 3 parameters appearing together would drive us crazy...

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

I meant that, instead of removing --daemon, we should remove --daemon and add --no-daemon.

I agree with you that we shouldn't have --daemon --no-daemon.

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

Done.

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

So much better now!

@@ -126,6 +126,7 @@ fun applyDefaults(model: CIBuildModel, buildType: BaseGradleBuildType, gradleTas
var gradleParameterString = gradleParameters.joinToString(separator = " ")

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

Maybe gradleParameters could be a function which takes the os and whether to use the daemon or not as parameters. Then we won't need to do any String replacements here any more. WDYT?

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

Sounds like a good refactoring. I'll do it.

@@ -43,7 +43,7 @@ class PerformanceTest(model: CIBuildModel, type: PerformanceTestType, stage: Sta
+ listOf("clean distributed${type.taskId}s -x prepareSamples --baselines %performance.baselines% ${type.extraParameters} -PtimestampedVersion -Porg.gradle.performance.branchName=%teamcity.build.branch% -Porg.gradle.performance.db.url=%performance.db.url% -Porg.gradle.performance.db.username=%performance.db.username% -PteamCityUsername=%TC_USERNAME% -PteamCityPassword=%teamcity.password.restbot% -Porg.gradle.performance.buildTypeId=${IndividualPerformanceScenarioWorkers(model).id} -Porg.gradle.performance.workerTestTaskName=fullPerformanceTest -Porg.gradle.performance.coordinatorBuildId=%teamcity.build.id% -Porg.gradle.performance.db.password=%performance.db.password.tcagent%",
buildScanTag("PerformanceTest"))
+ model.parentBuildCache.gradleParameters(OS.linux)
).joinToString(separator = " ")
).joinToString(separator = " ").replace("--daemon", "")

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

I saw that the IndividualPerformanceScenarioWorkers also switch off the daemon:

gradleParams = (
gradleParameters.map { if (it == "--daemon") "--no-daemon" else it }
+ listOf("""clean %templates% fullPerformanceTests --scenarios "%scenario%" --baselines %baselines% --warmups %warmups% --runs %runs% --checks %checks% --channel %channel% -x prepareSamples -x performanceReport -Porg.gradle.performance.db.url=%performance.db.url% -Porg.gradle.performance.db.username=%performance.db.username% -Porg.gradle.performance.db.password=%performance.db.password.tcagent% -PtimestampedVersion""",
buildScanTag("IndividualPerformanceScenarioWorkers"))
+ model.parentBuildCache.gradleParameters(OS.linux)
).joinToString(separator = " ")

Do you know why this has been in place already?

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

You did it... 3faae67

And the commit message says it's intended to avoid some TC issue.

This comment has been minimized.

Copy link
@wolfs

wolfs Jul 27, 2018

Member

I just move the --no-daemon to a different place. It has been there before my commit.

This comment has been minimized.

Copy link
@blindpirate

blindpirate Jul 27, 2018

Author Member

Then I have no idea...7e1f8c7 expressly disable daemon for the individual workers, I guess @jjohannes thought it would avoid interfering performance tests but actually performance tests are executed in isolated processes, right?

This is a follow-up of gradle/gradle-private#1352. The asciidoctor plugin
we're using has a serious issue that its native library sometimes cause the daemon JVM crash.
As a workaround, we identify the following builds need to run `docs:docs` task because they depend on
`binZip` task:

- wrapper
- toolingApi
- distributions
- performance test coordinators because they're running in the default pool

This commit disables daemon by removing `--daemon` from build parameter list. (TeamCity would automatically
add `-Dorg.gradle.daemon=false` at the end of the parameter list.)
@blindpirate blindpirate force-pushed the blindpirate/disable-daemon branch from 55f8929 to 96aead4 Jul 27, 2018
Copy link
Member

wolfs left a comment

Looks very good!

The only thing I would still do is find a better name for requiresDaemon. useDaemon would be better I think.

@blindpirate blindpirate merged commit 69fc4f2 into teamcity-versioned-settings Jul 27, 2018
2 checks passed
2 checks passed
DCO All commits have a DCO sign-off from the author
Details
WIP ready for review
Details
@blindpirate blindpirate deleted the blindpirate/disable-daemon branch Jul 27, 2018
@adammurdoch

This comment has been minimized.

Copy link
Member

adammurdoch commented Jul 28, 2018

I think we should fix the upstream plugin instead of adding workarounds to our build.

blindpirate added a commit that referenced this pull request Jul 30, 2018
This is a follow-up for gradle/gradle-private#1352 and #6106 . We disable daemon for some builds and use this to make sure no `AsciidoctorTask` is running on daemon.
blindpirate added a commit that referenced this pull request Aug 1, 2018
In #6106 we disabled daemon for some certain
subprojects, however, all subprojects' `noDaemonTest` requires allDistribution, which
in turn invokes userguide task. This PR disabled daemon for `noDaemonTest`, given that
noDaemonTest is excuted once a day.
blindpirate added a commit that referenced this pull request Aug 2, 2018
In #6106 we disabled daemon for some certain
subprojects, however, all subprojects' `noDaemonTest` requires allDistribution, which
in turn invokes userguide task. This PR disabled daemon for `noDaemonTest`, given that
noDaemonTest is excuted once a day.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.