----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65987/#review199274 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/mesos/isolators/network/cni/cni.hpp Lines 114 (patched) <https://reviews.apache.org/r/65987/#comment279568> I'd make this `const` and not set a default value. This will force the caller to set a value (reducing surprises) src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 409-410 (original), 398-399 (patched) <https://reviews.apache.org/r/65987/#comment279589> This is not correct. The entries here might be nested container. We need to introduce a parse method for ContainerID src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 578 (patched) <https://reviews.apache.org/r/65987/#comment279570> This should be: ``` const bool joinsParentsNetwork = isDebugContainer || !containerConfig.has_container_info() || containerConfig.container_info().network_infos().empty(); ``` This is because for debug containers, it joins parent network, but does not specify container_info or network_info src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 582 (patched) <https://reviews.apache.org/r/65987/#comment279571> Given above, this can be simplified to: ``` if (isNestedContainer && joinsParentsNetwork) { .. } ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 585-588 (patched) <https://reviews.apache.org/r/65987/#comment279572> This comment is no longer accurate. We only checkpoint containers that join non-host networks. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 763 (patched) <https://reviews.apache.org/r/65987/#comment279574> I'd move this up to where we created the Info struct. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 962-970 (patched) <https://reviews.apache.org/r/65987/#comment279583> we also allow users to override the hostname. as a result, we should only do that if `info->hostname.isNone()` This can simply be: ``` string hostname = info->hostname.isSome() ? info->hostname.get() : containerId.value(); ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Line 1155 (original), 1164 (patched) <https://reviews.apache.org/r/65987/#comment279576> I'd rather let `getInterfaceDir` to take `ContainerID` as the parameter. Ditto else where src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1400 (patched) <https://reviews.apache.org/r/65987/#comment279579> typo `network` - Jie Yu On March 15, 2018, 4:30 a.m., Sagar Patwardhan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65987/ > ----------------------------------------------------------- > > (Updated March 15, 2018, 4:30 a.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 f0b86775b > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 1d01915c2 > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 896656987 > > > Diff: https://reviews.apache.org/r/65987/diff/5/ > > > Testing > ------- > > Manually tested. > > Working on unit tests. > > > Thanks, > > Sagar Patwardhan > >
