Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review125362
---


Ship it!




Ship It!

- Jie Yu


On March 24, 2016, 2:34 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 24, 2016, 2:34 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 24, 2016, 10:34 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
9552312f9ba1e4df6564cfb737cc41e041cf4407 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 24, 2016, 5:04 a.m., Cong Wang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 228
> > 
> >
> > You always have ifIndex==0 here, it is never changed. So either you 
> > miss some code or it is not needed at all.

Thanks for catching this! Will fix it soon.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review125131
---


On March 23, 2016, 1:10 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 23, 2016, 1:10 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Cong Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review125131
---




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 228)


You always have ifIndex==0 here, it is never changed. So either you miss 
some code or it is not needed at all.


- Cong Wang


On March 23, 2016, 5:10 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 23, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Cong Wang]

> On March 11, 2016, 6:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 228
> > 
> >
> > Can there be a use case where you want multiple NICs to be attached to 
> > the same network? Servers use this configuration when they want to utilize 
> > NIC bonding. To aggregate the bandwidth available on the NICs
> 
> Qian Zhang wrote:
> Good point! I think it makes sense that user enables NIC bonding by 
> creating a bond device (e.g., bond0) as the master of the normal ethernet 
> devices (e.g., eth0 and eth1), and both eth0 and eth1 are set up by CNI 
> plugin and get assigned IP from CNI plugin in the same subnet. My only 
> concern is, how to configure the IP for bond0, maybe just use IP of either 
> eth0 or eth1 as its IP?
> 
> Avinash sridharan wrote:
> I think for the time being we can leave the behavior as is. To support 
> something like NIC bonding, I think we will need to introduce the concept of 
> `repeated` devices in `NetworkInfo`, which is definitely out of the scope of 
> this work.

Bonding is a L2 device as a whole, therefore you should just set IP address on 
the bonding master, its slaves don't need IP addresses.


- Cong


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123176
---


On March 23, 2016, 5:10 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 23, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 9552312f9ba1e4df6564cfb737cc41e041cf4407 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 23, 2016, 1:10 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
3878a7e85fe24175b3bd5e3a6268cf32a07f2d8b 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
9552312f9ba1e4df6564cfb737cc41e041cf4407 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review124881
---




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 117)


Can you use 'Owned' here?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 228)


s/i/ifIndex/



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 229)


s/names/networkNames/



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 249 - 250)


I would make it more explicit here:
```
NetworkInfo networkInfo;
networkInfo.networkName = networkName;
networkInfo.ifName = "eth" + stringify(ifIndex);

networkInfos.put(name, networkInfo);
```


- Jie Yu


On March 22, 2016, 9:50 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 22, 2016, 9:50 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 22, 2016, 5:50 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 22, 2016, 5:30 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 22, 2016, 5:06 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 20, 2016, 7:06 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 82
> > 
> >
> > s/NetworkResultInfo/Info/
> > 
> > We typically use 'Info' to store information about each container in 
> > isolators.
> > 
> > We can then have a hashmap from network name to a another data 
> > structure in Info to represent the networks the container has joined.
> > 
> > ```
> > // Information about a network that a container has joined.
> > struct NetworkInfo
> > {
> >   string ifname;
> >   cni::NetworkResult result;
> > };
> > 
> > struct Info
> > {
> >   hashmap networkInfos;
> > };
> > ```
> 
> Qian Zhang wrote:
> Are we going to support the use case that one container joins one CNI 
> network multiple times? If yes, then I think we may need to use `vector` 
> rather than `hashmap` in `struct Info`, like this:
> 
> struct NetworkResultInfo
> {
>   // CNI network name.
>   std::string name;
> 
>   // Interface name.
>   const std::string ifname;
> 
>   // Protobuf of CNI network result returned by CNI plugin
>   Option result;
> };
> 
> struct Info
> {
>   std::vector networkInfos;
> };
> 
> Jie Yu wrote:
> > Are we going to support the use case that one container joins one CNI 
> network multiple times?
> 
> Let's not consider this for now. I don't see a use case at this moment.
> 
> Also, I think the name 'NetworkResult' is weird (what result? what's 
> network result?!). Can we rename it to NetworkInfo?
> 
> ```
> struct NetworkInfo
> {
>   string name;
>   string ifname;
>   Option network;
> }
> 
> struct Info
> {
>   hashmap networkInfos;
> }
> ```
> 
> Qian Zhang wrote:
> Do we need to support NIC bonding (e.g., both eth0 and eth1 of a 
> container are in one CNI network)? If we do not need to support it, then I 
> agree we should use hashmap rather than vector, and I will add a check in 
> `prepare()` to make sure one container only joins a CNI network once, 
> otherwise return Failure.

