----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64978/#review195000 -----------------------------------------------------------
Can you add a test or update an existing test to verify the /files endpoint for task volume? Ideally, you could also verify that once the executor's work directory is gc'ed the files endpoint no longer serves the task volume directory to ensure we are not leaking those directories. src/slave/slave.cpp Lines 2796 (patched) <https://reviews.apache.org/r/64978/#comment274096> s/endpoints/endpoint/ src/slave/slave.cpp Lines 2822 (patched) <https://reviews.apache.org/r/64978/#comment274097> Can you comment here on exactly why you are doing this? The JIRA issue doesn't really talk about the final solution/workaround. src/slave/slave.cpp Lines 5836-5862 (patched) <https://reviews.apache.org/r/64978/#comment274099> I don't follow why you are detaching here? Even if an executor is terminated we still want to show its files (and its tasks files) in the WebUI. I think you want to detach when we are GC'ing the executor's run directory. See for example `garbageCollect` calls to executor run directory inside `removeExecutor` and `recoverExecutor`. You should do the detaching as a `.then`. src/slave/slave.cpp Lines 8684-8712 (patched) <https://reviews.apache.org/r/64978/#comment274100> See above. This should be in the `.then` continuation at #8718. src/slave/slave.cpp Lines 9132 (patched) <https://reviews.apache.org/r/64978/#comment274101> Ditto. Can you add a comment here. src/slave/slave.cpp Lines 9221-9246 (patched) <https://reviews.apache.org/r/64978/#comment274102> Kill this. See above comments. - Vinod Kone On Jan. 7, 2018, 10 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64978/ > ----------------------------------------------------------- > > (Updated Jan. 7, 2018, 10 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-8279 > https://issues.apache.org/jira/browse/MESOS-8279 > > > Repository: mesos > > > Description > ------- > > In MESOS-7225, we made a task can access any volumes specified in its > disk resources from its own sandbox by introducing a workaround to the > default executor, i.e., add a `SANDBOX_PATH` volume with type `PARENT` > to the corresponding nested container. It will be translated into a bind > mount in the nested container's mount namespace, thus not visible in the > host mount namespace, that means the task's volume directory can not be > visible in Mesos UI since it operates in the host mount namespace. > > In this patch, to make the task's volume directory visible in Mesos UI, > we attached the executor's volume directory to it, so when users browse > task's volume directory in Mesos UI, what they actually browse is the > executor's volume directory. Note when calling `Files::attach()`, the > third argument `authorized` is not specified, that is because it is > already specified when we do the attach for the executor's sandbox and > it is also applied to the executor's tasks. > > > Diffs > ----- > > src/slave/slave.cpp cfb675df677e7a0d476b8d5a586afc2f197ab810 > > > Diff: https://reviews.apache.org/r/64978/diff/2/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Qian Zhang > >
