hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Varun Vasudev (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (YARN-5430) Get container's ip and host from NM
Date Wed, 03 Aug 2016 15:47:20 GMT

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

Varun Vasudev edited comment on YARN-5430 at 8/3/16 3:46 PM:
-------------------------------------------------------------

Thanks for the patch [~jianhe]!

1)
{code}
+  @Private
+  @Unstable
+  public abstract void setIp(String ip);
+
{code}
We can’t return just one ip. In the non-docker scenario, multiple ips for a host are common
and Docker itself has support for multiple ips for a container. We don’t currently support
that but it’ll probably be required for us to support soon. Similarly we should change getIpAndHost
and split them up into getIps and getHost calls.

2)
{code}
+  optional string ip = 7;
+  optional string host = 8;
{code}

Instead of named variables - can we make this a bit more generic and add support for a simple
list of string->list? Then the next time we need to send a new variable, we don’t need
to mess around with the protobuf pieces. Something like
{code}
message StringListMapProto {
  required string key = 1;
  optional repeated string value = 2;
}
{code}
and in ContainerStatusProto

{code}
optional repeated StringListMapProto attributes_map = 7;
{code}

3)
{code}
+  public String[] getIpAndHost(Container container) {
+    return null;
+  }
{code}

The default implementation shouldn’t return null. We should return the host name and ip.

4)
{code}
+  public void setIpAndHost(String[] ipAndHost) {
+    this.ip = ipAndHost[0];
+    this.host = ipAndHost[1];
+  }
{code}

This may become a problem if the size of the array is more than 2(multiple ips)

5)
{code}
+  @Override
+  public String[] getIpAndHost(Container container) {
+    String[] ipAndHost = new String[2];
+    try {
+      InetAddress address = InetAddress.getLocalHost();
+      ipAndHost[0] = address.getHostAddress();
+      ipAndHost[1] = address.getHostName();
+    } catch (UnknownHostException e) {
+      LOG.error("Unable to get Local hostname and ip for " + container
+          .getContainerId());
+    }
+    return ipAndHost;
+  }
 }
{code}
Instead of this, we should just use NetUtils.getLocalHostname() and NetUtils.getIPs. we’ll
have to modify getIps() because today it expects a subnet and we want all ips irrespective
of subnet.

6)
{code}
+      LOG.info("Docker inspect output for " + containerId + ": " + output);
{code}
Change log level to debug?

7)
{code}
+    // Be sure to not use space in the argument, otherwise the
+    // extract_values_delim method in container-executor binary
+    // cannot parse the arguments correctly.
+    super.addCommandArguments("--format='{{range.NetworkSettings.Networks}}"
+        + "{{.IPAddress}}{{end}},{{.Config.Hostname}}'");
+    super.addCommandArguments(containerName);
+    return this;
{code}

Ugh. This is an ugly one. I thought there needed to be a space between range and NetworkSettings
because the code passed is meant to be a go template. Use this instead - {code}--format='{{range(.NetworkSettings.Networks)}}{code}

8)
{code}
+  if (docker_binary == NULL) {
+    docker_binary = "docker";
+  }
{code}

Can we make this a common function - the same code is in launch_docker_container_as_user?


was (Author: vvasudev):
Thanks for the patch [~jianhe]!

1)
{code}
+  @Private
+  @Unstable
+  public abstract void setIp(String ip);
+
{code}
We can’t return just one ip. In the non-docker scenario, multiple ips for a host are common
and Docker itself has support for multiple ips for a container. We don’t currently support
that but it’ll probably be required for us to support soon. Similarly we should change getIpAndHost
and split them up into getIps and getHost calls.

2)
{code}
+  optional string ip = 7;
+  optional string host = 8;
{code}

Instead of named variables - can we make this a bit more generic and add support for a simple
list of string->list? Then the next time we need to send a new variable, we don’t need
to mess around with the protobuf pieces. Something like
{code}
message StringListMapProto {
  required string key = 1;
  optional repeated string value = 2;
}

and in ContainerStatusProto

{code}
optional repeated StringListMapProto attributes_map = 7;
{code}

3)
{code}

+  public String[] getIpAndHost(Container container) {
+    return null;
+  }
{code}

The default implementation shouldn’t return null. We should return the host name and ip.

4)
{code}
+  public void setIpAndHost(String[] ipAndHost) {
+    this.ip = ipAndHost[0];
+    this.host = ipAndHost[1];
+  }
{code}

This may become a problem if the size of the array is more than 2(multiple ips)

5)
{code}
+  @Override
+  public String[] getIpAndHost(Container container) {
+    String[] ipAndHost = new String[2];
+    try {
+      InetAddress address = InetAddress.getLocalHost();
+      ipAndHost[0] = address.getHostAddress();
+      ipAndHost[1] = address.getHostName();
+    } catch (UnknownHostException e) {
+      LOG.error("Unable to get Local hostname and ip for " + container
+          .getContainerId());
+    }
+    return ipAndHost;
+  }
 }
{code}
Instead of this, we should just use NetUtils.getLocalHostname() and NetUtils.getIPs. we’ll
have to modify getIps() because today it expects a subnet and we want all ips irrespective
of subnet.

6)
{code}
+      LOG.info("Docker inspect output for " + containerId + ": " + output);
{code}
Change log level to debug?

7)
{code}
+    // Be sure to not use space in the argument, otherwise the
+    // extract_values_delim method in container-executor binary
+    // cannot parse the arguments correctly.
+    super.addCommandArguments("--format='{{range.NetworkSettings.Networks}}"
+        + "{{.IPAddress}}{{end}},{{.Config.Hostname}}'");
+    super.addCommandArguments(containerName);
+    return this;
{code}

Ugh. This is an ugly one. I thought there needed to be a space between range and NetworkSettings
because the code passed is meant to be a go template. Use this instead - --format='{{range(.NetworkSettings.Networks)}}

8)
{code}
+  if (docker_binary == NULL) {
+    docker_binary = "docker";
+  }
{code}

Can we make this a common function - the same code is in launch_docker_container_as_user?

> Get container's ip and host from NM
> -----------------------------------
>
>                 Key: YARN-5430
>                 URL: https://issues.apache.org/jira/browse/YARN-5430
>             Project: Hadoop YARN
>          Issue Type: Bug
>            Reporter: Jian He
>            Assignee: Jian He
>         Attachments: YARN-5430.1.patch, YARN-5430.2.patch, YARN-5430.3.patch
>
>
> In YARN-4757, we introduced a DNS mechanism for containers. That's based on the assumption
 that we can get the container's ip and host information and store it in the registry-service.
This jira aims to get the container's ip and host from the NM, primarily docker container



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
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