hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eric Badger (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6623) Add support to turn off launching privileged containers in the container-executor
Date Mon, 21 Aug 2017 15:32:00 GMT

    [ https://issues.apache.org/jira/browse/YARN-6623?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16135306#comment-16135306
] 

Eric Badger commented on YARN-6623:
-----------------------------------

[~vvasudev], I started reviewing the .003 patch, but I see that you've updated a few times
since then. So I'll post my comments so far and then continue my review with the most recent
patch.

Ran test-container-executor on both my mac and on rhel6 and it passed.

1. Should {{feature.docker.enabled}} be moved to be with the rest of the docker configs? 

2. For the Docker configs, I think it'd be more clear if they were prefixed with {{docker.}}

3. Since we're going to remove the hard-coded string in YARN-6968, should we leave the findbugs
warning and take care of it there?

4. 
{noformat}
+  /**
+   * Add command commandWithArguments - this method is only meant for use by
+   * sub-classes.
+   *
{noformat}
This is no longer just adding a command. Looks like it adds arguments to the command and adds
the command as well if it doesn't exist. So the comment should be updated.

5. 
{noformat}
+    this.commandArguments = new TreeMap<>();
+    commandArguments.put(dockerCommandKey, new ArrayList<>());
+    commandArguments.get(dockerCommandKey).add(command);
{noformat}
I’m not sure I understand how this piece of code is supposed to work. In this method we
add a new list with the key “docker-command”. But then in {{addCommandArguments()}} we
add arguments via the {{key}}, which is always called in the subclasses with the name of the
command being run (e.g. inspect). So the map for each {{DockerCommand}} will usually have
2 keys, “docker-command” and the command that actually needs arguments “inspect”.
Is this how it’s intended to work or did I miss something here?

6.
{noformat}
+  protected final void addCommandArguments(String key, String value) {
+    if (commandArguments.containsKey(key)) {
+      commandArguments.get(key).add(value);
+      return;
+    }
{noformat}
No need to call {{contains()}} and then call {{get()}}. We can just call get and then add
the value if the return value isn't null. 

7.
{noformat}
+  @Override
+  public String toString() {
+    StringBuffer ret = new StringBuffer("");
+    ret.append(this.command);
{noformat}
Any reason not to create the string with the command in the constructor? Something like
{{StringBuffer ret = new StringBuffer(this.command);}}

> Add support to turn off launching privileged containers in the container-executor
> ---------------------------------------------------------------------------------
>
>                 Key: YARN-6623
>                 URL: https://issues.apache.org/jira/browse/YARN-6623
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>            Reporter: Varun Vasudev
>            Assignee: Varun Vasudev
>            Priority: Blocker
>         Attachments: YARN-6623.001.patch, YARN-6623.002.patch, YARN-6623.003.patch, YARN-6623.004.patch,
YARN-6623.005.patch
>
>
> Currently, launching privileged containers is controlled by the NM. We should add a flag
to the container-executor.cfg allowing admins to disable launching privileged containers at
the container-executor level.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message