----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55790/#review162760 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 989) <https://reviews.apache.org/r/55790/#comment234106> Not yours but could you fix? s/host/host's src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 997) <https://reviews.apache.org/r/55790/#comment234300> Can we keep the `LOG(INFO)` dumping the contents of `resolv.conf`? src/slave/containerizer/mesos/isolators/network/cni/spec.hpp (line 50) <https://reviews.apache.org/r/55790/#comment234304> Maybe a comment explaining what this helper function does? Generates a formatted string representation of `DNS` that can be written into `/etc/resolv.conf`. src/tests/containerizer/cni_isolator_tests.cpp (line 937) <https://reviews.apache.org/r/55790/#comment234105> We should have a test where we can setup the returned DNS entries into the `/etc/resolv.conf` of a container. We can do this by re-writing the `mockConfi` plugin to return a bunch of DNS resolvers along with some search domain names and having a script that tests if the `/etc/resolv.conf` in the containers namespace has been setup correctly and trying to ping an external server using maybe a google.com resolver. src/tests/containerizer/cni_isolator_tests.cpp (line 939) <https://reviews.apache.org/r/55790/#comment234102> We generally define namespaces at the veryt op. maybe do ``` namespace spec = mesos::internal::slave::cni::spec; ``` src/tests/containerizer/cni_isolator_tests.cpp (line 941) <https://reviews.apache.org/r/55790/#comment234103> Based on the above comment s/DNS/spec::DNS src/tests/containerizer/cni_isolator_tests.cpp (line 945) <https://reviews.apache.org/r/55790/#comment234104> why do a `dns.Clear()`? src/tests/containerizer/cni_isolator_tests.cpp (line 950) <https://reviews.apache.org/r/55790/#comment234305> nameservers should be IP addresses? Otherwise it becomes a chicken and egg problem. src/tests/containerizer/cni_isolator_tests.cpp (line 953) <https://reviews.apache.org/r/55790/#comment234306> can we put these in separate lines? Would be more readable. - Avinash sridharan On Jan. 20, 2017, 10:45 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55790/ > ----------------------------------------------------------- > > (Updated Jan. 20, 2017, 10:45 p.m.) > > > Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-6858 > https://issues.apache.org/jira/browse/MESOS-6858 > > > Repository: mesos > > > Description > ------- > > Add support for the full set of DNS resolver configuration items that > a CNI IPAM plugin can specify. This implements updating the container's > resolv.conf with the 'domain', 'search', and 'options' keywords. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > e1cac954848e618a03ddb82fd6d040ae1d948e82 > src/slave/containerizer/mesos/isolators/network/cni/spec.hpp > ccd511ec14810dcc1020dec5e1641141f3a319b4 > src/slave/containerizer/mesos/isolators/network/cni/spec.cpp > ac48159dadcea422f605e723db94a7f3bb573fa2 > src/tests/containerizer/cni_isolator_tests.cpp > cb893d3ef005a9cc60c40768fa669b27c4863020 > > Diff: https://reviews.apache.org/r/55790/diff/ > > > Testing > ------- > > sudo make check (Fedora 25) > > > Thanks, > > James Peach > >