Let's not consider NIC bonding for now. We can easily s/hashmap/multihashmap/ 
if we want to support it in the future.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review124451
---


On March 18, 2016, 6:31 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 6:31 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 20, 2016, 7:06 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 82
> > 
> >
> > s/NetworkResultInfo/Info/
> > 
> > We typically use 'Info' to store information about each container in 
> > isolators.
> > 
> > We can then have a hashmap from network name to a another data 
> > structure in Info to represent the networks the container has joined.
> > 
> > ```
> > // Information about a network that a container has joined.
> > struct NetworkInfo
> > {
> >   string ifname;
> >   cni::NetworkResult result;
> > };
> > 
> > struct Info
> > {
> >   hashmap networkInfos;
> > };
> > ```
> 
> Qian Zhang wrote:
> Are we going to support the use case that one container joins one CNI 
> network multiple times? If yes, then I think we may need to use `vector` 
> rather than `hashmap` in `struct Info`, like this:
> 
> struct NetworkResultInfo
> {
>   // CNI network name.
>   std::string name;
> 
>   // Interface name.
>   const std::string ifname;
> 
>   // Protobuf of CNI network result returned by CNI plugin
>   Option result;
> };
> 
> struct Info
> {
>   std::vector networkInfos;
> };

> Are we going to support the use case that one container joins one CNI network 
> multiple times?

Let's not consider this for now. I don't see a use case at this moment.

Also, I think the name 'NetworkResult' is weird (what result? what's network 
result?!). Can we rename it to NetworkInfo?

```
struct NetworkInfo
{
  string name;
  string ifname;
  Option network;
}

struct Info
{
  hashmap networkInfos;
}
```


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review124451
---


On March 18, 2016, 6:31 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 6:31 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 21, 2016, 3:06 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 82
> > 
> >
> > s/NetworkResultInfo/Info/
> > 
> > We typically use 'Info' to store information about each container in 
> > isolators.
> > 
> > We can then have a hashmap from network name to a another data 
> > structure in Info to represent the networks the container has joined.
> > 
> > ```
> > // Information about a network that a container has joined.
> > struct NetworkInfo
> > {
> >   string ifname;
> >   cni::NetworkResult result;
> > };
> > 
> > struct Info
> > {
> >   hashmap networkInfos;
> > };
> > ```

Are we going to support the use case that one container joins one CNI network 
multiple times? If yes, then I think we may need to use `vector` rather than 
`hashmap` in `struct Info`, like this:

struct NetworkResultInfo
{
  // CNI network name.
  std::string name;

  // Interface name.
  const std::string ifname;

  // Protobuf of CNI network result returned by CNI plugin
  Option result;
};

struct Info
{
  std::vector networkInfos;
};


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review124451
---


On March 18, 2016, 2:31 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 2:31 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review124451
---




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 82)


s/NetworkResultInfo/Info/

We typically use 'Info' to store information about each container in 
isolators.

We can then have a hashmap from network name to a another data structure in 
Info to represent the networks the container has joined.

```
// Information about a network that a container has joined.
struct NetworkInfo
{
  string ifname;
  cni::NetworkResult result;
};

struct Info
{
  hashmap networkInfos;
};
```


- Jie Yu


On March 18, 2016, 6:31 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 6:31 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

> On March 17, 2016, 5:35 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 88
> > 
> >
> > Should this be a `Result` instead of an `Option` ? Since either the 
> > container was able to join the network or wasn't due to an error. 
> > 
> > I am thinking about a case where a container wants to join multiple 
> > networks, it is successful in joining a few but fails due to misconfig or 
> > unavailable IP addresses. In this case should we store an Error?
> 
> Qian Zhang wrote:
> I think `Result` is usually used to define the return value of a method 
> but not a field/member in a struct/class. And I am not sure why we want to 
> store an Error in the isolator. I think if a container wants to join multiple 
> networks and fails to join any one of them, we should just return an Error.

