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

TES Backend Prototype. Closes #1294 #1979

Merged
merged 1 commit into from
Feb 18, 2017

Conversation

adamstruck
Copy link
Contributor

Picking up from #1816.

Setup for Centaur testing. The current TES schema doesn't allow for changing the default user, so centaur fails on:

  • non_root_specified_user

Is there a way to configure Centaur to skip specific tests?

@kshakir
Copy link
Contributor

kshakir commented Feb 13, 2017

Try sbt 'testOnly -- -l localdockertest'

# because pulling the image during some of the tests would cause them to fail
# (specifically output_redirection which expects a specific value in stderr)
docker pull ubuntu:latest
centaur/test_cromwell.sh -j"${CROMWELL_JAR}" -c${TES_CENTAUR_CONF}
Copy link
Contributor

Choose a reason for hiding this comment

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

Until we figure out excluding these local tests by default, change:

centaur/test_cromwell.sh -j"${CROMWELL_JAR}" -c${TES_CENTAUR_CONF}

to:

centaur/test_cromwell.sh -j"${CROMWELL_JAR}" -c${TES_CENTAUR_CONF} -elocaldockertest

@adamstruck
Copy link
Contributor Author

adamstruck commented Feb 14, 2017

@kshakir adding -elocaldockertest addressed the issue when I ran the tests locally. In Travis CI, the non_root_default_user test is failing with:

Status: Downloaded newer image for mcovarr/notroot:v1
bin/bash: /cromwell-executions/woot/148f812b-028b-4264-82bd-ab2f089efe98/call-notroot/execution/script: Permission denied

This test passes when I run it locally. Any ideas?

@adamstruck adamstruck force-pushed the tes_backend_prototype branch 3 times, most recently from 1a87c5e to 0e02f81 Compare February 14, 2017 19:01
@mcovarr
Copy link
Contributor

mcovarr commented Feb 14, 2017

@adamstruck That test tries to run a Docker image which has a default user which is not root (the test was created based on this ticket). That error looks like the non-root user in the container doesn't have execute permission on the script that was created by Cromwell running as a different user.

def createTaskMessage(): TesTaskMessage = {
val task = TesTask(jobDescriptor, configurationDescriptor, jobLogger, tesJobPaths, runtimeAttributes, commandDirectory, backendEngineFunctions)

tesJobPaths.script.write(commandScriptContents)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, as Miguel pointed out, changing the permissions might help, specifically here?

tesJobPaths.script.write(commandScriptContents).chmod("rwxrwxrwx")

I haven't tried running this myself yet though. I'm assuming that this is the right spot to fix permissions, based on the error message in the logs producing the exit code 126:

/bin/bash: /cromwell-executions/woot/a2c488a2-e2a0-46f4-8714-47ac3a89bc2b/call-notroot/execution/script: Permission denied

I'm not 100% sure at the moment where/why the permissions on the script are not-as-expected though. The local backend gets around permissions errors by telling docker to run /bin/bash /path/to/script, that seems to work even if the file is rw-rw-r--.

If you really are running into problems and don't want to support this test and the "running as non-root" functionality for whatever reason, let us know, and we can figure out a way to exclude it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an issue on the TES side of things. I am resolving this today.

@adamstruck
Copy link
Contributor Author

@kshakir all TES Centaur tests are now passing.

cd ..

TES_CONF="$(pwd)/src/bin/travis/resources/tes.conf"
git clone https://github.com/ohsu-comp-bio/funnel.git
Copy link
Contributor

Choose a reason for hiding this comment

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

@adamstruck Can you change this to https://github.com/broadinstitute/funnel.git? I've forked the repo.

} get

tryWithResource(() => path.newInputStream) { inputStream =>
org.apache.commons.codec.digest.DigestUtils.md5Hex(inputStream)
Copy link
Contributor

Choose a reason for hiding this comment

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

ToL: Note for the future, this is fine for passing centaur tests, but takes too long on real BAM files. Hopefully whenever #1843 is implemented it will be reusable by this backend also.

@kshakir
Copy link
Contributor

kshakir commented Feb 17, 2017

👍 once centaur isn't downloading from an external repo

Approved with PullApprove

@@ -75,7 +80,8 @@ A [Workflow Management System](https://en.wikipedia.org/wiki/Workflow_management
* [Local Filesystem Options](#local-filesystem-options)
* [Imports](#imports)
* [Sub Workflows](#sub-workflows)
* [Meta blocks](#meta-blocks)
* [Execution](#execution)
* [Metadata](#metadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

oops thanks for fixing this!

private val MemoryDefaultValue = "2 GB"

val DiskSizeKey = "disk"
private val DiskSizeDefaultValue = "2 GB"
Copy link
Contributor

Choose a reason for hiding this comment

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

These default values are not consistent with the README

README.md Outdated
@@ -1392,6 +1399,65 @@ In order to monitor metrics (CPU, Memory, Disk usage...) about the VM during Cal

The output of this script will be written to a `monitoring.log` file that will be available in the call gcs bucket when the call completes. This feature is meant to run a script in the background during long-running processes. It's possible that if the task is very short that the log file does not flush before de-localization happens and you will end up with a zero byte file.

## GA4GH TES Backend
The TES backend submit jobs to a server that complies with the protocol described by the [GA4GH schema](https://github.com/ga4gh/task-execution-schemas).
Copy link
Contributor

Choose a reason for hiding this comment

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

submit jobs => submits jobs

@mcovarr
Copy link
Contributor

mcovarr commented Feb 17, 2017

👍

Approved with PullApprove

@geoffjentry
Copy link
Contributor

Pretty cool, thanks @adamstruck

@adamstruck
Copy link
Contributor Author

Thanks for the comments. I pushed the README changes and Funnel is now being downloaded from the broad's fork.

It looks like both the Local and Tes Centaur build are failing now though?

added -elocaldockertest to centaur command

print TES logs after run

fixed unmarshalling bug

inputs in read-only volume; matched TES case classes to schema

address final PR comments
@kshakir
Copy link
Contributor

kshakir commented Feb 18, 2017

The Local build did fail, due to an unrelated hiccup. A restart of the build cleared the error.

Meanwhile, JES Centaur failed due to #1717, but TES passed just fine.

Everything looking great. Thanks again for all of your contributions!

@kshakir kshakir merged commit 1524f3d into broadinstitute:develop Feb 18, 2017
@adamstruck adamstruck deleted the tes_backend_prototype branch May 12, 2017 21:45
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