mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhang...@cn.ibm.com>
Subject Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.
Date Tue, 22 Mar 2016 14:13:43 GMT


> On March 21, 2016, 3:08 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, lines 32-56
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307514#file1307514line32>
> >
> >     Can we introduce paths.hpp|cpp under cni/ directory  for the canonical locations.
> >     
> >     ```
> >     constexpr char ROOT_DIR[] = "...";
> >     
> >     string cni::paths::getNamespaceHandle(
> >         const string& rootDir,
> >         const ContainerID& containerId);
> >         
> >     string cni::paths::getNetworkPath(
> >         const string& rootDir,
> >         const ContainerID& containerId,
> >         const string& name);
> >     
> >     string cni::paths::getIPv4Path(
> >         const string& rootDir,
> >         const ContainerID& containerId,
> >         const string& name,
> >         const string& ifname);
> >         
> >     string cni::paths::getIPv6Path(
> >         const string& rootDir,
> >         const ContainerID& containerId,
> >         const string& name,
> >         const string& ifname);
> >     ```

Thanks for the comments, I have two questions:
1. Do you mean we define rootDir as a const string like this: `constexpr char ROOT_DIR[] =
"/var/run/mesos/isolators/network/cni/"`? If so, I'd like to know why we still need `rootDir`
as the first parameter of those methods (e.g., `getNamespaceHandle`), I think we should be
able to directly use that const string inside of those methods rather than passing it as a
parameter, right?
2. Take `getNamespaceHandle` as an example, so it will be like:
    string getNamespaceHandle(const ContainerID& containerId)
    {
      return path::join(ROOT_DIR, containerId);
    }
I think each of those `getXXX` method will be just a call to `path::join()`, so what is its
advantage against directly calling `path::join()` in cni.cpp?


> On March 21, 2016, 3:08 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 206-212
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307515#file1307515line206>
> >
> >     I suggest we save a rootDir in the isolator process. We can easily switch to
use a flag later. Also, we need to call 'realpath' here to make sure it's a realpath.
> >     
> >     We also need to make sure ROOT_DIR is a self bind mounted directory (slave+shared)
so that namespace bind mount does not leak into containers.

Do you mean we call `realpath()` to get the real path of the const string `ROOT_DIR` first
and then call `mkdir` with the real path as its parameter to create the directory?

And can you please elaborate why the namespace bind mount can be leaked into containers if
we do not make `ROOT_DIR` as a self bind mounted directory? I just want to know the rationale
behind it :-)


- Qian


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


On March 21, 2016, 12:27 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> -----------------------------------------------------------
> 
> (Updated March 21, 2016, 12:27 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
>     https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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