Valid argument. All or none makes sense. We should just log an ERROR during the 
isolate phase if this event happens and return an ERROR.


> On March 17, 2016, 5:35 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 240-244
> > 
> >
> > I think this comment is for static IP addresses. You should be 
> > prepoulating the NetworkResultInfo with the IP Address and use it when 
> > calling the plugin. 
> > 
> > `host-local` is something different right? It just uses a local IPAM to 
> > allocated addresses.
> 
> Qian Zhang wrote:
> In future, when we support framework to request static IP for its 
> container, we will need to introduce a new field in `NetworkResultInfo` and 
> fill it here with the value of `containerInfo.networkInfo.ip_addresses`, and 
> then in `isolate()`, use it as `CNI_ARGS` to call plugin. And we should only 
> do these if the IPAM plugin is `host-local`, since currently `host-local` is 
> the only IPAM plugin which can support to request a static IP.

Firstly, the CNI isolator should plugin agnostic. It shouldn'be aware of which 
plugin to invoke. Also host-local is just a poor man's DHCP server, there is 
nothing special that host-local provides in terms of static address allocation. 

I do agree that the CNI spec currently doesn't allow for the passing of an IP 
address as a command line argument. However, if we do want to support static IP 
addressing we could do the following:

a) If IP address has been set on the NetworkInfo, store it in 
`NetworkResultInfo`
b) During the isolate phase when we get the JSON from the config, modify the 
JSON for IPAM, by setting a /32 subnet. This is effectively requesting a 
specific IP address from the IPAM. Pass the mutated JSON to the plugin. 

Ofcourse we need to try this out. 

My suggestion is to remove this comment (since its not accurate), and then 
revisit static IP addressing once we have tried out the feasibility of the 
above algorithm.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123984
---


On March 17, 2016, 1:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 17, 2016, 1:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 17, 2016, 1:35 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 88
> > 
> >
> > Should this be a `Result` instead of an `Option` ? Since either the 
> > container was able to join the network or wasn't due to an error. 
> > 
> > I am thinking about a case where a container wants to join multiple 
> > networks, it is successful in joining a few but fails due to misconfig or 
> > unavailable IP addresses. In this case should we store an Error?

I think `Result` is usually used to define the return value of a method but not 
a field/member in a struct/class. And I am not sure why we want to store an 
Error in the isolator. I think if a container wants to join multiple networks 
and fails to join any one of them, we should just return an Error.


> On March 17, 2016, 1:35 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 84
> > 
> >
> > Define a constructor
> > NetworkResultInfo(_name, _result):
> >  name(_name), result(_result)
> >  
> >  Will be useful while initializing objects.

I think for a very simple struct, it should be OK to not define a constructor, 
e.g., 
https://github.com/apache/mesos/blob/0.27.2/src/master/quota_handler.cpp#L301


> On March 17, 2016, 1:35 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 240-244
> > 
> >
> > I think this comment is for static IP addresses. You should be 
> > prepoulating the NetworkResultInfo with the IP Address and use it when 
> > calling the plugin. 
> > 
> > `host-local` is something different right? It just uses a local IPAM to 
> > allocated addresses.

In future, when we support framework to request static IP for its container, we 
will need to introduce a new field in `NetworkResultInfo` and fill it here with 
the value of `containerInfo.networkInfo.ip_addresses`, and then in `isolate()`, 
use it as `CNI_ARGS` to call plugin. And we should only do these if the IPAM 
plugin is `host-local`, since currently `host-local` is the only IPAM plugin 
which can support to request a static IP.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123984
---


