tysonnorris commented on a change in pull request #2957: splunk logstore
URL: 
https://github.com/apache/incubator-openwhisk/pull/2957#discussion_r167375563
 
 

 ##########
 File path: 
common/scala/src/main/scala/whisk/core/containerpool/logging/LogDriverLogStore.scala
 ##########
 @@ -21,34 +21,21 @@ import akka.actor.ActorSystem
 import whisk.core.entity.Identity
 import pureconfig.loadConfigOrThrow
 import whisk.common.TransactionId
-import whisk.core.containerpool.Container
+import whisk.core.containerpool.{Container, ContainerArgsConfig}
 import whisk.core.entity.{ActivationLogs, ExecutableWhiskAction, 
WhiskActivation}
 
 import scala.concurrent.Future
-
-trait LogDriverLogStoreConfig {
-  def message: String
-  def dockerLogDriver: String
-  def dockerLogDriverOpts: Set[String]
-}
-
-case class ConsumerlessLogDriverLogStoreConfig(message: String,
-                                               dockerLogDriver: String,
-                                               dockerLogDriverOpts: 
Set[String])
-    extends LogDriverLogStoreConfig
-
 /**
  * Docker log driver based LogStore impl. Uses docker log driver to emit 
container logs to an external store.
- * Fetching logs from that external store is not provided in this trait.
+ * Fetching logs from that external store is not provided in this trait. This 
SPI now requires the container
+ * args to be used as that is the place where the logs are shipped and 
fetching them here is a NOOP.
  */
 class LogDriverLogStore(actorSystem: ActorSystem,
-                        config: LogDriverLogStoreConfig =
-                          
loadConfigOrThrow[ConsumerlessLogDriverLogStoreConfig]("whisk.logstore.log-driver"))
+                        config: ContainerArgsConfig =
+                          
loadConfigOrThrow[ContainerArgsConfig]("whisk.container-factory.container-args"))
     extends LogStore {
-  val logDriverMessage = config.message
-  val logParameters = Map("--log-driver" -> Set(config.dockerLogDriver), 
"--log-opt" -> config.dockerLogDriverOpts)
 
-  override def containerParameters = logParameters.map(kv => (kv._1, 
kv._2.toSet)).toMap
+  override def containerParameters = config.extraArgs
 
 Review comment:
   I think that loading the ContainerArgsConfig here will cause doubling of the 
args when the container launches? 
   
   Historically, there were a handful of `docker run` args passed, in addition 
to special config handling for dns, network, and a couple others. Then LogStore 
SPI came, and added a couple other special handling. Then the 
ContainerArgsConfig provided "extraArgs" on top of those. 
   
   So each of the ContainerArgsConfigs docker run flags should only be used in 
one place, I think, and separately there is a question in my mind about whether 
these should ALL reside under extraArgs (vs continuing to handle as "some args 
are special", which mostly means that those special ones should never be 
included in extraArgs).
   
   So if we want people to add "--log-driver" via extra args, then 
LogDriverStore should just have an empty map for logParameters; and if we want 
people to use a special config for it, then it should not use extraArgs, but 
maybe either LogDriverConfig or ContainerArgsConfig.logDriverArgs. 
    
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to