mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zhengju Sha <handai....@gmail.com>
Subject Re: Review Request 47633: Isolation/networking: check if IPv6 is loaded before trying to disable it
Date Sat, 21 May 2016 06:19:40 GMT


> On 五月 20, 2016, 5:02 a.m., Cong Wang wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp, line 3927
> > <https://reviews.apache.org/r/47633/diff/1/?file=1388820#file1388820line3927>
> >
> >     You need to move this check into the script rather than in the C++ code. Something
like "test -f /proc/sys/net/ipv6/conf/all/disable_ipv6 && echo 1 > /proc/sys/net/ipv6/conf/all/disable_ipv6".
> 
> Zhengju Sha wrote:
>     Thanks for the reply.
>     I thought this way before, but I notice that we also use os::exists() to check other
file in this function, so I choose to keep in consistent. Both of the manner is okay IMHO,
is there other considerations to move it to bash script?
> 
> haosdent huang wrote:
>     Hmm, keep consistent more reasonable. Because we have
>     
>     ```
>       // Enable route_localnet on lo because by default 127.0.0.1 traffic
>       // is dropped. This feature exists on 3.6 kernel or newer.
>       if (os::exists(path::join("/proc/sys/net/ipv4/conf", lo, "route_localnet"))) {
>         script << "echo 1 > /proc/sys/net/ipv4/conf/" << lo << "/route_localnet\n";
>       }
>     ```
> 
> Cong Wang wrote:
>     Nope, you don't understand it. Ipv6 module could be removed during preparation and
launch, although not likely, but still.
> 
> haosdent huang wrote:
>     Thanks @wangcong's nice explanation, it make sense. My bad to post that before.

Yeah, sound reasonable, your consideration is more thoughtful. 

But speak more, as talking of this kind of risk window, "test -f /proc/sys/net/ipv6/conf/all/disable_ipv6
&& echo 1 > /proc/sys/net/ipv6/conf/all/disable_ipv6" also doesn't avoid the problem
in essence, but narrow the potential probability.

And I also find the following code in PortMappingIsolatorProcess::isolate() which has the
some problem:

  2608   // Disable IPv6 for veth as IPv6 packets won't be forwarded anyway.
  2609   const string disableIPv6 =
  2610     path::join("/proc/sys/net/ipv6/conf", veth(pid), "disable_ipv6");
  2611 
  2612   if (os::exists(disableIPv6)) {
  2613     Try<Nothing> write = os::write(disableIPv6, "1");
  2614     if (write.isError()) {
  2615       return Failure(
  2616           "Failed to disable IPv6 for " + veth(pid) +
  2617           ": " + write.error());
  2618     }
  2619   }
  

As kernel doesn't export this kind of atomic test-set interface, these problems are very common.
Maybe we can design very robust error handling for such cases, but I doubt whether it deserves
high priority and extra energe in present circumstances of mesos community.

So if both of you have no objection, I'll choose Cong's proposal as a middle way in next version.
:p


- Zhengju


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


On 五月 20, 2016, 3:54 a.m., Zhengju Sha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47633/
> -----------------------------------------------------------
> 
> (Updated 五月 20, 2016, 3:54 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, and Cong Wang.
> 
> 
> Bugs: MESOS-5381
>     https://issues.apache.org/jira/browse/MESOS-5381
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Isolation/networking: check if IPv6 is loaded before trying to disable it
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp ad792def2bb3a1614d21ca28d858e400d2e3ede1

> 
> Diff: https://reviews.apache.org/r/47633/diff/
> 
> 
> Testing
> -------
> 
> Enniornment and steps:
> 1. Enable mesos-slave --isolation=network/port_mapping on CentOS7.2 with kernel version:
3.10.0-327.10.1.el7.x86_64
> 2. Create application on marathon framework with commands such as "echo hello" using
MesosContainerizer
> 3. Load IPv6 module by removing "ipv6.disable=1" of GRUB_CMDLINE_LINUX in /etc/default/grub
> 4. Disable IPv6 module by adding "ipv6.disable=1" of GRUB_CMDLINE_LINUX in /etc/default/grub
> 
> Now mesos can run both of the testcases successfully.
> 
> 
> Thanks,
> 
> Zhengju Sha
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message