From cd80f9c421f243d51a77871c633286c6fce7a224 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Mon, 26 Sep 2022 16:04:29 -0400 Subject: [PATCH 1/8] `SortedMap` shot in the dark --- .../scala/cromwell/engine/EngineFilesystems.scala | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala b/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala index 296d227d39f..c648261a7f4 100644 --- a/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala +++ b/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala @@ -8,19 +8,23 @@ import cromwell.core.filesystem.CromwellFileSystems import cromwell.core.path.{DefaultPathBuilderFactory, PathBuilder, PathBuilderFactory} import net.ceedubs.ficus.Ficus._ +import scala.collection.immutable.SortedMap import scala.concurrent.Future object EngineFilesystems { private val config: Config = ConfigFactory.load - private val defaultFileSystemFactory: Map[String, PathBuilderFactory] = + private val defaultFileSystemFactory: SortedMap[String, PathBuilderFactory] = Option(DefaultPathBuilderFactory.tuple) .filter(_ => config.as[Boolean]("engine.filesystems.local.enabled")) - .toMap + .to(collection.immutable.SortedMap) - private val pathBuilderFactories: Map[String, PathBuilderFactory] = { - CromwellFileSystems.instance.factoriesFromConfig(config.as[Config]("engine")) - .unsafe("Failed to instantiate engine filesystem") ++ defaultFileSystemFactory + private val pathBuilderFactories: SortedMap[String, PathBuilderFactory] = { + // Unordered maps are a classical source of randomness injection into a system + ( + CromwellFileSystems.instance.factoriesFromConfig(config.as[Config]("engine")) + .unsafe("Failed to instantiate engine filesystem") ++ defaultFileSystemFactory + ).to(collection.immutable.SortedMap) } def configuredPathBuilderFactories: List[PathBuilderFactory] = pathBuilderFactories.values.toList From 8f5d62ef7b277cbe8fef180eab85580027f37cae Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Mon, 26 Sep 2022 16:50:29 -0400 Subject: [PATCH 2/8] Comment --- core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala | 2 +- engine/src/main/scala/cromwell/engine/EngineFilesystems.scala | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala index 1f215ba2933..37e5867f658 100644 --- a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala +++ b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala @@ -18,7 +18,7 @@ object PathBuilderFactory { val sortedFactories = factories.sortWith({ case (_, DefaultPathBuilderFactory) => true case (DefaultPathBuilderFactory, _) => false - case (a, b) => factories.indexOf(a) < factories.indexOf(b) + case (a, b) => factories.indexOf(a) < factories.indexOf(b) // retains pre-existing order of `factories` }) sortedFactories.traverse(_.withOptions(workflowOptions)) } diff --git a/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala b/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala index c648261a7f4..1f349d7d4c4 100644 --- a/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala +++ b/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala @@ -27,7 +27,7 @@ object EngineFilesystems { ).to(collection.immutable.SortedMap) } - def configuredPathBuilderFactories: List[PathBuilderFactory] = pathBuilderFactories.values.toList + def configuredPathBuilderFactories: List[PathBuilderFactory] = pathBuilderFactories.values.toList.sorted(_.) def pathBuildersForWorkflow(workflowOptions: WorkflowOptions, factories: List[PathBuilderFactory])(implicit as: ActorSystem): Future[List[PathBuilder]] = { PathBuilderFactory.instantiatePathBuilders(factories, workflowOptions) From bcfbc4d93f81640a73c5f551eb4f7dc697c2607f Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Thu, 6 Oct 2022 17:27:08 -0400 Subject: [PATCH 3/8] Fix compiling --- engine/src/main/scala/cromwell/engine/EngineFilesystems.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala b/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala index 1f349d7d4c4..c648261a7f4 100644 --- a/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala +++ b/engine/src/main/scala/cromwell/engine/EngineFilesystems.scala @@ -27,7 +27,7 @@ object EngineFilesystems { ).to(collection.immutable.SortedMap) } - def configuredPathBuilderFactories: List[PathBuilderFactory] = pathBuilderFactories.values.toList.sorted(_.) + def configuredPathBuilderFactories: List[PathBuilderFactory] = pathBuilderFactories.values.toList def pathBuildersForWorkflow(workflowOptions: WorkflowOptions, factories: List[PathBuilderFactory])(implicit as: ActorSystem): Future[List[PathBuilder]] = { PathBuilderFactory.instantiatePathBuilders(factories, workflowOptions) From 330d10f751a29405a749813491bd6c5c3000ffaf Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 11 Oct 2022 18:11:53 -0400 Subject: [PATCH 4/8] Filesystem priority --- .../scala/cromwell/core/path/PathBuilderFactory.scala | 9 ++++++++- .../filesystems/blob/BlobPathBuilderFactory.scala | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala index 37e5867f658..ba415c30145 100644 --- a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala +++ b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala @@ -18,7 +18,7 @@ object PathBuilderFactory { val sortedFactories = factories.sortWith({ case (_, DefaultPathBuilderFactory) => true case (DefaultPathBuilderFactory, _) => false - case (a, b) => factories.indexOf(a) < factories.indexOf(b) // retains pre-existing order of `factories` + case (a, b) => a.priority < b.priority }) sortedFactories.traverse(_.withOptions(workflowOptions)) } @@ -29,4 +29,11 @@ object PathBuilderFactory { */ trait PathBuilderFactory { def withOptions(options: WorkflowOptions)(implicit as: ActorSystem, ec: ExecutionContext): Future[PathBuilder] + + /** + * Candidate filesystems are considered in a stable order, as some requests may match multiple filesystems. + * To customize this order, the priority of a filesystem may be adjusted. Lower number == higher priority. + * @return This filesystem's priority + */ + def priority: Int = 1000 } diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala index e6d269a07b0..48e19ceac25 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala @@ -33,4 +33,6 @@ final case class BlobPathBuilderFactory(globalConfig: Config, instanceConfig: Co new BlobPathBuilder(container, endpoint)(fsm) } } + + override def priority = 1 } From 261a4f50b9beab95aaed5e6995d531831f843e1f Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 11 Oct 2022 18:27:20 -0400 Subject: [PATCH 5/8] Leave room for higher priority --- .../cromwell/filesystems/blob/BlobPathBuilderFactory.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala index 48e19ceac25..6979a61829b 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala @@ -34,5 +34,5 @@ final case class BlobPathBuilderFactory(globalConfig: Config, instanceConfig: Co } } - override def priority = 1 + override def priority = 100 } From 217445d9f80cc38a94126b7c4657155b42f9d6ad Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Tue, 11 Oct 2022 18:31:50 -0400 Subject: [PATCH 6/8] Simplify sort --- .../cromwell/core/path/DefaultPathBuilderFactory.scala | 2 ++ .../main/scala/cromwell/core/path/PathBuilderFactory.scala | 6 +----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala b/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala index 91f05b10ef7..81d96efb61b 100644 --- a/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala +++ b/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala @@ -9,4 +9,6 @@ case object DefaultPathBuilderFactory extends PathBuilderFactory { override def withOptions(options: WorkflowOptions)(implicit actorSystem: ActorSystem, ec: ExecutionContext) = Future.successful(DefaultPathBuilder) val name = "local" val tuple = name -> this + + override def priority: Int = 10000 } diff --git a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala index ba415c30145..6562b76343b 100644 --- a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala +++ b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala @@ -15,11 +15,7 @@ object PathBuilderFactory { // The DefaultPathBuilderFactory always needs to be last. // The reason is path builders are tried in order, and the default one is very generous in terms of paths it "thinks" it supports // For instance, it will return a Path for a gcs url even though it doesn't really support it - val sortedFactories = factories.sortWith({ - case (_, DefaultPathBuilderFactory) => true - case (DefaultPathBuilderFactory, _) => false - case (a, b) => a.priority < b.priority - }) + val sortedFactories = factories.sortBy(_.priority) sortedFactories.traverse(_.withOptions(workflowOptions)) } } From 3608bfd390689c742df8b5bc1ec31fef707e1567 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Thu, 13 Oct 2022 17:59:41 -0400 Subject: [PATCH 7/8] Implement constants suggestion --- .../cromwell/core/path/DefaultPathBuilderFactory.scala | 3 ++- .../main/scala/cromwell/core/path/PathBuilderFactory.scala | 7 ++++++- .../cromwell/filesystems/blob/BlobPathBuilderFactory.scala | 3 ++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala b/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala index 81d96efb61b..20a2afdd5ce 100644 --- a/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala +++ b/core/src/main/scala/cromwell/core/path/DefaultPathBuilderFactory.scala @@ -2,6 +2,7 @@ package cromwell.core.path import akka.actor.ActorSystem import cromwell.core.WorkflowOptions +import cromwell.core.path.PathBuilderFactory.PriorityDefault import scala.concurrent.{ExecutionContext, Future} @@ -10,5 +11,5 @@ case object DefaultPathBuilderFactory extends PathBuilderFactory { val name = "local" val tuple = name -> this - override def priority: Int = 10000 + override def priority: Int = PriorityDefault } diff --git a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala index 6562b76343b..b339a84cb1e 100644 --- a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala +++ b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala @@ -5,6 +5,7 @@ import cromwell.core.{Dispatcher, WorkflowOptions} import cats.syntax.traverse._ import cats.instances.list._ import cats.instances.future._ +import cromwell.core.path.PathBuilderFactory.PriorityStandard import scala.concurrent.{ExecutionContext, Future} @@ -18,6 +19,10 @@ object PathBuilderFactory { val sortedFactories = factories.sortBy(_.priority) sortedFactories.traverse(_.withOptions(workflowOptions)) } + + val PriorityBlob = 100 // High priority to evaluate first, because blob files may inadvertently match other filesystems + val PriorityStandard = 1000 + val PriorityDefault = 10000 // "Default" is a fallback, evaluate last } /** @@ -31,5 +36,5 @@ trait PathBuilderFactory { * To customize this order, the priority of a filesystem may be adjusted. Lower number == higher priority. * @return This filesystem's priority */ - def priority: Int = 1000 + def priority: Int = PriorityStandard } diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala index 6979a61829b..1a9df21b0f2 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilderFactory.scala @@ -4,6 +4,7 @@ import akka.actor.ActorSystem import com.typesafe.config.Config import cromwell.core.WorkflowOptions import cromwell.core.path.PathBuilderFactory +import cromwell.core.path.PathBuilderFactory.PriorityBlob import net.ceedubs.ficus.Ficus._ import scala.concurrent.{ExecutionContext, Future} @@ -34,5 +35,5 @@ final case class BlobPathBuilderFactory(globalConfig: Config, instanceConfig: Co } } - override def priority = 100 + override def priority: Int = PriorityBlob } From 80500acaa3aa3fda340607ef61c4ec582a0fe4a4 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Thu, 13 Oct 2022 18:01:00 -0400 Subject: [PATCH 8/8] Remove redundant comment --- .../src/main/scala/cromwell/core/path/PathBuilderFactory.scala | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala index b339a84cb1e..79adaae4d6b 100644 --- a/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala +++ b/core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala @@ -13,9 +13,6 @@ object PathBuilderFactory { // Given a list of factories, instantiates the corresponding path builders def instantiatePathBuilders(factories: List[PathBuilderFactory], workflowOptions: WorkflowOptions)(implicit as: ActorSystem): Future[List[PathBuilder]] = { implicit val ec: ExecutionContext = as.dispatchers.lookup(Dispatcher.IoDispatcher) - // The DefaultPathBuilderFactory always needs to be last. - // The reason is path builders are tried in order, and the default one is very generous in terms of paths it "thinks" it supports - // For instance, it will return a Path for a gcs url even though it doesn't really support it val sortedFactories = factories.sortBy(_.priority) sortedFactories.traverse(_.withOptions(workflowOptions)) }