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.

Sign up for GitHub

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

Jump to bottom

Server logs should be written when integration tests fail #4804

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Server logs should be written when integration tests fail #4804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14a4c03
Server logs should be written when integration tests fail
niloc132 Nov 9, 2023
a406a21
review feedback
niloc132 Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 44 additions & 18 deletions buildSrc/src/main/groovy/Docker.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,34 @@ class Docker {

if (cfg.copyOut && !cfg.showLogsOnSuccess) {
// Single task with explicit inputs and outputs, to let gradle detect if it is up to date, and let docker
// cache what it can.
// cache what it can. While far more efficient for gradle to run (or rather, know when it does not need to
// run), it is also more bug-prone while we try to get various competing features working.
//
// To handle these use cases, we're not using dependsOn as we typically would do, but instead using
// finalizedBy and onlyIf. Here's a mermaid diagram:
//
// graph LR;
// MakeImage -. finalizedBy .-> Run
// Sync -- dependsOn --> MakeImage
// Run -. finalizedBy .-> Sync
//
//
// Unlike "A dependsOn B", "B finalized A" will let B run if A failed, and will not run A if B must run.
// Combining the chain of finalizedBys between MakeImage <- Run <- Sync with the dependsOn from
// Sync -> MakeImage lets us handle the following cases:
// * Successful run, output is sync'd afterwards, final task succeeds
// * Failed run, output is sync'd afterwards, final task fails
// * Failed image creation, no run, no sync, no final task
// * Previously successful run with no source changes, no tasks run (all "UP-TO-DATE")
//
// Tests to run to confirm functionality:
// * After changes, confirm that :web:assemble runs (isn't all "UP-TO-DATE")
// * Then run again with no changes, confirm all are UP-TO-DATE, roughly 2s build time
// * Edit a test that uses deephavenDocker to fail, confirm that the test fails, that the test-reports
// are copied out, and that server logs are written to console
// * Ensure that if the test is set to pass that the test-reports are copied out, and server logs are
// not written.
// Note that at this time integration tests using the deephavenDocker plugin are never UP-TO-DATE.

// Note that if "showLogsOnSuccess" is true, we don't run this way, since that would omit logs when cached.
def buildAndRun = project.tasks.register("${taskName}Run", CombinedDockerRunTask) { cacheableDockerTask ->
Expand Down Expand Up @@ -337,35 +364,34 @@ class Docker {
}
}

// Handle copying failure. This is now distinct from the "actual" Sync task that depends directly
// on the CombinedDockerRunTask.
def syncAfterFail = project.tasks.register("${taskName}SyncAfterFail", Sync) { sync ->
// Specify that makeImage is finalized by buildAndRun - that is, in this configuration buildAndRun
// must run after makeImage finishes
Comment on lines +367 to +368
Copy link
Member

@devinrsmith devinrsmith Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Your comment buildAndRun must run after makeImage is a bit at odds with the typical gradle terminology. I'm digging in now; I don't know if finalizedBy is the technically correct solution here (it may very well be...).

https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:finalizer_tasks

Finalizer tasks are useful in situations where the build creates a resource that has to be cleaned up regardless of the build failing or succeeding.

https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:ordering_tasks

When you use the “must run after” ordering rule you specify that taskB must always run after taskA, whenever both taskA and taskB will be run. This is expressed as taskB.mustRunAfter(taskA).

Copy link
Member Author

Choose a reason for hiding this comment

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

mustRunAfter/shouldRunAfter only sets an ordering requirement, but does not mandate that the earlier or later task run at all, they can run or not run independently.

On the other hand, b.dependsOn(a) means that if b is requested to run, a must run first and must be successful, whereas a.finalizedBy(b) means that if a will run, b must run afterwards (regardless of whether or not a is successful).

makeImage.configure {it ->
it.finalizedBy(buildAndRun)
}

// Handle copying output from the docker task to the user-controlled location
def syncOutput = project.tasks.register(taskName, Sync) { sync ->
sync.with {
dependsOn(makeImage)
// run the provided closure first
cfg.copyOut.execute(sync)

// then set the from location
from dockerCopyLocation

onlyIf { buildAndRun.get().state.failure != null }
doLast {
if (buildAndRun.get().state.failure != null) {
throw new GradleException('Docker task failed, see earlier task failures for details')
Copy link
Member

@devinrsmith devinrsmith Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I might add a comment this isn't an illegal state, we just want to preserve / propagate the failure, correct? I wonder if we should have a separate task that propagates this failure, instead of attaching it to the Sync task? (which may have specific semantics attached to sync-related failures.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct - not an illegal state (and the choice of GradleException is I thought intended to convey that), but just a failure. If this task did not throw an exception (or otherwise fail), it wouldn't be clear to other tasks that they should/shouldn't continue (e.g. "if it failed, dont continue to build downstream artifacts" vs "if it failed, make sure we dump the server's logs").

Copy link
Member Author

Choose a reason for hiding this comment

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

Note also that a new task with actions (i.e. "fail if that other thing failed") but no inputs or outputs can never be up to date. I'm not aware of Sync specific errors, but can add a comment here to call out what is going on, besides the wall of text written at the top of this section.

}
}
}
}
buildAndRun.configure {t ->
t.finalizedBy syncAfterFail
t.finalizedBy syncOutput
}

// Sync outputs to the desired location
return project.tasks.register(taskName, Sync) { sync ->
sync.with {
dependsOn buildAndRun

// run the provided closure first
cfg.copyOut.execute(sync)

// then set the from location
from dockerCopyLocation
}
}
return syncOutput
}
// With no outputs, we can use the standard individual containers, and gradle will have to re-run each time
// the task is invoked, can never be marked as up to date.
Expand Down
8 changes: 6 additions & 2 deletions buildSrc/src/main/groovy/io.deephaven.python-wheel.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ configurations {
}

project.evaluationDependsOn(Docker.registryProject('python'))
def wheelPath = project.layout.buildDirectory.dir('wheel')

def buildWheel = Docker.registerDockerTask(project, 'buildWheel') { config ->
config.copyIn { Sync sync ->
// apply the extension spec, copying into src
Expand All @@ -58,10 +60,12 @@ def buildWheel = Docker.registerDockerTask(project, 'buildWheel') { config ->
config.parentContainers = [ Docker.registryTask(project, 'python') ]
config.containerOutPath='/usr/src/app/dist'
config.copyOut { Sync sync ->
sync.into "build/wheel"
sync.into wheelPath
}
}

artifacts {
pythonWheel buildWheel
pythonWheel(wheelPath) {
builtBy buildWheel
}
}
2 changes: 1 addition & 1 deletion proto/raw-js-openapi/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ dependencies {

def webpackSourcesLocation = layout.buildDirectory.dir("${buildDir}/dhapi")

Docker.registerDockerTask(project, 'webpackSources') {
def webpackSources = Docker.registerDockerTask(project, 'webpackSources') {
copyIn {
from(configurations.js) {
// note: we are only copying the JS and not TS files.
Expand Down
Loading