mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.
Date Thu, 29 Sep 2016 15:48:31 GMT

-----------------------------------------------------------
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
> 
>


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