mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.
Date Mon, 26 Sep 2016 23:42:10 GMT


> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > <https://reviews.apache.org/r/52058/diff/1/?file=1502968#file1502968line773>
> >
> >     (You'll want to update this comment.)
> >     
> >     There isn't always a stdout/stderr file in the sandbox, as this depends on the
ContainerLogger module.  Would non-recursively chown-ing the sandbox directory work as well?
 (This is already done when the agent first creates the sandbox.)
> 
> Megha Sharma wrote:
>     The FetcherProcess::run(...) already creates the stdout and stderr and then recursively
changes the ownership of the sandbox to make sure these 2 files have the right ownership which
I modified to change the ownership of just these 2 files exclusively and not the entire sandbox.
So, at this point the stdout and stderr are already created. 
>     Also, like you mentioned the agent is already doing a chown while creating the sandbox
directory so the sandbox is already owned by the desired user. The concern that I am addressing
here is avoiding unnecessary chowning of sandbox so files laid out by other entities don't
change ownership but we still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
>     Right, forgot about the fetcher's own stdout/stderr :)
>     
>     I'd argue it makes sense to not do any chown-ing at all.  The stdout/stderr files
are technically created by other entities than the executor (i.e. the fetcher, agent, and
logger module).  Not sure if this breaks any expectations in the fetcher though...
> 
> Megha Sharma wrote:
>     Since now the fetcher is being run as task user as a result of jira https://issues.apache.org/jira/browse/MESOS-5845
so these 2 files need to be chowned to the same user for the fetcher to be able to write to
them.
> 
> Jiang Yan Xu wrote:
>     @Joseph: Given the way it works today, is the fetcher stdout/stderr handled by the
container logger at all? It doesn't look so but should it?
>     
>     The chown (recursive or not) does look out of place in the fetcher. Since we always
have a container logger now (default to SandboxContainerLogger which has no runtime component
but only exists during container setup) it seems appropriate for the logger to assume the
responsibility to set up stderr and stdout FDs (backed by files or not) and make sure the
contained processes (fetcher and executor both now run under the task user) **can write**
to them, right?
> 
> Joseph Wu wrote:
>     It does make sense to pass responsibility for the fetcher's stdout/stderr into the
ContainerLogger too.  This will give a unified logging interface, in case anyone wants to
pipe their logs somewhere else.
>     
>     In order to do this, looks like there are a couple changes needed to the fetcher
interface and how we pass along file descriptors from the ContainerLogger.
>     
>     (1) The fetcher will need to take some additional arguments of the form:
>     ```
>     const ContainerLogger::SubprocessInfo& subprocessInfo
>     ```
>     Here:
>     https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/fetcher.hpp#L95-L106
>     
>     ---
>     
>     (2) The containerizers need to pass this `SubprocessInfo` from the ContainerLogger
to Fetcher.  We can accomplish this fairly trivially in the Mesos Containerizer:
>     
>     The logger is called here:
>     https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1188-L1190
>     
>     And the fetcher is called inside the logger's continuation:
>     https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1358-L1361
>     
>     Unfortunately, the Docker Containerizer has it backwards:
>     https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L1111-L1123
>     https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L1143-L1155
>     
>     ---
>     
>     (3) We will need to tweak the way `SubprocessInfo::FD` is passed around.  Currently,
as soon as the FD is sent into `subprocess`, the ownership of the FD is transfered into the
subprocess.  If we plan to send the same FD to the Executor and Fetcher, the fetcher cannot
be allowed to take ownership.
>     
>     https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/include/mesos/slave/container_logger.hpp#L79-L86
> 
> Jiang Yan Xu wrote:
>     Thanks for the reply. In addition to passing SubprocessInfo to the fetcher, we have
to set the perms for the files. Since it's only relavent to the sandbox logger, I guess we
have to handle it in the sandbox logger itself? This would probably require the 'user' (which
can't be derived from `ExecutorInfo`) to be passed into `ContainerLogger::prepare()`.
>     
>     Thoughts?

There's a separate issue in progress to fix passing the user into all ContainerLoggers:
https://issues.apache.org/jira/browse/MESOS-5856


- Joseph


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


On Sept. 20, 2016, 4:32 p.m., Megha Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
>     https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 96e24500a12825161553eb050da389088b122695

> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> -------
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>


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