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 2016/04/09 00:48:06 UTC
Review Request 45956: Added the `_isolate` method.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Qian Zhang.
Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922
Repository: mesos
Description
-------
Once the `isolate` is successful, the `_isolate` method calls out the
`mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
/etc/resolv.conf for the container.
Diffs
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
Diff: https://reviews.apache.org/r/45956/diff/
Testing
-------
make
*Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
Thanks,
Avinash sridharan
Re: Review Request 45956: Added the `_isolate` method.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128015
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45953, 45954, 45955, 45956]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 9, 2016, 5:48 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 5:48 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128809
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On April 14, 2016, midnight, Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 14, 2016, midnight)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 3f64bc2d5e114da63a76d93b6406e70891dd0242
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ac01abc29f0e1a145516d8ec7d373c6cedb33b38
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/
-----------------------------------------------------------
(Updated April 14, 2016, midnight)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Adding back the code to set hostname in /etc/hosts.
Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922
Repository: mesos
Description
-------
Once the `isolate` is successful, the `_isolate` method calls out the
`mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
/etc/resolv.conf for the container.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 3f64bc2d5e114da63a76d93b6406e70891dd0242
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ac01abc29f0e1a145516d8ec7d373c6cedb33b38
Diff: https://reviews.apache.org/r/45956/diff/
Testing
-------
make
*Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
Thanks,
Avinash sridharan
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/
-----------------------------------------------------------
(Updated April 13, 2016, 7:18 a.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Fixed Jie's comments.
Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922
Repository: mesos
Description
-------
Once the `isolate` is successful, the `_isolate` method calls out the
`mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
/etc/resolv.conf for the container.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 3f64bc2d5e114da63a76d93b6406e70891dd0242
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ac01abc29f0e1a145516d8ec7d373c6cedb33b38
Diff: https://reviews.apache.org/r/45956/diff/
Testing
-------
make
*Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
Thanks,
Avinash sridharan
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/
-----------------------------------------------------------
(Updated April 13, 2016, 5:22 a.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Rebased.
Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922
Repository: mesos
Description
-------
Once the `isolate` is successful, the `_isolate` method calls out the
`mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
/etc/resolv.conf for the container.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 3f64bc2d5e114da63a76d93b6406e70891dd0242
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ac01abc29f0e1a145516d8ec7d373c6cedb33b38
Diff: https://reviews.apache.org/r/45956/diff/
Testing
-------
make
*Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
Thanks,
Avinash sridharan
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On April 13, 2016, 5:16 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 716-717
> > <https://reviews.apache.org/r/45956/diff/4/?file=1342332#file1342332line716>
> >
> > What if IP is not found? Should we use 127.0.0.1?
If the container does not have any IP the hostname should resolve to 169.254.x.y (which is a self assigned IP address). In fact I gave this a bit more thought, and realized that we don't need to add and explicit resolution for hostname in /etc/hosts file. If the hostname is not present in /etc/hosts file the kernel will automatically pick the first, non-loopback IP, as the resolution for the hostname, so explicitly setting the hostname to a an IP in /etc/hosts doesn't help. I verified this on my Ubuntu 14.04 VM, hostnames are not stored in /etc/hosts. According have gone ahead and removed the code to pick an IP for the hostname and adding it to /etc/hosts.
We still need the /etc/hosts for resolving `localhost`.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128609
-----------------------------------------------------------
On April 13, 2016, 7:18 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 7:18 a.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 3f64bc2d5e114da63a76d93b6406e70891dd0242
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ac01abc29f0e1a145516d8ec7d373c6cedb33b38
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Jie Yu <yu...@gmail.com>.
> On April 13, 2016, 5:16 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 716-717
> > <https://reviews.apache.org/r/45956/diff/4/?file=1342332#file1342332line716>
> >
> > What if IP is not found? Should we use 127.0.0.1?
>
> Avinash sridharan wrote:
> If the container does not have any IP the hostname should resolve to 169.254.x.y (which is a self assigned IP address). In fact I gave this a bit more thought, and realized that we don't need to add and explicit resolution for hostname in /etc/hosts file. If the hostname is not present in /etc/hosts file the kernel will automatically pick the first, non-loopback IP, as the resolution for the hostname, so explicitly setting the hostname to a an IP in /etc/hosts doesn't help. I verified this on my Ubuntu 14.04 VM, hostnames are not stored in /etc/hosts. According have gone ahead and removed the code to pick an IP for the hostname and adding it to /etc/hosts.
>
> We still need the /etc/hosts for resolving `localhost`.
Sorry, I got confused, what happens if I do `ping <containerId>` in the container without setting up /etc/hosts?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128609
-----------------------------------------------------------
On April 13, 2016, 7:18 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 7:18 a.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 3f64bc2d5e114da63a76d93b6406e70891dd0242
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ac01abc29f0e1a145516d8ec7d373c6cedb33b38
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On April 13, 2016, 5:16 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 716-717
> > <https://reviews.apache.org/r/45956/diff/4/?file=1342332#file1342332line716>
> >
> > What if IP is not found? Should we use 127.0.0.1?
>
> Avinash sridharan wrote:
> If the container does not have any IP the hostname should resolve to 169.254.x.y (which is a self assigned IP address). In fact I gave this a bit more thought, and realized that we don't need to add and explicit resolution for hostname in /etc/hosts file. If the hostname is not present in /etc/hosts file the kernel will automatically pick the first, non-loopback IP, as the resolution for the hostname, so explicitly setting the hostname to a an IP in /etc/hosts doesn't help. I verified this on my Ubuntu 14.04 VM, hostnames are not stored in /etc/hosts. According have gone ahead and removed the code to pick an IP for the hostname and adding it to /etc/hosts.
>
> We still need the /etc/hosts for resolving `localhost`.
>
> Jie Yu wrote:
> Sorry, I got confused, what happens if I do `ping <containerId>` in the container without setting up /etc/hosts?
Looks like you are right, the behavior seems to be dependent on how nsswtich.conf is setup. It seems to be safer to leave the hostname in /etc/hosts. Bought back the loop that adds the hostname to /etc/hosts.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128609
-----------------------------------------------------------
On April 13, 2016, 7:18 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 7:18 a.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 3f64bc2d5e114da63a76d93b6406e70891dd0242
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ac01abc29f0e1a145516d8ec7d373c6cedb33b38
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128609
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (lines 108 - 109)
<https://reviews.apache.org/r/45956/#comment192060>
can you follow style guide here?
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 123)
<https://reviews.apache.org/r/45956/#comment192061>
`const Flags& _flags`
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 128)
<https://reviews.apache.org/r/45956/#comment192062>
move up. Please following the original style.
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 170)
<https://reviews.apache.org/r/45956/#comment192063>
const Flags flags;
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 80 - 81)
<https://reviews.apache.org/r/45956/#comment192064>
```
new NetworkCniIsolatorProcess(
flags,
hashmap<string, NetworkConfigInfo>())));
```
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 568 - 571)
<https://reviews.apache.org/r/45956/#comment192066>
```
Owned<info> info(new Info(
containerNetworks,
containerConfig.rootfs()));
infos.put(containerId, info);
```
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 661 - 673)
<https://reviews.apache.org/r/45956/#comment192067>
Can you move that to `_isolate` as well?
```
return await(futures)
.then(defer(
self(),
&Self::_isolate,
containerId,
pid,
const list<Future<Nothing>>& attaches));
```
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 675)
<https://reviews.apache.org/r/45956/#comment192068>
See my above comments.
Also, capture `this` pointer is in general bad. If this isolator terminates (e.g., during teardown in tests) while the above future is still pending, when the future becomes ready, it'll cause a segfault.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 689 - 693)
<https://reviews.apache.org/r/45956/#comment192070>
This should be a CHECK instead?
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 702)
<https://reviews.apache.org/r/45956/#comment192071>
Kill this line.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 706)
<https://reviews.apache.org/r/45956/#comment192072>
No need to print containerId in the error message here since the caller will print it anyway.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 709)
<https://reviews.apache.org/r/45956/#comment192073>
Add a TODO saying that we only support ipv4 for now
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 714)
<https://reviews.apache.org/r/45956/#comment192074>
Please align the parameters.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 716 - 717)
<https://reviews.apache.org/r/45956/#comment192077>
What if IP is not found? Should we use 127.0.0.1?
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 722 - 725)
<https://reviews.apache.org/r/45956/#comment192075>
```
Try<net::IPNetwork> ipNetwork = net::IPNetwork::parse(
network.cniNetworkInfo->ip4().ip(),
AF_INET);
if (ipNetwork.isError()) {
return Failure("");
}
hosts << ipNetwork->address() << " " << containerId << endl;
```
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 732 - 733)
<https://reviews.apache.org/r/45956/#comment192076>
This fits in one line?
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 750 - 751)
<https://reviews.apache.org/r/45956/#comment192078>
DItto on alignment.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 752 - 761)
<https://reviews.apache.org/r/45956/#comment192079>
I probably do
```
if (network.cniNetworkInfo.isNone() ||
!network.cniNetworkInfo->has_dns()) {
continue;
}
foreach (const string& nameserver,
network.cniNetwork->dns().nameservers()) {
resolv << "nameserver " << nameserver << endl;
}
```
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 754)
<https://reviews.apache.org/r/45956/#comment192080>
THis is not needed.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 771 - 773)
<https://reviews.apache.org/r/45956/#comment192081>
Please align `<<`. See other files in the code base.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 776)
<https://reviews.apache.org/r/45956/#comment192082>
Ditto. Fix all such cases.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 788 - 797)
<https://reviews.apache.org/r/45956/#comment192083>
I think subprocess supports 'Flags' directly. Please see how to use that in port mapping isolator.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 807 - 808)
<https://reviews.apache.org/r/45956/#comment192084>
Fix the style here.
- Jie Yu
On April 13, 2016, 4:40 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 4:40 a.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/
-----------------------------------------------------------
(Updated April 13, 2016, 4:40 a.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Fixed missing quotes in `Failure`.
Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922
Repository: mesos
Description
-------
Once the `isolate` is successful, the `_isolate` method calls out the
`mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
/etc/resolv.conf for the container.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
Diff: https://reviews.apache.org/r/45956/diff/
Testing
-------
make
*Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
Thanks,
Avinash sridharan
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/
-----------------------------------------------------------
(Updated April 12, 2016, 2:13 a.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922
Repository: mesos
Description
-------
Once the `isolate` is successful, the `_isolate` method calls out the
`mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
/etc/resolv.conf for the container.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
Diff: https://reviews.apache.org/r/45956/diff/
Testing
-------
make
*Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
Thanks,
Avinash sridharan
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On April 11, 2016, 7:10 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 762
> > <https://reviews.apache.org/r/45956/diff/2/?file=1338338#file1338338line762>
> >
> > I don't think 'realpath' is necessary here.
Will remove `realpath`.
What should be the behavior in case "/etc/resolv.conf" does not exist ? Should we create a `/etc/resolv.conf` or just throw a `Failure?
> On April 11, 2016, 7:10 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 797-823
> > <https://reviews.apache.org/r/45956/diff/2/?file=1338338#file1338338line797>
> >
> > Please follow style in 'attach' to monitor the subprocess outputs.
Did you mean convert the Lambda into a function (similar to `_attach`) ?
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128211
-----------------------------------------------------------
On April 9, 2016, 5:48 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 5:48 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review128211
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 674)
<https://reviews.apache.org/r/45956/#comment191601>
.then in the next line.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 717 - 720)
<https://reviews.apache.org/r/45956/#comment191602>
Can you use stout IPNetwork::parse first and get IP using 'IPNetwork::address()'
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 762)
<https://reviews.apache.org/r/45956/#comment191603>
I don't think 'realpath' is necessary here.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 797 - 823)
<https://reviews.apache.org/r/45956/#comment191604>
Please follow style in 'attach' to monitor the subprocess outputs.
- Jie Yu
On April 9, 2016, 5:48 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 5:48 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 45956: Added the `_isolate` method.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/
-----------------------------------------------------------
(Updated April 9, 2016, 5:48 p.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Using realpath of `/etc/resolv.conf`
Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922
Repository: mesos
Description
-------
Once the `isolate` is successful, the `_isolate` method calls out the
`mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
/etc/resolv.conf for the container.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
Diff: https://reviews.apache.org/r/45956/diff/
Testing
-------
make
*Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
Thanks,
Avinash sridharan
Re: Review Request 45956: Added the `_isolate` method.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45956/#review127936
-----------------------------------------------------------
Patch looks great!
Reviews applied: [45953, 45954, 45955, 45956]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 8, 2016, 10:48 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45956/
> -----------------------------------------------------------
>
> (Updated April 8, 2016, 10:48 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once the `isolate` is successful, the `_isolate` method calls out the
> `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and
> /etc/resolv.conf for the container.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 654137c552a7c416f394365e43ea80770fe1ef8d
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 159152a01b68a667dbd57fa6452c6c2a3422787c
>
> Diff: https://reviews.apache.org/r/45956/diff/
>
>
> Testing
> -------
>
> make
>
> *Ran mesos_execute with single master/slave setup to verify that containers get the right hostname and network files when attached to a CNI network.
>
>
> Thanks,
>
> Avinash sridharan
>
>