You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Avinash sridharan <av...@mesosphere.io> on 2017/02/27 17:18:20 UTC
Re: Review Request 55790: Support the full CNI DNS specification.
-----------------------------------------------------------
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
>
>