> 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? > > Jie Yu wrote: > Yes, it does. If it fails, the slave will exit cleanly (like a check).
added a comment to mention the kernel patch. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1073 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1073> > > > > Add comment about what it's ignoring here? ethX, lo and other veths? commented. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1804 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1804> > > > > But don't the filters get set up before any pid is isolated!? when prepare fails for reasons like no more ephemeral ports available, cleanup is called by containerizer, at which point pid isn't isolated yet. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1932 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1932> > > > > 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. I don't think the comment reflects the reason we added this. we saw a pathological case in testing where the veth doesn't get cleaned up quickly by the kernel (a ~10m delay) even when all the processes of a container have been killed. This is a simple work around to just kill it instead of waiting for the kernel. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 756 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line756> > > > > This does more than check for resources.get().ports.isSome() - it > > validates the range doesn't it? could you elaborate on this? > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1803 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1803> > > > > Include containerId in log message. This code path is used to clean up both normal containers and orphaned containers. We don't have a containerId for a recovered orphaned container. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 887 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line887> > > > > Can this be pulled out into stout and tested? I think it's a good idea and could be useful for tests, but here we need the name of the eth0 anyway. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1311 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1311> > > > > Hasn't this been created during initialization of the isolator? removed. Jie, I don't think this is particularly useful. Do you try to detect if someone has removed the directory after 'create'? > 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? > > Jie Yu wrote: > Yes, use() is necessary for recovery. changed use to allocate as well, so we have an overloaded allocate now. changed free to deallocate. adjusted comments. > 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. > > Jie Yu wrote: > It's tricky because allocator is created during "create" before 'recover' > is done. I like the idea of having a constructor with only the 'total' resources for the allocator to manage. having a free and a used is less intuitive. this constructor is called only once in create and 'used' is useless there. having only a 'total' can also simplify test cases. Jie is right about rocover. Let's use the overloaded allocate. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1991 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1991> > > > > 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 at this point, /proc has been remounted, so we are checking container's proc. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/tests/mesos.cpp, line 347 > > <https://reviews.apache.org/r/21594/diff/2/?file=607524#file607524line347> > > > > This will include the network isolator in all tests if it was compiled > > in? Perhaps we should start with selectively enabling it in tests? this only affects ContainerizeTest, which means only SlaveRecoveryTest. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1850 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1850> > > > > 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. yeah, I'll add a test to check 'clean up' is indeed finished in the system after containers exit. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1918 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1918> > > > > Do we verify that happens? Do we wait until it's completed before > > returning from cleanup? I fixed the comments since now we delete veth in cleanup. all the fiters on it go away when veth is deleted. > On June 13, 2014, 6:05 p.m., Ian Downes wrote: > > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1323 > > <https://reviews.apache.org/r/21594/diff/2/?file=607518#file607518line1323> > > > > What about a bind mount abstraction that took source and target and did > > the validation, touch, etc.? not much gain from this, since there is only two calls and there is only one place for them. more, 'touch' does not have a recursive 'touch' semantic. - Chi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21594/#review45513 ----------------------------------------------------------- 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 ([email protected]) > - Jie Yu ([email protected]) > - Ian Downes ([email protected]) > > > 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 > >
