> On March 17, 2016, 1:35 p.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?
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 > > <https://reviews.apache.org/r/44514/diff/3/?file=1301051#file1301051line84> > > > > 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 > > <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. 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 > >
