Varun Vasudev commented on YARN-3852:

Thanks for the patch [~ashahab]. The patch isn't working for me. There are two 
issues -
# No default value for "docker.binary". I think we should assume this to be 
"docker" and allow it to be overriden.
# The docker launch fails due to 
if (change_effective_user(user_uid, user_gid) != 0)
in launch_docker_container_as_user. For docker run to work, the effective user 
needs to be root(something like change_effective_user(0, user_gid) is probably 
the right way).

Some other issues -
-static const char* DEFAULT_BANNED_USERS[] = {"yarn", "mapred", "hdfs", "bin", 
+static const char* DEFAULT_BANNED_USERS[] = {"mapred", "hdfs", "bin", 0};
Why are you removing the yarn user from the banned users? I'm guessing this is 
due to a branch-2/trunk issue. The yarn user is banned in trunk but not in 
# A couple of formatting fixes
+   fprintf(LOGFILE, "done opening pid\n");
+        fflush(LOGFILE);
+    fprintf(LOGFILE, "done writing pid to tmp\n");
+         fflush(LOGFILE);
# Can we change the error message in the message below to a more descriptive 
+     fprintf(ERRORFILE, "Error reading\n");
+     fflush(ERRORFILE);
# In parse_docker_command_file
+  int read;
should we use ssize_t instead or int?
# In parse_docker_command_file, we have some exit(1) calls - can we change this 
to use the error codes in container-executor.h?
# In run_docker
+      free(docker_binary);
+      free(args);
+      free(docker_command_with_binary);
+      free(docker_command);
+      exit_code = DOCKER_RUN_FAILED;
+  }
+  exit_code = 0;
+  return exit_code;
The exit code from the function will always be 0
# Formatting
+int create_script_paths(const char *work_dir,
+                      const char *script_name, const char *cred_file,
+                 char** script_file_dest, char** cred_file_dest,
+                 int* container_file_source, int* cred_file_source ) {
# In create_script_paths, we use a bunch of goto's but the goto target doesn't 
have any special logic or handling. Can we avoid using the goto?
+    //kill me now.
No need for the commentary :)
# In main.c
+    char * resources = argv[optind++];// key,value pair describing resources
+    char * resources_key = malloc(strlen(resources));
+    char * resources_value = malloc(strlen(resources));

Can we move the declarations of resources, resources_key and resources_value 
out of the case block(since the same variables are used in two case blocks)?

> Add docker container support to container-executor 
> ---------------------------------------------------
>                 Key: YARN-3852
>                 URL: https://issues.apache.org/jira/browse/YARN-3852
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: yarn
>            Reporter: Sidharta Seethana
>            Assignee: Abin Shahab
>         Attachments: YARN-3852.patch
> For security reasons, we need to ensure that access to the docker daemon and 
> the ability to run docker containers is restricted to privileged users ( i.e 
> users running applications should not have direct access to docker). In order 
> to ensure the node manager can run docker commands, we need to add docker 
> support to the container-executor binary.

This message was sent by Atlassian JIRA

Reply via email to