----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32426/#review77780 -----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126269> Is it worth differentiating the case where there is some unexpected error vs when it's a dangling symlink? i.e., return Result<pid_t> instead of Try<pid_it>? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126262> Is this a TODO or a NOTE? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126271> s/handles/handles, i.e., handles which doesn't have symlinks/ ? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126267> Include the 'path' in the failure because getContainerIdFromSymlink doesn't always include the 'path' in its returned failure. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126270> If this returns a Result as commented above, we could do a CHECK(!pid.isNone()), because that's the invariant right? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126268> ditto. include pid. src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126272> Don't you also want to remove the network namespace handle itself? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126273> How is this possible? IIUC, if a slave crashes after it removes the 'veth link' but before cleaning up the namespace handle and/or container id simlink, wouldn't that be cleaned up during recovery before a new container is launched? Are you worried about non-crash case where we ignore removal of symlink? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126275> Instead of linkees, linkers and _linkers, how about just using a multihashmap of linkers. ``` // We use a multihashmap here because multiple container ids can map to the same pid when... multihashmap<pid_t, ContainerID> linkers; // Construct the linkers map from bind mount..... for (all files in bind mount) { // Do validation. // If valid entry. linkers.put(pid, containerid); } // If multiple container ids point to same pid, we remove all those symlinks. This is safe because... foreach (pid_t pid, linkers.keys()) { if (linkers.get(pid).size() > 1) { // Remove symlinks etc. // Remove it from the map. linkers.remove(pid); } } ``` src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126276> Delete network namespace handle too? src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment126277> Why is a removal error ok? What does it have to do with backwards compatibility? Is it because os::rm(linker) returns an error if 'linker' doesn't exist? If yes, you probably want to just do the rm inside "if (os::exists(linkers))". src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/32426/#comment125991> s/later/in 0.23.0/ src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/32426/#comment125994> s/mount file/mount file or its symlink/ ? src/tests/port_mapping_tests.cpp <https://reviews.apache.org/r/32426/#comment126278> Any new tests that explicitly verify the fix? - Vinod Kone On March 25, 2015, 6:43 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32426/ > ----------------------------------------------------------- > > (Updated March 25, 2015, 6:43 p.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone. > > > Bugs: MESOS-2528 > https://issues.apache.org/jira/browse/MESOS-2528 > > > Repository: mesos > > > Description > ------- > > Symlink the namespace handle with ContainerID for the port mapping isolator. > > See ticket for details. This patch will allow a smooth upgrade (i.e., rolling > foward and back are both safe). > > > Diffs > ----- > > src/slave/containerizer/isolators/network/port_mapping.hpp > 4dd066a47d43cb1d52f93294d86309151738743e > src/slave/containerizer/isolators/network/port_mapping.cpp > 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd > src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e > > Diff: https://reviews.apache.org/r/32426/diff/ > > > Testing > ------- > > sudo make check > > Many existing tests should already capture the regression. For example, > ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the > container is destroyed. > > Will add a compatibility test tomorrow. > > > Thanks, > > Jie Yu > >
