> 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.
@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? - 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 > >
