mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 54515: Use `os::var()` in `Flags::runtime_dir`.
Date Fri, 16 Dec 2016 00:39:05 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54515/#review159393
-----------------------------------------------------------


Ship it!





src/slave/flags.cpp (line 226)
<https://reviews.apache.org/r/54515/#comment230389>

    Nit: We close `#ifdef`s like `#endif // __WINDOWS__`.



src/slave/flags.cpp (lines 227 - 230)
<https://reviews.apache.org/r/54515/#comment230388>

    You might have overlooked the ending `()` in `[]() -> string {}()` when writing this
comment :)
    
    The default value for `--runtime_dir` is only calculated once per instantiation of the
flag object.  Not once per access of the `runtime_dir` field.
    
    I'll reword this comment to make it more accurate.


- Joseph Wu


On Dec. 15, 2016, 4:08 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54515/
> -----------------------------------------------------------
> 
> (Updated Dec. 15, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6722
>     https://issues.apache.org/jira/browse/MESOS-6722
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit improves the logic of `Flags::runtime_dir` to use
> `os::var()` for encapsulation of `/var` vs `C:\ProgramData`.
> 
> However, because the suffixes are different (`/run/mesos` versus
> `/mesos/runtime`), the CLI still has some platform-specific code. We
> chose to keep this logic in the CLI so that it is set once, and users
> can continue to consume `Flags::runtime_dir` instead of something like
> `os::runtime_dir()`. This is primarily due to the fact that we have two
> default paths: a system path (in `/var/run` and `ProgramData`) and a
> fallback path in `/tmp` if the `os::access()` check fails. We
> specifically want to avoid the situation where `os::access()` is called
> multiple times due the use of `su` throughout Mesos. The `runtime_dir`
> must not change after it has been set.
> 
> The permission check for the system default path is fixed to check
> actual access permissions to the directory in which Mesos creates its
> runtime directory, instead of assuming permissions based on a comparison
> of `os::user() == "root"`.
> 
> Stylistically we chose a bit of duplication as a trade-off for enhanced
> readability and auto-formatting to work properly.
> 
> 
> Diffs
> -----
> 
>   src/slave/flags.cpp a1dfbf934ef7d44cdb019aae826c90caaf477775 
> 
> Diff: https://reviews.apache.org/r/54515/diff/
> 
> 
> Testing
> -------
> 
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
> 
> Checked that running the agent as non-root on Linux *without* read/write permissions
to `/var/run`
> correctly fell back to `/tmp/mesos/runtime`.
> 
> Checked that running the agent as non-administrator on Windows correctly fell back to
`os::temp()/mesos/runtime`.
> 
> Checked that running as `root` on Linux and `Administrator` on Windows
> chose the correct default `runtime_dir` paths.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


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