----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45571/#review126592 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 371) <https://reviews.apache.org/r/45571/#comment189602> s/on/only instead of `non-host network(s)` maybe `joined a CNI network(s) ? Didn't follow the last part of the sentence "and cleanup _might_ be required for that container." What does this mean ? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 379) <https://reviews.apache.org/r/45571/#comment189608> Another case where the container directory might not exist is if the container was launched on the host network? We are doing the right thing here by just returning `Nothing` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 421) <https://reviews.apache.org/r/45571/#comment189611> Maybe "Currently a container can have only one interface attached to a CNI network." src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 424) <https://reviews.apache.org/r/45571/#comment189612> You had mentioned that if we return an error during `recover` the agent will restart. This state from checkpointed information that is not changing. After restart would the Agent end up hitting this condition again ? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 470) <https://reviews.apache.org/r/45571/#comment189613> s/crashes/crashed/ Why would cleanup be called on this container? Cause they are `orphans`? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 803) <https://reviews.apache.org/r/45571/#comment189614> We should add a comment here explaining why we don't throw a `Failure` as with other isolators. "It is possible that this container is on the host network instead of a CNI network. We only store information for containers that have joined a CNI network." src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 976) <https://reviews.apache.org/r/45571/#comment189615> Maybe "failed to detech container " + containerId + " from network '" ..... - Avinash sridharan On April 1, 2016, 1:08 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45571/ > ----------------------------------------------------------- > > (Updated April 1, 2016, 1:08 a.m.) > > > Review request for mesos, Avinash sridharan and Qian Zhang. > > > Bugs: MESOS-4759 > https://issues.apache.org/jira/browse/MESOS-4759 > > > Repository: mesos > > > Description > ------- > > A few cleanups and simplifications in CNI isolator. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp > 3a07540909ed771d1bd3b22888e04d5fb451710d > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > 486c382365d5293cd9d53b8b239f70a543c46792 > > Diff: https://reviews.apache.org/r/45571/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
