mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Avinash sridharan <avin...@mesosphere.io>
Subject Re: Review Request 51097: Added a `PortMapper` class.
Date Sat, 03 Sep 2016 01:27:23 GMT


> On Sept. 2, 2016, 11:46 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp,
line 36
> > <https://reviews.apache.org/r/51097/diff/15/?file=1490676#file1490676line36>
> >
> >     Should put before `using std::cout`;

you mean using mesos ... should come before using std:: ...

I think we follow:
using std...

followed by 
using process

followed by 
using mesos

https://github.com/apache/mesos/blob/79b6a9e38bdeb711bd25c890e7ad88a01d7dbfea/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L39


> On Sept. 2, 2016, 11:46 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp,
line 69
> > <https://reviews.apache.org/r/51097/diff/15/?file=1490676#file1490676line69>
> >
> >     I think it would be better to define a `CNIError` exetends from `Error`, and
return `Try<Owned<PorMapper>, CNIError>` here?

Valid point. Was thinking of something similar since spec::Error was becoming unweildy.


> On Sept. 2, 2016, 11:46 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp,
line 69
> > <https://reviews.apache.org/r/51097/diff/15/?file=1490677#file1490677line69>
> >
> >     Should return `Try<Nothing>`, return `"OK"` looks a bit wried.

This is just a place holder function. In follow up patches will be implementing execute. Will
return "". In this patch. Need to return string representation of `spec::NetworkInfo` since
that is what is expected.


- Avinash


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


On Sept. 2, 2016, 10:33 p.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51097/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2016, 10:33 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.
> 
> Also added helper functions in `cni::spec` to be able to log CNI
> plugin error as a JSON formatted string of `spec::error`.
> 
> 
> 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 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp e6967415a689184f13d65a986f13b0af54364fb2

>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 938a9f3bff2a95c8591450f0edafaddb01833e59

> 
> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message