----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55790/#review166879 -----------------------------------------------------------
src/tests/containerizer/cni_isolator_tests.cpp (line 116) <https://reviews.apache.org/r/55790/#comment238956> s/failed/Failed src/tests/containerizer/cni_isolator_tests.cpp (line 124) <https://reviews.apache.org/r/55790/#comment238957> s/failed/Failed src/tests/containerizer/cni_isolator_tests.cpp (line 133) <https://reviews.apache.org/r/55790/#comment238958> s/failed/Failed src/tests/containerizer/cni_isolator_tests.cpp (line 149) <https://reviews.apache.org/r/55790/#comment238970> Leave one line after a multi-line statement. src/tests/containerizer/cni_isolator_tests.cpp (line 157) <https://reviews.apache.org/r/55790/#comment238971> Leave one line after a multi-line statement. src/tests/containerizer/cni_isolator_tests.cpp (line 161) <https://reviews.apache.org/r/55790/#comment238972> I like the `setupMockCNIPlugin` helper, but since we are going to have only one `__MESOS_TEST__` network maybe we could have either a separate helper for `setupMockCNIConfi` or just call this particular piece of code during `Setup`. The CNI config needs to be written only once, even if we invoke the plugin at different times. src/tests/containerizer/cni_isolator_tests.cpp (line 979) <https://reviews.apache.org/r/55790/#comment238976> A suggestion. This test makes sense, but in order to really test that the `resolv.conf` format is correct, would it make sense to set the resolvers to 1.1.1.1, 8.8.8.8 and 8.8.4.4 and domain to apache.org and then do a ping on `mesos`.The ping should succeed. The first nameserver would ideally fail, and the second or third nameservers should kick in. Maybe add another test case just for this? You will need to add the INTERNET tag to the test. src/tests/containerizer/cni_isolator_tests.cpp (lines 1056 - 1065) <https://reviews.apache.org/r/55790/#comment238974> The alignment here seems off? src/tests/containerizer/cni_isolator_tests.cpp (line 1068) <https://reviews.apache.org/r/55790/#comment238979> This should fit in a single line? src/tests/containerizer/cni_isolator_tests.cpp (line 1074) <https://reviews.apache.org/r/55790/#comment238978> Any point in setting the `hostname` ? We are not testing it further down? - Avinash sridharan On Jan. 27, 2017, 1:27 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55790/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2017, 1:27 a.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 > 84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 > 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 > >
