[GitHub] [storm] agresch commented on a change in pull request #3287: [STORM-3271] enable supervisors to launch a worker inside a docker container

[GitBox <gi...@apache.org>]

agresch commented on a change in pull request #3287:
URL: https://github.com/apache/storm/pull/3287#discussion_r450930020



##########
File path: storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java
##########
@@ -290,10 +290,24 @@ public static String scriptFilePath(String dir) {
      * @param dir         the directory under which the script is to be written
      * @param command     the command the script is to execute
      * @param environment optional environment variables to set before running the script's command. May be  null.
+     * @param umask umask to be set. It can be null.

Review comment:
       this looks like it should be added to writeScript() below

##########
File path: docs/Docker-support.md
##########
@@ -0,0 +1,135 @@
+---
+title: Docker Support
+layout: documentation
+documentation: true
+---
+
+# Docker Support
+
+This page describes how storm supervisor launches the worker in a docker container. 
+
+Note: This is only tested on RHEL7.
+
+## Motivation
+
+This is mostly about security and portability. With workers running inside of docker containers, we isolate running user code from each other and from the hosted machine so that the whole system is less vulnerable to attack. 
+It also allows users to run their topologies on different os versions using different docker images.
+
+## Implementation
+
+The implementation is pretty easy to understand. Essentially, `DockerManager` composes a docker-run command and uses `worker-launcher` executable to execute the command 

Review comment:
       I think this first sentence (easy to understand) is irrelevant.  I would drop it.  Nice that it is easy though!

##########
File path: docs/Docker-support.md
##########
@@ -0,0 +1,135 @@
+---
+title: Docker Support
+layout: documentation
+documentation: true
+---
+
+# Docker Support
+
+This page describes how storm supervisor launches the worker in a docker container. 
+
+Note: This is only tested on RHEL7.

Review comment:
       "This has only been tested" maybe?

##########
File path: docs/Docker-support.md
##########
@@ -0,0 +1,135 @@
+---
+title: Docker Support
+layout: documentation
+documentation: true
+---
+
+# Docker Support
+
+This page describes how storm supervisor launches the worker in a docker container. 
+
+Note: This is only tested on RHEL7.
+
+## Motivation
+
+This is mostly about security and portability. With workers running inside of docker containers, we isolate running user code from each other and from the hosted machine so that the whole system is less vulnerable to attack. 

Review comment:
       "This feature is mostly"...?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org