> On Aug. 26, 2016, 9:23 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp, > > line 97 > > <https://reviews.apache.org/r/51097/diff/8/?file=1486418#file1486418line97> > > > > I think in our context, we will always set this env var in CNI isolator > > (`NetworkCniIsolatorProcess::attach()`), so it should be a mandatory one.
Don't want to make the CNI plugin `network/cni` isolator specific. These fields are not expected by the `delegate` plugins so lets follow the spec. > On Aug. 26, 2016, 9:23 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp, > > line 107 > > <https://reviews.apache.org/r/51097/diff/8/?file=1486418#file1486418line107> > > > > s/cniIfname/cniIfName/ prefer `ifname` as a single word. > On Aug. 26, 2016, 9:23 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp, > > line 113 > > <https://reviews.apache.org/r/51097/diff/8/?file=1486418#file1486418line113> > > > > s/cniNetns/cniNetNs/ prefer `netns` as a single word. > On Aug. 26, 2016, 9:23 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp, > > line 169 > > <https://reviews.apache.org/r/51097/diff/8/?file=1486418#file1486418line169> > > > > A newline before this line. These two lines of code are related. So lets keep it together. > On Aug. 26, 2016, 9:23 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp, > > lines 191-192 > > <https://reviews.apache.org/r/51097/diff/8/?file=1486418#file1486418line191> > > > > I think we also need to make sure the 'ipam' plugin (if any) exists and > > we have executable permissions on the plugin. Valid point. > On Aug. 26, 2016, 9:23 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp, > > lines 61-62 > > <https://reviews.apache.org/r/51097/diff/8/?file=1486417#file1486417line61> > > > > According to the design doc, it seems the `excludeDevList` field is > > missed here? The `excludeDevList` is optional. It not necessary to set it always in the DNAT rule. So lets not make it mandatory here. Will update the document with this fact. > On Aug. 26, 2016, 9:23 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp, > > line 81 > > <https://reviews.apache.org/r/51097/diff/8/?file=1486416#file1486416line81> > > > > I think we need to print `result.get()` on success. The output needs to go to `STDOUT`. The `main` returns a code, so it should be returning `EXIT_SUCCESS`. - Avinash ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51097/#review146782 ----------------------------------------------------------- On Aug. 26, 2016, 5:21 a.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51097/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 5:21 a.m.) > > > Review request for mesos, Jie Yu and Qian Zhang. > > > Bugs: MESOS-6023 > https://issues.apache.org/jira/browse/MESOS-6023 > > > Repository: mesos > > > Description > ------- > > This class will embody the logic for implementing the CNI port-mapper > plugin. > > > Diffs > ----- > > src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp > PRE-CREATION > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp > PRE-CREATION > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp > PRE-CREATION > > Diff: https://reviews.apache.org/r/51097/diff/ > > > Testing > ------- > > Tested the port-mapper with the following CNI config: > { > "name": "mynet", > "type": "port-mapper", > "chain": "MESOS", > "delegate": { > "type" : "bridge", > "bridge": "cni0", > "isGateway": true, > "ipMasq": true, > "ipam": { > "type": "host-local", > "subnet": "10.22.0.0/16", > "routes": [ > { "dst": "0.0.0.0/0" } > ] > } > }, > "args" : { > "org.apache.mesos" : { > "network_info" : { > "port_mappings": { > "host_port" : 80, > "container_port" : 80 > } > } > } > } > } > > and the following environment variables: > export CNI_COMMAND="ADD" > export CNI_CONTAINERID="0000000111101110" > export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin" > export CNI_IFNAME="eth0" > export CNI_NETNS="/etc/netns" > > If we remove fields from the above CNI config, or remove certain environment > variables the creation of the `PortMapper` correctly fails. However, if > config and environment variables are passed as is it will create the > `PortMapper` correctly. > > > Thanks, > > Avinash sridharan > >
