----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52058/#review150823 -----------------------------------------------------------
src/slave/containerizer/fetcher.cpp (lines 755 - 756) <https://reviews.apache.org/r/52058/#comment218894> 'check if it's a valid user' is not accurate, here we have past the point that the user can still be invalid. It's obvious that we are chowning the two files here so I'd say we don't need to comment on it anymore. src/slave/containerizer/fetcher.cpp (lines 757 - 783) <https://reviews.apache.org/r/52058/#comment218895> Let's reorder a bit. ``` Try<Nothing> chownOut = os::chown( user.get(), path::join(info.sandbox_directory(), "stdout"), false); if (chownOut.isError()) { os::close(out.get()); os::close(err.get()); return Failure(...); } Try<Nothing> chownErr = os::chown( user.get(), path::join(info.sandbox_directory(), "stderr"), false); if (chownErr.isError()) { os::close(out.get()); os::close(err.get()); return Failure(...); } ``` Even though we are calling ``` os::close(out.get()); os::close(err.get()); ``` in two places but with a little bit of redundancy we would not be in the dilemma of choosing which error to propagate and "what if they both failed?" If chownOut failed, we return without chowning the other file, which is pretty reasonable. src/slave/containerizer/fetcher.cpp (line 764) <https://reviews.apache.org/r/52058/#comment218896> No space between function name and open paren. src/slave/containerizer/fetcher.cpp (lines 770 - 773) <https://reviews.apache.org/r/52058/#comment218898> Let's remove a few redundant words. The following output should be clear enough: "Failed to chown '/path/to/sandbox/stdout' to user foo: <chownOut.error()>" src/slave/containerizer/fetcher.cpp (lines 771 - 773) <https://reviews.apache.org/r/52058/#comment218897> Wrap operator `+` at the end of lines (for consistency). src/tests/containerizer/mesos_containerizer_tests.cpp (line 505) <https://reviews.apache.org/r/52058/#comment218903> We probably don't need such a constraint. macOS should be fine. src/tests/containerizer/mesos_containerizer_tests.cpp (lines 506 - 524) <https://reviews.apache.org/r/52058/#comment218902> Can we just use the user `nobody`? See the comments in the test. src/tests/containerizer/mesos_containerizer_tests.cpp (line 529) <https://reviews.apache.org/r/52058/#comment218901> s/SandboxOwnershipTest/MesosContainerizerExecuteTest/ would probably make sense. src/tests/containerizer/mesos_containerizer_tests.cpp (line 530) <https://reviews.apache.org/r/52058/#comment218900> Can we just use ``` const string user = "nobody"; Result<uid_t> uid = os::getuid(user); ASSERT_SOME(uid); ``` ? src/tests/containerizer/mesos_containerizer_tests.cpp (line 536) <https://reviews.apache.org/r/52058/#comment218899> This is probably not necessary. I understand that you are simulating the agent's chowning of the sandbox but here what matters is that the test runs under root but the files are still owned by `nobody`, then we have verified that it's working. src/tests/containerizer/mesos_containerizer_tests.cpp (lines 538 - 541) <https://reviews.apache.org/r/52058/#comment218904> We don't need these? src/tests/containerizer/mesos_containerizer_tests.cpp (lines 551 - 552) <https://reviews.apache.org/r/52058/#comment218907> This is OK but we can directly use `DEFAULT_CONTAINER_ID` inline below. src/tests/containerizer/mesos_containerizer_tests.cpp (lines 554 - 561) <https://reviews.apache.org/r/52058/#comment218905> Simplify: ``` ExecutorInfo executorInfo = CREATE_EXECUTOR_INFO("executor", "echo hello"); executorInfo.mutable_command()->set_value(user); ``` src/tests/containerizer/mesos_containerizer_tests.cpp (line 564) <https://reviews.apache.org/r/52058/#comment218906> DEFAULT_CONTAINER_ID src/tests/containerizer/mesos_containerizer_tests.cpp (line 579) <https://reviews.apache.org/r/52058/#comment218931> Capitalization and punctuation. src/tests/containerizer/mesos_containerizer_tests.cpp (lines 581 - 582) <https://reviews.apache.org/r/52058/#comment218930> Inline it? ``` ::stat(path::join(sandbox, "stdout").c_str(), &s); ``` src/tests/containerizer/mesos_containerizer_tests.cpp (line 585) <https://reviews.apache.org/r/52058/#comment218932> Capitalization and punctuation. src/tests/containerizer/mesos_containerizer_tests.cpp (lines 586 - 587) <https://reviews.apache.org/r/52058/#comment218933> Same as above. - Jiang Yan Xu On Sept. 28, 2016, 10:17 a.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52058/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2016, 10:17 a.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 > 11104d66e6dd05d8eb1d37a2e3250aca19278110 > src/tests/containerizer/mesos_containerizer_tests.cpp > 8b5a19e121ef74eaf99b39682f8fd170605bf56d > > Diff: https://reviews.apache.org/r/52058/diff/ > > > Testing > ------- > > make check on linux > > > Thanks, > > Megha Sharma > >
