----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21594/#review46101 -----------------------------------------------------------
include/mesos/mesos.proto <https://reviews.apache.org/r/21594/#comment81261> I think this merits its own review. Can you split this out? also, put benh on that review. src/slave/constants.hpp <https://reviews.apache.org/r/21594/#comment81263> s/DEFAULT_PER_CONTAINER_EPHEMERAL_PORT_SIZE/DEFAULT_EPHEMERAL_PORTS_PER_CONTAINER/ ? src/slave/constants.hpp <https://reviews.apache.org/r/21594/#comment81262> s/ports/ports range/ src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81271> s/perContainerSize/portsPerContainer/ ? src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81272> Should this not return an error if the range is already allocated? src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81270> s/perContainerSize_/portsPerContainer_/ ? src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81273> s/re-use/reuse/ src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81275> How about a single constructor that takes an Option<pid_t> with maybe a default of None()? src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81274> s/port/ports/ src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81279> s/childIsolationScript/scripts/ ? src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment81278> What does this mean? Are there un-managed nonEphemeral ports? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81281> s/purpose/purposes/ ? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81284> how about "t is aligned to power of 2" ? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81286> Not sure I understand this algorithm. Maybe some comments? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81380> why is this pulled out into a function? seems like it is used only once? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81288> print the value.get()? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81289> print split[0]? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81290> print split[1]? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81381> ditto. don't pull out. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81292> const? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81294> s/host// ? "host etho" would be weird in the log? looks like etho is hostname. here and everywhere else. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81385> Why do we drop these? Are there no apps out there which spoof the source ip? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81387> ditto. why drop? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81393> print the pid? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81395> What should users do when they encounter this error? Maybe include that in the message? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81396> s/checkRoutingLibrary/check/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81397> s/checkCommandTc/shell/ or s/checkCommandTc/status/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81398> reuse the variable from above. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81399> as commented earlier, these should really be called "ephemeral_ports" in the flag. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81400> hmm. this name "previousPowerofTwo" here doesn't really convey what you are looking for. is there a better name? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81401> print the sizes. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81402> print that it needs atleast MIN_EPHEMERAL_PORTS_SIZE src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81403> s/hostEthoExists/exists/. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81404> How about if (!hostIP.isSome()) { return Error("Failed to get the public IP of " + eth0.get() + ": " + (hostIP.isError() ? hostIP.error() : "does not have IPv4 address")); } src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81405> ditto. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81406> s/fair/fairly/. Also, a blurb about why we need to do this? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81407> What if it's None? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81408> +1 src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81413> I don't think the containerizer is supposed to ask isolators to recover a run that doesn't have a forked pid. So this should be a CHECK instead? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81416> This would log once for every container! We don't do this with other isolators. Do you want a VLOG instead? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81420> s/expectedly/unexpectedly/ ? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81421> s/should/we should/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81422> VLOG? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81423> also print the executor id. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81425> "target to source" or "source to target"? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81429> s/is/to be/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81432> Add a comment on what this function does? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment81435> s/allocator/ephemeral ports allocator/ src/slave/flags.hpp <https://reviews.apache.org/r/21594/#comment81268> Maybe add a blurb about port mapping isolation here? You probably also want to mention about the configure option. src/slave/flags.hpp <https://reviews.apache.org/r/21594/#comment81264> s/per_container_epehemeral_port_size/epehemeral_ports_per_container/ ? src/slave/flags.hpp <https://reviews.apache.org/r/21594/#comment81265> maybe also explicitly mention that these are not exposed to master/frameworks. src/slave/flags.hpp <https://reviews.apache.org/r/21594/#comment81266> s/type/type of/ src/slave/flags.hpp <https://reviews.apache.org/r/21594/#comment81267> s/ports/ephemeral_ports/ to be more explicit? - Vinod Kone On June 18, 2014, 1:16 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21594/ > ----------------------------------------------------------- > > (Updated June 18, 2014, 1:16 a.m.) > > > Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang. > > > Bugs: https://issues.apache.org/jira/browse/MESOS-1324 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-1324 > > > Repository: mesos-git > > > Description > ------- > > Added a network isolator using port-range based traffic redirection on Linux. > > - Containers are assigned non-ephemeral ports by the scheduler and ephemeral > ports by the network isolator. > - Virtual ethernet devices and Traffic Control filters are set up so that > network traffic in and out of the containers is isolated based on the ports > assigned to them. > - Containers run inside their own network namespaces with separate network > stacks, from which per-container network statistics can be retrieved. > > A joint work with: > - Cong Wang (cw...@twopensource.com) > - Jie Yu (yujie....@gmail.com) > - Ian Downes (ian.dow...@gmail.com) > > > Diffs > ----- > > include/mesos/mesos.proto 2f6be05 > src/Makefile.am b1b7d2d > src/launcher/main.cpp b497e98 > src/slave/constants.hpp c65a62d > src/slave/constants.cpp 51f65bb > src/slave/containerizer/isolators/network/port_mapping.hpp PRE-CREATION > src/slave/containerizer/isolators/network/port_mapping.cpp PRE-CREATION > src/slave/containerizer/linux_launcher.cpp 85c74f0 > src/slave/containerizer/mesos_containerizer.cpp 4d97d49 > src/slave/flags.hpp 3b8ba08 > src/slave/main.cpp 8c2b70c > src/tests/environment.cpp 005fc54 > src/tests/mesos.cpp f197da6 > > Diff: https://reviews.apache.org/r/21594/diff/ > > > Testing > ------- > > make check on linux. more test cases are being written. > > > Thanks, > > Chi Zhang > >