On March 16, 2016, 8:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 16, 2016, 8:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 18, 2016, 2:31 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 17, 2016, 1:35 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 240-244
> > 
> >
> > I think this comment is for static IP addresses. You should be 
> > prepoulating the NetworkResultInfo with the IP Address and use it when 
> > calling the plugin. 
> > 
> > `host-local` is something different right? It just uses a local IPAM to 
> > allocated addresses.
> 
> Qian Zhang wrote:
> In future, when we support framework to request static IP for its 
> container, we will need to introduce a new field in `NetworkResultInfo` and 
> fill it here with the value of `containerInfo.networkInfo.ip_addresses`, and 
> then in `isolate()`, use it as `CNI_ARGS` to call plugin. And we should only 
> do these if the IPAM plugin is `host-local`, since currently `host-local` is 
> the only IPAM plugin which can support to request a static IP.
> 
> Avinash sridharan wrote:
> Firstly, the CNI isolator should plugin agnostic. It shouldn'be aware of 
> which plugin to invoke. Also host-local is just a poor man's DHCP server, 
> there is nothing special that host-local provides in terms of static address 
> allocation. 
> 
> I do agree that the CNI spec currently doesn't allow for the passing of 
> an IP address as a command line argument. However, if we do want to support 
> static IP addressing we could do the following:
> 
> a) If IP address has been set on the NetworkInfo, store it in 
> `NetworkResultInfo`
> b) During the isolate phase when we get the JSON from the config, modify 
> the JSON for IPAM, by setting a /32 subnet. This is effectively requesting a 
> specific IP address from the IPAM. Pass the mutated JSON to the plugin. 
> 
> Ofcourse we need to try this out. 
> 
> My suggestion is to remove this comment (since its not accurate), and 
> then revisit static IP addressing once we have tried out the feasibility of 
> the above algorithm.

OK, let me remove the comment, we could revisit static IP addressing feature at 
post-MVP.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123984
---


On March 17, 2016, 9:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 17, 2016, 9:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 16, 2016, 8:57 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123984
---




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 84)


Define a constructor
NetworkResultInfo(_name, _result):
 name(_name), result(_result)
 
 Will be useful while initializing objects.



src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 88)


Should this be a `Result` instead of an `Option` ? Since either the 
container was able to join the network or wasn't due to an error. 

I am thinking about a case where a container wants to join multiple 
networks, it is successful in joining a few but fails due to misconfig or 
unavailable IP addresses. In this case should we store an Error?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 238)


If you have the constructor you can do 
NetworkResultInfo(name)



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 240 - 244)


I think this comment is for static IP addresses. You should be prepoulating 
the NetworkResultInfo with the IP Address and use it when calling the plugin. 

`host-local` is something different right? It just uses a local IPAM to 
allocated addresses.


- Avinash sridharan


On March 16, 2016, 12:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 16, 2016, 12:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 17, 2016, 9:17 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/
---

