> On March 17, 2016, 5:35 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 88
> > <https://reviews.apache.org/r/44514/diff/3/?file=1301051#file1301051line88>
> >
> >     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
> > <https://reviews.apache.org/r/44514/diff/3/?file=1301052#file1301052line240>
> >
> >     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
> 
>

Reply via email to