> 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
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? - Jiang Yan ----------------------------------------------------------- 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 > >