(Updated March 16, 2016, 3:49 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Bugs: MESOS-4759
https://issues.apache.org/jira/browse/MESOS-4759


Repository: mesos


Description
---

Implemented prepare() method of "network/cni" isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44514/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.
> 
> Qian Zhang wrote:
> Yes, that is the tricky part. For orphan container (e.g., the container 
> created by the framework without checkpoint enabled), when agent restarts, in 
> the recover() we only have container ID which is not enough to invoke CNI 
> plugin DEL. Totally we need:
> 1) Container ID.
> 2) Network namespace path.
> 3) Network configuration.
> 4) Name of the interface inside the container.
> 
> I think currently we only have 1).
> For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a 
> bind mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol 
> link (`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). 
> And then in `recover()`, since we have `containerId` for orphan containers, 
> we should be able to figure out the `pid` based on the symbol link, and then 
> get the network namespace path `/proc/[pid]/ns/net`.
> For 3), actually when we call CNI plugin for a container in `isolate()`, 
> the CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file 
> under `/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP 
> assigned to the container, and the content of file is the container ID. So if 
> a container is assigned an IP from a CNI network, it must have a file under 
> `/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
> figure out the name of CNI networks the orphan container joins. But this may 
> only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
> IPAM plugin, I think it will not write any files for containers.
> For 4), I do not have idea for it yet :-(
> 
> Or another solution would be, when we create the symbol link in 
> `isolate()`, we may encode more info into the name of symbol link, e.g., 
> `/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in 
> net1]-[net2]_[ifName in net2]...`, however this seems ugly to me.
> 
> Avinash sridharan wrote:
> For (2) why not bind mount directly to 
> /var/run/mesos/cni/netns/[continaerId] and create a simlink to 
> /var/run/netns/. Also, Jie mentioned it would be a good idea to prefix the 
> `[containerId]` with a conanical string like mesos-cni. For (3) I don't think 
> we should rely on files created by CNI plugins. There is no guarantee that 
> the IPAM plugin would maintain those files. Instead if we have already bind 
> moutned the network namespace, the namespace will persist in the kernel even 
> after the container has exited (became an orphan). At recover we just need to 
> walk to list of `containerID` stored under /var/run/mesos/cni/netns and for 
> each network namespace we can run the `ip netns comamnd` to retrieve the link 
> and IP address for that network namespace.
> 
> Qian Zhang wrote:
> For 2), what symlink do you want to create under `/var/run/netns`? 
> `/var/run/netns/[pid] -> /var/run/mesos/cni/netns/[containerId]`? Or 
> `/var/run/netns/[containerId] -> /var/run/mesos/cni/netns/[contiainerId]`? 
> And you can take a look at 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L2205:L2243,
>  What it does is same as what I proposed above.
> 
> And good point for 3), I agree in `recover()`, we can run `ip netns exec 
> [namespace] ip addr` to retrieve the link and IP, but the problem is how we 
> can know which CNI networks 

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 4:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.
> 
> Qian Zhang wrote:
> Yes, that is the tricky part. For orphan container (e.g., the container 
> created by the framework without checkpoint enabled), when agent restarts, in 
> the recover() we only have container ID which is not enough to invoke CNI 
> plugin DEL. Totally we need:
> 1) Container ID.
> 2) Network namespace path.
> 3) Network configuration.
> 4) Name of the interface inside the container.
> 
> I think currently we only have 1).
> For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a 
> bind mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol 
> link (`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). 
> And then in `recover()`, since we have `containerId` for orphan containers, 
> we should be able to figure out the `pid` based on the symbol link, and then 
> get the network namespace path `/proc/[pid]/ns/net`.
> For 3), actually when we call CNI plugin for a container in `isolate()`, 
> the CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file 
> under `/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP 
> assigned to the container, and the content of file is the container ID. So if 
> a container is assigned an IP from a CNI network, it must have a file under 
> `/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
> figure out the name of CNI networks the orphan container joins. But this may 
> only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
> IPAM plugin, I think it will not write any files for containers.
> For 4), I do not have idea for it yet :-(
> 
> Or another solution would be, when we create the symbol link in 
> `isolate()`, we may encode more info into the name of symbol link, e.g., 
> `/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in 
> net1]-[net2]_[ifName in net2]...`, however this seems ugly to me.
> 
> Avinash sridharan wrote:
> For (2) why not bind mount directly to 
> /var/run/mesos/cni/netns/[continaerId] and create a simlink to 
> /var/run/netns/. Also, Jie mentioned it would be a good idea to prefix the 
> `[containerId]` with a conanical string like mesos-cni. For (3) I don't think 
> we should rely on files created by CNI plugins. There is no guarantee that 
> the IPAM plugin would maintain those files. Instead if we have already bind 
> moutned the network namespace, the namespace will persist in the kernel even 
> after the container has exited (became an orphan). At recover we just need to 
> walk to list of `containerID` stored under /var/run/mesos/cni/netns and for 
> each network namespace we can run the `ip netns comamnd` to retrieve the link 
> and IP address for that network namespace.
> 
> Qian Zhang wrote:
> For 2), what symlink do you want to create under `/var/run/netns`? 
> `/var/run/netns/[pid] -> /var/run/mesos/cni/netns/[containerId]`? Or 
> `/var/run/netns/[containerId] -> /var/run/mesos/cni/netns/[contiainerId]`? 
> And you can take a look at 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L2205:L2243,
>  What it does is same as what I proposed above.
> 
> And good point for 3), I agree in `recover()`, we can run `ip netns exec 
> [namespace] ip addr` to retrieve the link and IP, but the problem is how we 
> can know which CNI networks 

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.
> 
> Qian Zhang wrote:
> Yes, that is the tricky part. For orphan container (e.g., the container 
> created by the framework without checkpoint enabled), when agent restarts, in 
> the recover() we only have container ID which is not enough to invoke CNI 
> plugin DEL. Totally we need:
> 1) Container ID.
> 2) Network namespace path.
> 3) Network configuration.
> 4) Name of the interface inside the container.
> 
> I think currently we only have 1).
> For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a 
> bind mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol 
> link (`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). 
> And then in `recover()`, since we have `containerId` for orphan containers, 
> we should be able to figure out the `pid` based on the symbol link, and then 
> get the network namespace path `/proc/[pid]/ns/net`.
> For 3), actually when we call CNI plugin for a container in `isolate()`, 
> the CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file 
> under `/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP 
> assigned to the container, and the content of file is the container ID. So if 
> a container is assigned an IP from a CNI network, it must have a file under 
> `/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
> figure out the name of CNI networks the orphan container joins. But this may 
> only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
> IPAM plugin, I think it will not write any files for containers.
> For 4), I do not have idea for it yet :-(
> 
> Or another solution would be, when we create the symbol link in 
> `isolate()`, we may encode more info into the name of symbol link, e.g., 
> `/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in 
> net1]-[net2]_[ifName in net2]...`, however this seems ugly to me.
> 
> Avinash sridharan wrote:
> For (2) why not bind mount directly to 
> /var/run/mesos/cni/netns/[continaerId] and create a simlink to 
> /var/run/netns/. Also, Jie mentioned it would be a good idea to prefix the 
> `[containerId]` with a conanical string like mesos-cni. For (3) I don't think 
> we should rely on files created by CNI plugins. There is no guarantee that 
> the IPAM plugin would maintain those files. Instead if we have already bind 
> moutned the network namespace, the namespace will persist in the kernel even 
> after the container has exited (became an orphan). At recover we just need to 
> walk to list of `containerID` stored under /var/run/mesos/cni/netns and for 
> each network namespace we can run the `ip netns comamnd` to retrieve the link 
> and IP address for that network namespace.
> 
> Qian Zhang wrote:
> For 2), what symlink do you want to create under `/var/run/netns`? 
> `/var/run/netns/[pid] -> /var/run/mesos/cni/netns/[containerId]`? Or 
> `/var/run/netns/[containerId] -> /var/run/mesos/cni/netns/[contiainerId]`? 
> And you can take a look at 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L2205:L2243,
>  What it does is same as what I proposed above.
> 
> And good point for 3), I agree in `recover()`, we can run `ip netns exec 
> [namespace] ip addr` to retrieve the link and IP, but the problem is how we 
> can know which CNI networks 

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 4:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.
> 
> Qian Zhang wrote:
> Yes, that is the tricky part. For orphan container (e.g., the container 
> created by the framework without checkpoint enabled), when agent restarts, in 
> the recover() we only have container ID which is not enough to invoke CNI 
> plugin DEL. Totally we need:
> 1) Container ID.
> 2) Network namespace path.
> 3) Network configuration.
> 4) Name of the interface inside the container.
> 
> I think currently we only have 1).
> For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a 
> bind mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol 
> link (`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). 
> And then in `recover()`, since we have `containerId` for orphan containers, 
> we should be able to figure out the `pid` based on the symbol link, and then 
> get the network namespace path `/proc/[pid]/ns/net`.
> For 3), actually when we call CNI plugin for a container in `isolate()`, 
> the CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file 
> under `/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP 
> assigned to the container, and the content of file is the container ID. So if 
> a container is assigned an IP from a CNI network, it must have a file under 
> `/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
> figure out the name of CNI networks the orphan container joins. But this may 
> only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
> IPAM plugin, I think it will not write any files for containers.
> For 4), I do not have idea for it yet :-(
> 
> Or another solution would be, when we create the symbol link in 
> `isolate()`, we may encode more info into the name of symbol link, e.g., 
> `/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in 
> net1]-[net2]_[ifName in net2]...`, however this seems ugly to me.
> 
> Avinash sridharan wrote:
> For (2) why not bind mount directly to 
> /var/run/mesos/cni/netns/[continaerId] and create a simlink to 
> /var/run/netns/. Also, Jie mentioned it would be a good idea to prefix the 
> `[containerId]` with a conanical string like mesos-cni. For (3) I don't think 
> we should rely on files created by CNI plugins. There is no guarantee that 
> the IPAM plugin would maintain those files. Instead if we have already bind 
> moutned the network namespace, the namespace will persist in the kernel even 
> after the container has exited (became an orphan). At recover we just need to 
> walk to list of `containerID` stored under /var/run/mesos/cni/netns and for 
> each network namespace we can run the `ip netns comamnd` to retrieve the link 
> and IP address for that network namespace.

For 2), what symlink do you want to create under `/var/run/netns`? 
`/var/run/netns/[pid] -> /var/run/mesos/cni/netns/[containerId]`? Or 
`/var/run/netns/[containerId] -> /var/run/mesos/cni/netns/[contiainerId]`? And 
you can take a look at 
https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L2205:L2243,
 What it does is same as what I proposed above.

