----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65987/#review198968 -----------------------------------------------------------
Looking good! Is it possible to add some test? src/slave/containerizer/mesos/isolators/network/cni/cni.hpp Lines 114 (patched) <https://reviews.apache.org/r/65987/#comment279262> nits: `joinsParentsNetwork` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Line 389 (original), 389-397 (patched) <https://reviews.apache.org/r/65987/#comment279263> Instead of relying on an additional checkpoint file, I think we can infer if a nested container joins its parent container's network or not by first listing all entries in `rootDir`, and then recover those known containers, and then cleanup unknown orphans. Something like the following: ``` // Build ContainerID -> ContainerState mapping // List rootDir // For each entry, find the corresponding ContainerState state (state is optional) // Call _recover(containerId, state), if containerId is nested, set joinsParentsNetwork to false. // Cleanup unknown orphans (not in container state mapping or orphans map) ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 576-579 (original), 585-588 (patched) <https://reviews.apache.org/r/65987/#comment279257> These should be `const bool` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 577-578 (original), 586-587 (patched) <https://reviews.apache.org/r/65987/#comment279246> style nits. We typically do the following ``` bool isDebugContainer = containerConfig.container_class() == ContainerClass::DEBUG; ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 589-590 (patched) <https://reviews.apache.org/r/65987/#comment279247> style nits: we typically align like this: ``` bool joinParentsNetwork = !containerConfig.has_container_info() || containerConfig.container_info().network_infos().empty(); ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 613 (patched) <https://reviews.apache.org/r/65987/#comment279248> I'd add a comment here: ``` // This is either a top level container, or a nested container // joining separate network than its parent. ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Line 581 (original), 613-614 (patched) <https://reviews.apache.org/r/65987/#comment279249> It's possible that `containerConfig.has_container_inf() == false` here (top level container joining host network without container image). We typically avoid depending on the default value of `ContainerInfo`. So I'd suggest we do: ``` if (containerCOnfig.has_container_info()) { const ContainerInfo& containerInfo = containerConfig.container_info(); if (containerInfo.type() != ContainerInfo::MESOS) { return Failure("Can only prepare CNI networks for a MESOS container"); } ... } ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Line 683 (original), 687 (patched) <https://reviews.apache.org/r/65987/#comment279255> Not yours, let's reduce nesting by remove this `else` here given that we already shortcircuit above `return None()` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 696-698 (original), 700-702 (patched) <https://reviews.apache.org/r/65987/#comment279254> Debug container is always nested. So I'd adjust the logic here: ``` if (isDebugContainer) { CHECK(isNestedContainer); // Nested DEBUG containers never need a new MOUNT namespace, so // we don't maintain information about them in the `infos` map. } ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 770 (patched) <https://reviews.apache.org/r/65987/#comment279256> hum, this looks problematic. We don't create `Info` struct for debug containers. Also, should we also update that field for the case where `if (containerNetworks.empty()) {` above (just to be consistent)? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1269-1285 (patched) <https://reviews.apache.org/r/65987/#comment279258> I don't think we need to checkpoint this additional information. You should be able to infer if a nested container has its own network namespace or not by checking if there's a container directory under `cni::paths::ROOT_DIR`. See src/slave/containerizer/mesos/isolators/network/cni/paths.hpp src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Line 1441 (original) <https://reviews.apache.org/r/65987/#comment279259> Why this change? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1488 (patched) <https://reviews.apache.org/r/65987/#comment279260> What about top level containers? I think `joinParentsNetwork` should be orthogonol to whether it is nested or not. Top level container can join its parent's network (agent) too. If you think about that way, here we should do ``` if (isNestedContainer && joinsParentsNetwork) ``` - Jie Yu On March 8, 2018, 5:07 p.m., Sagar Patwardhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65987/ > ----------------------------------------------------------- > > (Updated March 8, 2018, 5:07 p.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-8534 > https://issues.apache.org/jira/browse/MESOS-8534 > > > Repository: mesos > > > Description > ------- > > Continued from https://github.com/apache/mesos/pull/263 > > > Diffs > ----- > > src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp > 1d01915c2db66e54ed68a3dbaa12ea061ca5f6b2 > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > 896656987012b3ffe5008ce6873c9a5249c058de > src/slave/containerizer/mesos/isolators/network/cni/paths.hpp > 7678a7c81c3cdb27410c1f066021eb34bd02a83f > src/slave/containerizer/mesos/isolators/network/cni/paths.cpp > f9056c90f1075cb19c4f554e7d5b629561d06035 > > > Diff: https://reviews.apache.org/r/65987/diff/1/ > > > Testing > ------- > > Manually tested. > > Working on unit tests. > > > Thanks, > > Sagar Patwardhan > >
