> On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.hpp, line 79 > > <https://reviews.apache.org/r/21594/diff/2/?file=607517#file607517line79> > > > > Why not statically allocate the ranges at construction and then you > > allocate a range which is unused, tagging it as such? This seems vastly > > simpler than calculating ranges at each request!
We thought about static allocation too from the very beginning. The main problem with static allocation is that it cannot handle ephemeral-ports-per-container size change easily. For example, when slave restarts, it uses a different 'max_containers' flag (which leads to different ephemeral-ports-per-container size). When recovering existing running containers, how the static allocator deals with mixed container sizes? > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.hpp, line 66 > > <https://reviews.apache.org/r/21594/diff/2/?file=607517#file607517line66> > > > > Why is there allocate(), and use(), and used in the constructor? Can > > this not be simplified? > > > > Shouldn't the interface simply be to get a range and then to release it > > when finished? > > > > Is use() necessary for recovery? Yes, use() is necessary for recovery. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.hpp, line 54 > > <https://reviews.apache.org/r/21594/diff/2/?file=607517#file607517line54> > > > > What about constructing with the total range to use and then some > > separate recovery which specifies the ranges already in use. It's tricky because allocator is created during "create" before 'recover' is done. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 918 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line918> > > > > Does this require recent kernel patches? Yes, it does. If it fails, the slave will exit cleanly (like a check). > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 954 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line954> > > > > call variable createQdisc and reuse? I would prefer a more explicit variable name than reusing the variable. What's the benefit of reusing the variable? I don't think register pressure is a problem:) > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1394 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1394> > > > > name create and reuse? Again, I prefer more explicit variable names:) > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1722 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1722> > > > > why use the temporary? > > > > if (stat.get().contains("rx_packets")) { > > result.set_net_rx_packets(stat.get()["rx_packets"]); > > } stat.get()["rx_packets"] This won't compile as '[]' is not a const method. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1854 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1854> > > > > Rename to remove and reuse for icmp and arp? Ditto. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1887 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1887> > > > > rename to update and use for icmp and arp? Ditto. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1953 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1953> > > > > Again, why not just use XXX.address() - it seems much, much cleaner > > than introducing XXXNoPrefix locals? The routing lib interfaces expect an net::IP. We have a reason because we want to support subnet in the routing lib in the near future. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/flags.hpp, line 235 > > <https://reviews.apache.org/r/21594/diff/2/?file=607521#file607521line235> > > > > It doesn't feel correct to specify the maximum number of containers > > here. What about specifying the size of the ephemeral range, i.e., number > > of ephemeral ports per container? The reason for 'max_container' is that it is independent of the ephemeral ports specified by the slave. For example, if we use the size of the ephemeral port range for each container (say 1024), what if the slave says that the ephemeral ports available is less than 1024? - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21594/#review45513 ----------------------------------------------------------- On June 12, 2014, 6:45 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21594/ > ----------------------------------------------------------- > > (Updated June 12, 2014, 6:45 p.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 ([email protected]) > - Jie Yu ([email protected]) > - Ian Downes ([email protected]) > > > Diffs > ----- > > include/mesos/mesos.proto 102289b > src/Makefile.am c91b438 > src/launcher/main.cpp b497e98 > src/slave/constants.hpp ace4590 > 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 c17724b > src/slave/containerizer/mesos_containerizer.cpp a38564f > src/slave/flags.hpp 15e5b64 > src/slave/main.cpp 8c2b70c > src/tests/environment.cpp 005fc54 > src/tests/mesos.cpp e6d807c > > Diff: https://reviews.apache.org/r/21594/diff/ > > > Testing > ------- > > make check on linux. more test cases are being written. > > > Thanks, > > Chi Zhang > >