And good point for 3), I agree in `recover()`, we can run `ip netns exec 
[namespace] ip addr` to retrieve the link and IP, but the problem is how we can 
know which CNI networks that these links/IPs belong to? E.g., after run `ip 

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.
> 
> Qian Zhang wrote:
> Yes, that is the tricky part. For orphan container (e.g., the container 
> created by the framework without checkpoint enabled), when agent restarts, in 
> the recover() we only have container ID which is not enough to invoke CNI 
> plugin DEL. Totally we need:
> 1) Container ID.
> 2) Network namespace path.
> 3) Network configuration.
> 4) Name of the interface inside the container.
> 
> I think currently we only have 1).
> For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a 
> bind mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol 
> link (`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). 
> And then in `recover()`, since we have `containerId` for orphan containers, 
> we should be able to figure out the `pid` based on the symbol link, and then 
> get the network namespace path `/proc/[pid]/ns/net`.
> For 3), actually when we call CNI plugin for a container in `isolate()`, 
> the CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file 
> under `/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP 
> assigned to the container, and the content of file is the container ID. So if 
> a container is assigned an IP from a CNI network, it must have a file under 
> `/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
> figure out the name of CNI networks the orphan container joins. But this may 
> only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
> IPAM plugin, I think it will not write any files for containers.
> For 4), I do not have idea for it yet :-(
> 
> Or another solution would be, when we create the symbol link in 
> `isolate()`, we may encode more info into the name of symbol link, e.g., 
> `/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in 
> net1]-[net2]_[ifName in net2]...`, however this seems ugly to me.

For (2) why not bind mount directly to /var/run/mesos/cni/netns/[continaerId] 
and create a simlink to /var/run/netns/. Also, Jie mentioned it would be a good 
idea to prefix the `[containerId]` with a conanical string like mesos-cni. For 
(3) I don't think we should rely on files created by CNI plugins. There is no 
guarantee that the IPAM plugin would maintain those files. Instead if we have 
already bind moutned the network namespace, the namespace will persist in the 
kernel even after the container has exited (became an orphan). At recover we 
just need to walk to list of `containerID` stored under 
/var/run/mesos/cni/netns and for each network namespace we can run the `ip 
netns comamnd` to retrieve the link and IP address for that network namespace.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123200
---


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> 

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 6:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 228
> > 
> >
> > Can there be a use case where you want multiple NICs to be attached to 
> > the same network? Servers use this configuration when they want to utilize 
> > NIC bonding. To aggregate the bandwidth available on the NICs
> 
> Qian Zhang wrote:
> Good point! I think it makes sense that user enables NIC bonding by 
> creating a bond device (e.g., bond0) as the master of the normal ethernet 
> devices (e.g., eth0 and eth1), and both eth0 and eth1 are set up by CNI 
> plugin and get assigned IP from CNI plugin in the same subnet. My only 
> concern is, how to configure the IP for bond0, maybe just use IP of either 
> eth0 or eth1 as its IP?

I think for the time being we can leave the behavior as is. To support 
something like NIC bonding, I think we will need to introduce the concept of 
`repeated` devices in `NetworkInfo`, which is definitely out of the scope of 
this work.


> On March 11, 2016, 6:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 236
> > 
> >
> > If the CNI configuration is host-local then we will be calling the 
> > host-local plugin during `isolate`. So why do we need this?
> 
> Qian Zhang wrote:
> The purpose of this comment is, when we support to explicitly request an 
> IP for container in future, here we need to get `NetworkInfo.ip_addresses` to 
> know which IP the container want to request, and keep it in the isolator 
> (e.g., in the Info struct for the container), and later in isolate() method, 
> we need to set the value of the environment variable `CNI_ARGS` to that IP 
> when invoking plugin, see 
> https://github.com/appc/cni/blob/master/Documentation/host-local.md#supported-arguments
>  for more details.

Now that we have a `struct Info` for each container, we should have 
`NetworkInfo` as a field in `struct Info`. We can store any statically defined 
IP addresses in the `NetworkInfo`.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123176
---


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 4:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.

Yes, that is the tricky part. For orphan container (e.g., the container created 
by the framework without checkpoint enabled), when agent restarts, in the 
recover() we only have container ID which is not enough to invoke CNI plugin 
DEL. Totally we need:
1) Container ID.
2) Network namespace path.
3) Network configuration.
4) Name of the interface inside the container.

