----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51097/#review146782 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp (line 81) <https://reviews.apache.org/r/51097/#comment213786> I think we need to print `result.get()` on success. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (lines 30 - 31) <https://reviews.apache.org/r/51097/#comment213754> A newline between these two lines, and I think we should change them to: ``` #include <mesos/mesos.hpp> #include "slave/containerizer/mesos/isolators/network/cni/spec.hpp" ``` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 38) <https://reviews.apache.org/r/51097/#comment213755> A period in the end. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (lines 39 - 41) <https://reviews.apache.org/r/51097/#comment213788> I am not sure why we need two classes here, I think we can just have one class `PortMapper`, and in `PortMapper::execute()`, we can: 1. Execute delegate plugin via `subprocess`. 2. Wait for the execution of delegate plugin to finish. 3. Install port-forwarding rules. 4. Return the result of delegate plugin. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 44) <https://reviews.apache.org/r/51097/#comment213757> s/createPortMapper/create/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 52) <https://reviews.apache.org/r/51097/#comment213769> s/portMapperProcess/process/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (lines 61 - 62) <https://reviews.apache.org/r/51097/#comment213772> According to the design doc, it seems the `excludeDevList` field is missed here? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 64) <https://reviews.apache.org/r/51097/#comment213759> Missed a `"` in the end? src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (line 76) <https://reviews.apache.org/r/51097/#comment213760> s/createPortMapperProcess/create/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (lines 100 - 101) <https://reviews.apache.org/r/51097/#comment213773> It should be: ``` const std::map<std::string, std::string>& _environment) : environment(_environment), ``` src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp (lines 118 - 119) <https://reviews.apache.org/r/51097/#comment213761> A newline between these two lines. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 16) <https://reviews.apache.org/r/51097/#comment213762> A newline before this line. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 44) <https://reviews.apache.org/r/51097/#comment213764> A newline before this line, and a space after `if`. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 97) <https://reviews.apache.org/r/51097/#comment213776> I think in our context, we will always set this env var in CNI isolator (`NetworkCniIsolatorProcess::attach()`), so it should be a mandatory one. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 107) <https://reviews.apache.org/r/51097/#comment213777> s/cniIfname/cniIfName/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 113) <https://reviews.apache.org/r/51097/#comment213778> s/cniNetns/cniNetNs/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 169) <https://reviews.apache.org/r/51097/#comment213780> A newline before this line. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 191 - 192) <https://reviews.apache.org/r/51097/#comment213785> I think we also need to make sure the 'ipam' plugin (if any) exists and we have executable permissions on the plugin. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 194) <https://reviews.apache.org/r/51097/#comment213782> Kill this blank line. src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (line 196) <https://reviews.apache.org/r/51097/#comment213783> s/.get().value/->value/ src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp (lines 213 - 215) <https://reviews.apache.org/r/51097/#comment213784> Should change to: ``` } else if (!permissions.get().owner.x && !permissions.get().group.x && !permissions.get().others.x) { ``` - Qian Zhang On Aug. 26, 2016, 1:21 p.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51097/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 1:21 p.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 > >
