----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21594/#review45513 -----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment80445> What about constructing with the total range to use and then some separate recovery which specifies the ranges already in use. src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment80403> s/running out of free/exhausting available/ src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment80442> 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? src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment80405> More precisely, doesn't it return true if *any* port in ports is in-use? src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment80443> 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! src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment80406> s/wanna/want to/ src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/21594/#comment80407> s/is not enough/are insufficient/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80408> s/minimal/minimum/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80452> Private IP would normally mean the IP on a private network. This should be LOOPBACK_IP or LOOPBACK_NETWORK. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80411> s/Due to .../The kernel restricts link names to 16 characters or less so we.../ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80413> s/setup/set up/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80416> This should use os::namespaces() from stout/setns. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80417> Include the error code? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80418> Include the error code? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80419> s/not/no/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80420> This does more than check for resources.get().ports.isSome() - it validates the range doesn't it? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80421> s/ / / kill the double whitespace src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80448> Can this be pulled out into stout and tested? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80447> Can this be pulled out into stout and tested? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80449> Can this be pulled out into stout and tested? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80423> Does this require recent kernel patches? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80424> How can setHostLoMac not be SOME, given the lines above - why not remove this? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80425> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80428> call variable createQdisc and reuse? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80429> These direct writes to /proc control files should be abstracted out into helpers. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80430> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80431> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80432> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80433> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80434> Add comment about what it's ignoring here? ethX, lo and other veths? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80450> forkedPid is an Option - how are you sure it's isSome()? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80435> Technically, you're not 'marking' it. What about "Remove the successfully recovered pid" or some such. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80436> s/is/has been/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80437> ditto src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80438> What about "Network isolator recovery complete" to be more inline with other log messages. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80451> s/intersected/intersecting/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80441> merge into one line like 1274. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80453> Single line. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80454> Hasn't this been created during initialization of the isolator? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80455> What about a bind mount abstraction that took source and target and did the validation, touch, etc.? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80456> s/adding/add/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80457> name create and reuse? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80460> Single line. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80458> Single line. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80462> Change to ignore and return Nothing() for unknown containers. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80463> Single line. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80464> Remove this log warning as nothing is really wrong here. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80465> What happens if stat isNone? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80466> why use the temporary? if (stat.get().contains("rx_packets")) { result.set_net_rx_packets(stat.get()["rx_packets"]); } src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80500> ditto and all the others src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80501> Single line. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80510> Can you elaborate on this? What's the problem and how is this used to work around it. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80502> Why do this - why not delete in the caller since it has the pointer? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80505> Include containerId in log message. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80503> But don't the filters get set up before any pid is isolated!? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80509> I think just using hostIP.address() everywhere instead is cleaner? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80506> Include containerId in log message. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80504> Include containerId in log message. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80507> Is there a test to ensure these get added back correctly when a container is launched? i.e., start a container, stop container, start container. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80511> Rename to remove and reuse for icmp and arp? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80512> rename to update and use for icmp and arp? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80513> Do we verify that happens? Do we wait until it's completed before returning from cleanup? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80514> The contract is that the launcher ensures all processes are killed before calling cleanup on the isolators. If there are processes remaining it's a bug in the launcher. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80515> "Successfully cleaned up veth " << veth(pid) " for container " << containerId src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80516> s/logics/logic/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80517> Again, why not just use XXX.address() - it seems much, much cleaner than introducing XXXNoPrefix locals? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80518> Comment that this will be done by a FilesystemIsolator in the future. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80519> s/Setup/Set up/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80520> s/setup/set up/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80521> s/exits/exists/ src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80522> This is checking the host proc for existence - wouldn't it be better to check the container's proc instead? if [ -e /proc/sys/... ]; then ...; fi src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/21594/#comment80528> As mentioned above, why not pre-compute all of these at initialization rather than at each call? src/slave/containerizer/linux_launcher.cpp <https://reviews.apache.org/r/21594/#comment80402> Isn't this now defined in stout/setns? src/slave/flags.hpp <https://reviews.apache.org/r/21594/#comment80422> 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? src/slave/main.cpp <https://reviews.apache.org/r/21594/#comment80367> s/Setup/Set up/ src/tests/environment.cpp <https://reviews.apache.org/r/21594/#comment80529> either s/, network/, the network/ or s/isolator/isolation/ src/tests/mesos.cpp <https://reviews.apache.org/r/21594/#comment80530> This will include the network isolator in all tests if it was compiled in? Perhaps we should start with selectively enabling it in tests? - Ian Downes On June 12, 2014, 11:45 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21594/ > ----------------------------------------------------------- > > (Updated June 12, 2014, 11:45 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 ([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 > >