I think currently we only have 1).
For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a bind 
mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol link 
(`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). And then 
in `recover()`, since we have `containerId` for orphan containers, we should be 
able to figure out the `pid` based on the symbol link, and then get the network 
namespace path `/proc/[pid]/ns/net`.
For 3), actually when we call CNI plugin for a container in `isolate()`, the 
CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file under 
`/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP assigned 
to the container, and the content of file is the container ID. So if a 
container is assigned an IP from a CNI network, it must have a file under 
`/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
figure out the name of CNI networks the orphan container joins. But this may 
only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
IPAM plugin, I think it will not write any files for containers.
For 4), I do not have idea for it yet :-(

Or another solution would be, when we create the symbol link in `isolate()`, we 
may encode more info into the name of symbol link, e.g., 
`/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in net1]-[net2]_[ifName 
in net2]...`, however this seems ugly to me.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123200
---


On March 10, 2016, 10:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 2:19 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 228
> > 
> >
> > Can there be a use case where you want multiple NICs to be attached to 
> > the same network? Servers use this configuration when they want to utilize 
> > NIC bonding. To aggregate the bandwidth available on the NICs

Good point! I think it makes sense that user enables NIC bonding by creating a 
bond device (e.g., bond0) as the master of the normal ethernet devices (e.g., 
eth0 and eth1), and both eth0 and eth1 are set up by CNI plugin and get 
assigned IP from CNI plugin in the same subnet. My only concern is, how to 
configure the IP for bond0, maybe just use IP of either eth0 or eth1 as its IP?


> On March 12, 2016, 2:19 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 236
> > 
> >
> > If the CNI configuration is host-local then we will be calling the 
> > host-local plugin during `isolate`. So why do we need this?

The purpose of this comment is, when we support to explicitly request an IP for 
container in future, here we need to get `NetworkInfo.ip_addresses` to know 
which IP the container want to request, and keep it in the isolator (e.g., in 
the Info struct for the container), and later in isolate() method, we need to 
set the value of the environment variable `CNI_ARGS` to that IP when invoking 
plugin, see 
https://github.com/appc/cni/blob/master/Documentation/host-local.md#supported-arguments
 for more details.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123176
---


On March 10, 2016, 10:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?

I am not sure. Why do we need to store/recover IP address? When destroy a 
container, the CNI plugin only needs container ID (which can be the same as our 
container Id), the namespace handle and the networkconfig file. The rest can 
all be fixed.

I think the trick part is: when there is an orphan container (no 
executorinfo/taskinfo), how can we figure out its network namespace handle and 
networconfig file so that we can destroy it (call CNI plugin DEL). We need to 
know the name of the network to get the networkcofig file, and the pid to get 
the network namespace handle.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123200
---


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.

We also need to recover the IP address associated with the container. Since 
this has to be returned as part of `NetworkINfo` . Need to figure out how to 
recover this?


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123200
---


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123200
---




src/slave/containerizer/mesos/isolators/network/cni.hpp (line 85)


I would suggest we have a `Info` for each container.

```
struct Info
{
  ...
};
```

Remember that for any field you put into the Info struct, you need to be 
able to 'recover' it in recover function. It's also possible that 
ExecutorInfo/TaskInfo are not available during recover (orphans due to wiped 
meta data). You need to think about how to recover (e.g., the name of the 
network) that the container has joined.


- Jie Yu


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44514/#review123176
---




src/slave/containerizer/mesos/isolators/network/cni.hpp (line 85)


I don't think we should have a hashmap within a hashmap. Hashmap are 
expensive in terms of memory. The outer hashmap makes sense since the isolator 
might need to manage hundreds of containers, but the inner hashmap is just a 
mapping from the network name to the result of attaching the container to that 
network, and most containers would joing a single network. Maybe just have a 
vector of strings (network names) here, and later, a vector of `NetworkResults` 
(once you introduce the protobuf) and name an `optional` field in the protobuf?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 228)


Can there be a use case where you want multiple NICs to be attached to the 
same network? Servers use this configuration when they want to utilize NIC 
bonding. To aggregate the bandwidth available on the NICs



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 236)


If the CNI configuration is host-local then we will be calling the 
host-local plugin during `isolate`. So why do we need this?


- Avinash sridharan


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>