Merge branch 'develop'

CHANGELOG.md

@@ -1,5 +1,92 @@
 # Cromwell Change Log
 
+## 41 Release Notes
+
+### Workflow Options
+
+* It is now possible to supply custom `google-labels` in [workflow options](https://cromwell.readthedocs.io/en/stable/wf_options/Google/).
+
+### AWS backend
+
+It is now possible to use WDL disk attributes with the following formats on AWS.
+```
+disks: "local-disk 20 SSD"
+```
+```
+disks: "/some/mnt 20 SSD"
+```
+Because Cromwell's AWS backend auto-sizes disks, the size specification is simply discarded.
+
+### Time Formatting
+
+In previous versions of Cromwell, times were converted to strings using
+[the default Java formatter](https://docs.oracle.com/javase/8/docs/api/java/time/OffsetDateTime.html#toString--) which
+generates a variety of ISO-8601 formats. String conversions also retained whatever server time zone generated that
+specific time instance.
+
+Going forward, times stored in Cromwell metadata, and later returned via the HTTP endpoint, are now converted to UTC
+then formatted with exactly three digits of milliseconds.
+
+For example:
+- `2017-01-19T12:34:56-04:00` will now be formatted as
+- `2017-01-19T16:34:56.000Z`
+
+This change only affects newly formatted dates. Older dates already formatted and stored by previous versions of
+Cromwell will not be updated however they will still return a
+[valid ISO-8601 format](https://en.wikipedia.org/wiki/ISO_8601). The older format may be in various non-UTC time zones,
+and may or may not include microseconds or even nanoseconds, for example `2017-01-19T12:34:56.123456789-04:00`.
+
+### Config Changes
+
+#### Heartbeat failure shutdown
+
+When a Cromwell instance is unable to write heartbeats for some period of time it will automatically shut down. For more
+information see the docs on [configuring Workflow Hearbeats](https://cromwell.readthedocs.io/en/stable/Configuring/).
+
+NOTE: In the remote chance that the `system.workflow-heartbeats.ttl` has been configured to be less than `5 minutes`
+then the new configuration value `system.workflow-heartbeats.write-failure-shutdown-duration` must also be explicitly
+set less than the `ttl`.
+
+#### nVidia Driver Attribute Change
+
+The runtime attribute `nvidia-driver-version` was previously allowed only as a default runtime attribute in configuration.
+Because WDL does not allow attribute names to contain `-` characters, this has been changed to `nvidiaDriverVersion`.
+This field is now accepted within WDL files as well as within the configuration file.
+
+#### Logging long running jobs
+
+All backends can now emit slow job warnings after a configurable time running. 
+NB This example shows how to configure this setting for the PAPIv2 backend:
+```conf
+# Emit a warning if jobs last longer than this amount of time. This might indicate that something got stuck.
+backend {
+  providers {
+    PAPIv2 {
+      config { 
+        slow-job-warning-time: 24 hours
+      }
+    }
+  }
+}
+```
+
+### Runtime Attributes
+
+#### GPU Attributes
+
+* The `gpuType` attribute is no longer validated against a whitelist at workflow submission time. Instead, validation now happens at runtime. This allows any valid accelerator to be used.
+* The `nvidiaDriverVersion` attribute is now available in WDL `runtime` sections. The default continues to be `390.46` which applies if and only if GPUs are being used.
+* A default `gpuType` ("nvidia-tesla-k80") will now be applied if `gpuCount` is specified but `gpuType` is not.
+* Similarly, a default `gpuCount` (1) will be applied if `gpuType` is specified but `cpuCount` is not. 
+
+### Bug fixes
+
+#### Better validation of workflow heartbeats
+
+An error will be thrown on startup when the `system.workflow-heartbeats.heartbeat-interval` is not less than the
+`system.workflow-heartbeats.ttl`.
+
+
 ## 40 Release Notes
 
 ### Config Changes
@@ -76,7 +163,13 @@ services {
   }
 }
 ``` 
+### Workflow options changes
 
+A new workflow option is added. If the `final_workflow_outputs_dir` is set 
+`use_relative_output_paths` can be used. When set to `true` this will copy 
+all the outputs relative to their execution directory. 
+my_final_workflow_outputs_dir/~~MyWorkflow/af76876d8-6e8768fa/call-MyTask/execution/~~output_of_interest.
+More information can be found in [the workflow options documentation](https://cromwell.readthedocs.io/en/stable/wf_options/Overview/#output-copying).
 
 ### Bug fixes
 

CITATION.md

@@ -0,0 +1,26 @@
+# Citing `Cromwell`
+
+If you use `Cromwell` in your work we would prefer it if you would use the following reference in your work.
+
+## BibTeX
+
+```bibtex
+@misc{https://doi.org/10.7490/f1000research.1114634.1,
+  doi = {10.7490/f1000research.1114634.1},
+  url = {https://f1000research.com/slides/6-1381},
+  author = {Voss,  Kate and Auwera,  Geraldine Van Der and Gentry,  Jeff},
+  title = {Full-stack genomics pipelining with GATK4 + WDL + Cromwell [version 1; not peer reviewed]},
+  journal = {ISCB Comm J},
+  publisher = {F1000Research},
+  type = {slides}
+  volume = {6},
+  number = {1381},
+  year = {2017}
+}
+```
+
+## Textual
+
+Voss K, Van der Auwera G and Gentry J. Full-stack genomics pipelining with GATK4 + WDL + Cromwell
+[version 1; not peer reviewed]. _F1000Research_ 2017, **6**(ISCB Comm J):1381 (slides)
+(https://doi.org/10.7490/f1000research.1114634.1) 

CONTRIBUTING.md

@@ -0,0 +1,12 @@
+### Contributing to Cromwell
+
+Thank you for contributing to Cromwell. The project is sponsored by Broad Institute and has participants all over the world, who have collectively added tremendous value over the years. This page describes how we handle external contributions to maximize the impact of your work and reduce delays in getting your PRs accepted.
+
+#### Run your idea by us first
+If you're thinking of writing a non-trivial amount of code to solve a problem, we encourage you to reach out via an [issue](https://github.com/broadinstitute/cromwell/issues/new) to get feedback. It is likely we will have suggestions about how to proceed. It's also possible, though hopefully rare, that there is a hidden impediment that would prevent your solution from working. If we spot it at the idea stage, we can give you feedback much earlier than if we have to reject your pull request!
+
+#### Maintenance considerations
+The Cromwell team at Broad maintains and enhances the application to serve the needs of both Broad Institute and external users. Sometimes, we may identify a feature idea or pull request that works and is a good idea, but we may be unable to commit to maintaining it indefinitely. This may be because it does not align with the strategic direction of the project, or simply due to time constraints on the maintainers. Once again, it always helps to solicit early feedback.
+
+#### Reviewing pull requests
+Because pull requests require a substantial amount of time to review carefully, we prioritize and schedule them into our sprints alongside all of our other work. At present, the team operates on three-week sprints so if you happen to submit a PR early on in the sprint it may be a while before a team member has a chance to look at it. We realize this may be frustrating and strive to provide timely updates about PR status.

CromIAM/src/main/scala/cromiam/sam/SamClient.scala

@@ -136,15 +136,17 @@ class SamClient(scheme: String,
     val createCollection = registerCreation(user, collection, cromIamRequest)
 
     createCollection flatMap {
-      case r if r.status == StatusCodes.Conflict => requestAuth(CollectionAuthorizationRequest(user, collection, "add"), cromIamRequest)
       case r if r.status == StatusCodes.NoContent => Monad[FailureResponseOrT].unit
       case r => FailureResponseOrT[IO, HttpResponse, Unit](IO.raiseError(SamRegisterCollectionException(r.status)))
+    } recoverWith {
+      case r if r.status == StatusCodes.Conflict => requestAuth(CollectionAuthorizationRequest(user, collection, "add"), cromIamRequest)
+      case r => FailureResponseOrT[IO, HttpResponse, Unit](IO.raiseError(SamRegisterCollectionException(r.status)))
     }
   }
 
-  private def registerCreation(user: User,
-                               collection: Collection,
-                               cromIamRequest: HttpRequest): FailureResponseOrT[HttpResponse] = {
+  protected def registerCreation(user: User,
+                                 collection: Collection,
+                                 cromIamRequest: HttpRequest): FailureResponseOrT[HttpResponse] = {
     val request = HttpRequest(method = HttpMethods.POST, uri = samRegisterUri(collection), headers = List[HttpHeader](user.authorization))
 
     instrumentRequest(

CromIAM/src/test/scala/cromiam/sam/SamClientSpec.scala

@@ -4,37 +4,40 @@ import akka.actor.ActorSystem
 import akka.http.scaladsl.model.headers.{Authorization, OAuth2BearerToken}
 import akka.http.scaladsl.model.{HttpEntity, HttpRequest, HttpResponse, StatusCodes}
 import akka.stream.ActorMaterializer
+import cats.Monad
 import cromiam.auth.{Collection, User}
 import cromiam.sam.SamClient.{CollectionAuthorizationRequest, SamDenialException, SamRegisterCollectionException}
-import cromiam.webservice.MockSamClient
+import cromiam.webservice.MockSamClient._
+import cromiam.webservice._
+import cromwell.api.CromwellClient.UnsuccessfulRequestException
 import cromwell.api.model._
 import org.broadinstitute.dsde.workbench.model.WorkbenchUserId
-import org.scalatest.EitherValues._
 import org.scalatest.{AsyncFlatSpec, BeforeAndAfterAll, Matchers}
+import org.specs2.mock.Mockito
 
 import scala.concurrent.ExecutionContextExecutor
 
-class SamClientSpec extends AsyncFlatSpec with Matchers with BeforeAndAfterAll  {
+class SamClientSpec extends AsyncFlatSpec with Matchers with BeforeAndAfterAll with Mockito {
 
   implicit val actorSystem: ActorSystem = ActorSystem("SamClientSpec")
   implicit val ece: ExecutionContextExecutor = actorSystem.dispatcher
   implicit val materializer: ActorMaterializer = ActorMaterializer()
 
-  val samClient = new MockSamClient()
-  val samClientNoWhitelist = new MockSamClient(checkSubmitWhitelist = false)
-  val expectedErrorMessage = "expected error"
-  val expectedErrorResponse = HttpResponse(StatusCodes.InternalServerError, entity = HttpEntity(expectedErrorMessage))
-  val samClientWithError = new MockSamClient(samResponseOption = Option(expectedErrorResponse))
+  private val expectedErrorResponse =
+    HttpResponse(StatusCodes.InternalServerError, entity = HttpEntity("expected error"))
 
   val authorization = Authorization(OAuth2BearerToken("my-token"))
-  val authorizedUserWithCollection: User = User(WorkbenchUserId(samClient.authorizedUserCollectionStr), authorization)
-  val unauthorizedUserWithNoCollection: User = User(WorkbenchUserId(samClient.unauthorizedUserCollectionStr), authorization)
-  val notWhitelistedUser: User = User(WorkbenchUserId(samClient.notWhitelistedUser), authorization)
-
-  val authorizedCollection: Collection = Collection(samClient.authorizedUserCollectionStr)
-  val unauthorizedCollection: Collection = Collection(samClient.unauthorizedUserCollectionStr)
-  val authorizedCollectionRequest: CollectionAuthorizationRequest = CollectionAuthorizationRequest(authorizedUserWithCollection, authorizedCollection, "add")
-  val unauthorizedCollectionRequest: CollectionAuthorizationRequest = CollectionAuthorizationRequest(unauthorizedUserWithNoCollection, unauthorizedCollection, "add")
+  val authorizedUserWithCollection = User(WorkbenchUserId(MockSamClient.AuthorizedUserCollectionStr), authorization)
+  val unauthorizedUserWithNoCollection =
+    User(WorkbenchUserId(MockSamClient.UnauthorizedUserCollectionStr), authorization)
+  val notWhitelistedUser = User(WorkbenchUserId(MockSamClient.NotWhitelistedUser), authorization)
+
+  val authorizedCollection = Collection(MockSamClient.AuthorizedUserCollectionStr)
+  val unauthorizedCollection = Collection(MockSamClient.UnauthorizedUserCollectionStr)
+  val authorizedCollectionRequest =
+    CollectionAuthorizationRequest(authorizedUserWithCollection, authorizedCollection, "add")
+  val unauthorizedCollectionRequest =
+    CollectionAuthorizationRequest(unauthorizedUserWithNoCollection, unauthorizedCollection, "add")
 
   val emptyHttpRequest: HttpRequest = HttpRequest()
 
@@ -46,28 +49,37 @@ class SamClientSpec extends AsyncFlatSpec with Matchers with BeforeAndAfterAll
 
   behavior of "SamClient"
   it should "return true if user is whitelisted" in {
+    val samClient = new MockSamClient()
     samClient.isSubmitWhitelisted(authorizedUserWithCollection, emptyHttpRequest).map(v => assert(v))
       .asIo.unsafeToFuture()
   }
 
   it should "return false if user is not whitelisted" in {
+    val samClient = new MockSamClient()
     samClient.isSubmitWhitelisted(notWhitelistedUser, emptyHttpRequest).map(v => assert(!v))
       .asIo.unsafeToFuture()
   }
 
   it should "return sam errors while checking is whitelisted" in {
-    samClientWithError.isSubmitWhitelisted(notWhitelistedUser, emptyHttpRequest).value.unsafeToFuture() map {
-      _.left.value should be(expectedErrorResponse)
+    val samClient = new MockSamClient() {
+      override def isSubmitWhitelistedSam(user: User, cromiamRequest: HttpRequest): FailureResponseOrT[Boolean] = {
+        MockSamClient.returnResponse(expectedErrorResponse)
+      }
+    }
+    samClient.isSubmitWhitelisted(notWhitelistedUser, emptyHttpRequest).value.unsafeToFuture() map {
+      _ should be(Left(expectedErrorResponse))
     }
   }
 
   it should "eventually return the collection(s) of user" in {
+    val samClient = new MockSamClient()
     samClient.collectionsForUser(authorizedUserWithCollection, emptyHttpRequest).map(collectionList =>
-      assert(collectionList == samClient.userCollectionList)
+      assert(collectionList == MockSamClient.UserCollectionList)
     ).asIo.unsafeToFuture()
   }
 
   it should "fail if user doesn't have any collections" in {
+    val samClient = new MockSamClient()
     recoverToExceptionIf[Exception] {
       samClient.collectionsForUser(unauthorizedUserWithNoCollection, emptyHttpRequest)
         .asIo.unsafeToFuture()
@@ -76,19 +88,14 @@ class SamClientSpec extends AsyncFlatSpec with Matchers with BeforeAndAfterAll
     )
   }
 
-  it should "return sam errors while checking collections" in {
-    samClientWithError.collectionsForUser(unauthorizedUserWithNoCollection, emptyHttpRequest)
-      .value.unsafeToFuture() map {
-      _.left.value should be(expectedErrorResponse)
-    }
-  }
-
   it should "return true if user is authorized to perform action on collection" in {
+    val samClient = new MockSamClient()
     samClient.requestAuth(authorizedCollectionRequest, emptyHttpRequest).map(_ => succeed)
       .asIo.unsafeToFuture()
   }
 
   it should "throw SamDenialException if user is not authorized to perform action on collection" in {
+    val samClient = new MockSamClient()
     recoverToExceptionIf[SamDenialException] {
       samClient.requestAuth(unauthorizedCollectionRequest, emptyHttpRequest)
         .asIo.unsafeToFuture()
@@ -97,18 +104,14 @@ class SamClientSpec extends AsyncFlatSpec with Matchers with BeforeAndAfterAll
     }
   }
 
-  it should "return sam errors while checking if user is authorized" in {
-    samClientWithError.requestAuth(authorizedCollectionRequest, emptyHttpRequest).value.unsafeToFuture() map {
-      _.left.value should be(expectedErrorResponse)
-    }
-  }
-
   it should "register collection to Sam if user has authorization to create/add to collection" in {
+    val samClient = new MockSamClient()
     samClient.requestSubmission(authorizedUserWithCollection, authorizedCollection, emptyHttpRequest).map(_ => succeed)
       .asIo.unsafeToFuture()
   }
 
   it should "throw SamRegisterCollectionException if user doesn't have authorization to create/add to collection" in {
+    val samClient = new MockSamClient()
     recoverToExceptionIf[SamRegisterCollectionException] {
       samClient.requestSubmission(unauthorizedUserWithNoCollection, unauthorizedCollection, emptyHttpRequest).map(_ => succeed)
         .asIo.unsafeToFuture()
@@ -117,10 +120,80 @@ class SamClientSpec extends AsyncFlatSpec with Matchers with BeforeAndAfterAll
     }
   }
 
-  it should "return sam errors while checking user authorization" in {
-    samClientWithError.requestSubmission(unauthorizedUserWithNoCollection, unauthorizedCollection, emptyHttpRequest)
-      .value.unsafeToFuture() map {
-      _.left.value should be(expectedErrorResponse)
+  it should "add the user when create returns a conflict" in {
+    val samClient = new BaseMockSamClient() {
+      override protected def registerCreation(user: User,
+                                              collection: Collection,
+                                              cromiamRequest: HttpRequest): FailureResponseOrT[HttpResponse] = {
+        val conflictResponse = HttpResponse(StatusCodes.Conflict, entity = HttpEntity("expected conflict"))
+        returnResponse(conflictResponse)
+      }
+
+      override def requestAuth(authorizationRequest: CollectionAuthorizationRequest,
+                               cromiamRequest: HttpRequest): FailureResponseOrT[Unit] = {
+        Monad[FailureResponseOrT].unit
+      }
+    }
+    samClient
+      .requestSubmission(unauthorizedUserWithNoCollection, unauthorizedCollection, emptyHttpRequest)
+      .map(_ => succeed)
+      .asIo
+      .unsafeToFuture()
+  }
+
+  it should "fail to add the user when create returns a conflict but then adding returns an error" in {
+    val samClient = new BaseMockSamClient() {
+      override protected def registerCreation(user: User,
+                                              collection: Collection,
+                                              cromiamRequest: HttpRequest): FailureResponseOrT[HttpResponse] = {
+        val conflictResponse = HttpResponse(StatusCodes.Conflict, entity = HttpEntity("expected conflict"))
+        returnResponse(conflictResponse)
+      }
+
+      override def requestAuth(authorizationRequest: CollectionAuthorizationRequest,
+                               cromiamRequest: HttpRequest): FailureResponseOrT[Unit] = {
+        returnResponse(expectedErrorResponse)
+      }
+    }
+    recoverToExceptionIf[UnsuccessfulRequestException] {
+      samClient.requestSubmission(unauthorizedUserWithNoCollection, unauthorizedCollection, emptyHttpRequest)
+        .asIo.unsafeToFuture()
+    } map { exception =>
+      assert(exception.getMessage == "expected error")
+    }
+  }
+
+  it should "fail to add the user when create returns an unexpected successful response" in {
+    val samClient = new BaseMockSamClient() {
+      override protected def registerCreation(user: User,
+                                              collection: Collection,
+                                              cromiamRequest: HttpRequest): FailureResponseOrT[HttpResponse] = {
+        val unexpectedOkResponse = HttpResponse(StatusCodes.OK, entity = HttpEntity("elided ok message"))
+        returnResponse(unexpectedOkResponse)
+      }
+    }
+    recoverToExceptionIf[SamRegisterCollectionException] {
+      samClient.requestSubmission(unauthorizedUserWithNoCollection, unauthorizedCollection, emptyHttpRequest)
+        .asIo.unsafeToFuture()
+    } map { exception =>
+      exception.getMessage should be("Can't register collection with Sam. Status code: 200 OK")
+    }
+  }
+
+  it should "fail to add the user when create returns an unexpected failure response" in {
+    val samClient = new BaseMockSamClient() {
+      override protected def registerCreation(user: User,
+                                              collection: Collection,
+                                              cromiamRequest: HttpRequest): FailureResponseOrT[HttpResponse] = {
+        val unexpectedFailureResponse = HttpResponse(StatusCodes.ImATeapot, entity = HttpEntity("elided error message"))
+        returnResponse(unexpectedFailureResponse)
+      }
+    }
+    recoverToExceptionIf[SamRegisterCollectionException] {
+      samClient.requestSubmission(unauthorizedUserWithNoCollection, unauthorizedCollection, emptyHttpRequest)
+        .asIo.unsafeToFuture()
+    } map { exception =>
+      exception.getMessage should be("Can't register collection with Sam. Status code: 418 I'm a teapot")
     }
   }
 }