Re: Review Request 55790: Support the full CNI DNS specification.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review169089
---


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 986 (original), 979 (patched)


Can you follow up with a patch to document this?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 999 (patched)


Why do you need this?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 152-154 (patched)


We typically combine these two:
```
Try result = os::mkdir(cniPluginDir);
```



src/tests/containerizer/cni_isolator_tests.cpp
Lines 167-168 (patched)


I would adjust the indentation here:
```
result = os::chmod(
mockPlugin,
S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
```



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1204 (patched)


I would rename this to CniSpecTest.GenerateResolverConfig


- Jie Yu


On March 7, 2017, 5:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated March 7, 2017, 5:01 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 
> 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/5/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[Avinash sridharan]

> On March 7, 2017, 2:22 p.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1112 (patched)
> > 
> >
> > Instead of setting this to the hosts IP address can we set it to the 
> > `loopback` (127.0.0.1). We want the query to this nameserver to fail before 
> > it hits the google servers. 
> > 
> > Just being a bit cautious here in case there is a DNS masquerade 
> > running on the host :).
> 
> James Peach wrote:
> dnsmasq would listen on loopback too, but sure :)

That's true. Unfortunately, __MESOS_TEST__ doesn't create a separate network 
namespace, else this would have made sense.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review168128
---


On March 7, 2017, 5:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated March 7, 2017, 5:01 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 
> 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/5/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

> On March 7, 2017, 2:22 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Lines 999 (patched)
> > 
> >
> > Do you explicitly need this? Even if you do please define this at the 
> > top.

AFAICT this is required here. I wasn't able to find a different permutation 
that would build.


> On March 7, 2017, 2:22 p.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1112 (patched)
> > 
> >
> > Instead of setting this to the hosts IP address can we set it to the 
> > `loopback` (127.0.0.1). We want the query to this nameserver to fail before 
> > it hits the google servers. 
> > 
> > Just being a bit cautious here in case there is a DNS masquerade 
> > running on the host :).

dnsmasq would listen on loopback too, but sure :)


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review168128
---


On March 1, 2017, 4:51 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated March 1, 2017, 4:51 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 
> 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/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/
---

(Updated March 7, 2017, 5:01 p.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


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 (updated)
-

  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/5/

Changes: https://reviews.apache.org/r/55790/diff/4-5/


Testing
---

sudo make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 55790: Support the full CNI DNS specification.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review168128
---


Fix it, then Ship it!




Ship It!


src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 999 (patched)


Do you explicitly need this? Even if you do please define this at the top.



src/tests/containerizer/cni_isolator_tests.cpp
Lines 165 (patched)


s/mockPlugin/`mockPlugin`



src/tests/containerizer/cni_isolator_tests.cpp
Lines 174 (patched)


s/mockPlugin/`mockPlugin`



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1048 (patched)


s/formatResolverConfig()/`formatResolverConfig()`/



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1112 (patched)


Instead of setting this to the hosts IP address can we set it to the 
`loopback` (127.0.0.1). We want the query to this nameserver to fail before it 
hits the google servers. 

Just being a bit cautious here in case there is a DNS masquerade running on 
the host :).


- Avinash sridharan


On March 1, 2017, 4:51 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated March 1, 2017, 4:51 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 
> 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/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review167620
---



Patch looks great!

Reviews applied: [55790]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 1, 2017, 4:51 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated March 1, 2017, 4:51 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 
> 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/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/
---

(Updated March 1, 2017, 4:51 p.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebased and addressed review feedback.


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 (updated)
-

  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/4/

Changes: https://reviews.apache.org/r/55790/diff/3-4/


Testing
---

sudo make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 55790: Support the full CNI DNS specification.

[Avinash sridharan]

---
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)


s/failed/Failed



src/tests/containerizer/cni_isolator_tests.cpp (line 124)


s/failed/Failed



src/tests/containerizer/cni_isolator_tests.cpp (line 133)


s/failed/Failed



src/tests/containerizer/cni_isolator_tests.cpp (line 149)


Leave one line after a multi-line statement.



src/tests/containerizer/cni_isolator_tests.cpp (line 157)


Leave one line after a multi-line statement.



src/tests/containerizer/cni_isolator_tests.cpp (line 161)


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)


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)


The alignment here seems off?



src/tests/containerizer/cni_isolator_tests.cpp (line 1068)


This should fit in a single line?



src/tests/containerizer/cni_isolator_tests.cpp (line 1074)


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
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review163229
---



Patch looks great!

Reviews applied: [55790]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


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
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

---
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.


Changes
---

Address review feedback.


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 (updated)
-

  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

Re: Review Request 55790: Support the full CNI DNS specification.

[Avinash sridharan]

> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1006
> > 
> >
> > Can we keep the `LOG(INFO)` dumping the contents of `resolv.conf`?
> 
> James Peach wrote:
> This seems more usefult for debugging that for operations. Maybe a VLOG? 
> I'm also not sure that nameservers are the what you want to see in this.

Generally nameserver are pretty critical for containers to work (service 
discovery). In practice I have seen containers fail due to bad nameservers 
being setup and its pretty hard to debug those issues, since DNS configuration 
doesn't show up in container logs. Given that there is an explicit nameserver 
being thrown here thought LOG(INFO) would be helpful to the operators to 
understand how container connectivity would work.


> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 950
> > 
> >
> > nameservers should be IP addresses? Otherwise it becomes a chicken and 
> > egg problem.
> 
> James Peach wrote:
> Sure, but there's no enforcement of this in any of the API layers. From 
> the perspective of the tests this is an arbitrary string.

I agree. Was just pointing this out that having a random IP address is more 
conformant, and would be more readable.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review162760
---


On Jan. 26, 2017, 11:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated Jan. 26, 2017, 11:19 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 
> 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
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 945
> > 
> >
> > why do a `dns.Clear()`?

Consistency. Each block is the same and it remove the risk of breakage from 
reordering or adding new checks.


> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 950
> > 
> >
> > nameservers should be IP addresses? Otherwise it becomes a chicken and 
> > egg problem.

Sure, but there's no enforcement of this in any of the API layers. From the 
perspective of the tests this is an arbitrary string.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review162760
---


On Jan. 26, 2017, 11:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated Jan. 26, 2017, 11:19 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 
> 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
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/
---

(Updated Jan. 26, 2017, 11:19 p.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu.


Changes
---

Rebase and address review feedback.


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 (updated)
-

  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

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1006
> > 
> >
> > Can we keep the `LOG(INFO)` dumping the contents of `resolv.conf`?

This seems more usefult for debugging that for operations. Maybe a VLOG? I'm 
also not sure that nameservers are the what you want to see in this.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review162760
---


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
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[Avinash sridharan]

---
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)


Not yours but could you fix?
s/host/host's



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 997)


Can we keep the `LOG(INFO)` dumping the contents of `resolv.conf`?



src/slave/containerizer/mesos/isolators/network/cni/spec.hpp (line 50)


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)


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)


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)


Based on the above comment
s/DNS/spec::DNS



src/tests/containerizer/cni_isolator_tests.cpp (line 945)


why do a `dns.Clear()`?



src/tests/containerizer/cni_isolator_tests.cpp (line 950)


nameservers should be IP addresses? Otherwise it becomes a chicken and egg 
problem.



src/tests/containerizer/cni_isolator_tests.cpp (line 953)


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
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/#review162532
---



Patch looks great!

Reviews applied: [55790]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


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
> 
>

Review Request 55790: Support the full CNI DNS specification.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55790/
---

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