mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Date Tue, 06 Dec 2016 03:24:45 GMT


> On Dec. 6, 2016, 2:33 a.m., Joseph Wu wrote:
> > src/slave/flags.cpp, lines 214-216
> > <https://reviews.apache.org/r/54336/diff/5/?file=1577362#file1577362line214>
> >
> >     Seems like the problem on Windows is `os::user()` rather than the value of the
`--runtime_dir` flag.  As long as `os::user()` returns some value, we'll get an appropriate
default runtime directory for Windows.  In most cases, `/var/run/mesos` should work on Windows
(unless there are permission issues?), because our code does a recursive `mkdir` before using
the runtime directory.
> >     
> >     If this is the case, this review is probably the approach we want to take: https://reviews.apache.org/r/53706/
> 
> Alex Clemmer wrote:
>     I actually don't see it this way. As I said in the comments of #54335, the reason
we are switching on the `os::user` at all is because `/var` is accessible only from `root`
in some POSIXes. Since Windows does not suffer from this problem, I think we should put it
in a place that is sensible for Windows, which should eventually be something like `path::join(os::var(),
"mesos", "runtime")`. Until we have `os::var` (sometime in the next couple days), I think
it's reasonable to put this data in the default non-`root` Unix folder to unblock the rest
of the Windows team. What do you think?

Also, I'm not a super big fan of putting it in `/var` on Windows. :) I'd personally rather
find the right place for it on different platforms and put it there instead of just mirroring
what we have on POSIX.


> On Dec. 6, 2016, 2:33 a.m., Joseph Wu wrote:
> > src/slave/constants.hpp, lines 141-144
> > <https://reviews.apache.org/r/54336/diff/5/?file=1577361#file1577361line141>
> >
> >     There isn't any need to get rid of this constant, but we could update the comment.
> >     
> >     i.e. This is the _desired_ default, but if the path is inaccessible (posix non-root),
the default will be a temporary folder.
> 
> Alex Clemmer wrote:
>     I have a different view, actually. To me it seems like there's no particular reason
to have it, while there _are_ in my view a few good reasons to not have it (and, I tend to
believe that you should have to justify why something exists rather than why it shouldn't
exist). And, since it is used only once, it costs nothing to change.
>     
>     * We should really try to keep path literals out of the codebase as much as possible,
because of the subtle bugs and problems with combining paths with '\' and '/' on Windows.
Even if it is safe in this case to use, I think we should always think carefully about whether
we have a good reason for having a string literal path, and in this case, I don't see a clear
benefit, while I do see a clear risk of someone accidentally `path::join`'ing onto the end
at some point in the future. It seems like a better choice (as Jie suggests in #54335) would
be to implement this as `os::runstatedir`. See my next point for more on this.
>     * To me, it makes sense to have a platform-indepenent variable data root in a new
function `os::var()` (_i.e._ `/var` on POSIX, and something like `ProgramData` on Windows),
and to use something like `path::join(os::var(), ...)` to implement a function, `os::runstatedir()`.
I think this is reasonable in particular since (per conversation with Jie) we don't want to
change the default of this path away from `/var`.
>     
>     What are your thoughts? Am I missing something?

Oh, also, on the two points above, it's worth reading my next comment to see a more complete
picture for our current plan to accomplish the second part in particular.


- Alex


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


On Dec. 6, 2016, 1:06 a.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2016, 1:06 a.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Bugs: MESOS-6677
>     https://issues.apache.org/jira/browse/MESOS-6677
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit fixes MESOS-6677, which breaks the ability
> to run any agent on Windows, and thus is blocking all
> Windows development progress on the `master` branch.
> 
> The cause was that the default `runtime_dir` value was POSIX specific,
> and used `os::user()` which is deleted on Windows.
> The fix is to guard the POSIX code, and add a
> Windows implementation.
> 
> 
> Diffs
> -----
> 
>   src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 
>   src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 
> 
> Diff: https://reviews.apache.org/r/54336/diff/
> 
> 
> Testing
> -------
> 
> make && make check on Linux: 1411 tests passed, no failures.
> 
> msbuild and attached to Linux master: no runtime failures.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


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