You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2016/12/05 17:38:30 UTC
Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/
-----------------------------------------------------------
Review request for mesos.
Bugs: MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6677
Repository: mesos
Description
-------
The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.
Now uses `os::var()` to get `C:\ProgramData\mesos\runtime` for Windows
and `/var/lib/mesos/runtime` for POSIX.
Diffs
-----
src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57
src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44
Diff: https://reviews.apache.org/r/54336/diff/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 5, 2016, 7:56 p.m., Alex Clemmer wrote:
> > src/slave/flags.cpp, lines 213-231
> > <https://reviews.apache.org/r/54336/diff/1/?file=1575121#file1575121line213>
> >
> > In general, I do question whether this should be in the CLI code. It seems like this lambda should be wrapped up into a function, `Try<std::string> os::runstatedir` (as Jie suggests in #54335)? Then you can `CHECK_SOME` on that result here.
> >
> > Just as a note, since this review is blocking the agent code working, I do think it's fine to not have it block on the `os::var` review, and just have it return a directory in `temp` on Windows until we sort out the issues in the `os::var` review. The questions about what where we should put variable data on Windows is (IMO) worth considering somewhat carefully, but they are not necessary to complete here.
>
> Andrew Schwartzmeyer wrote:
> Agreed, rewrote to not be dependent on `os::var`. Waiting on tests.
I'd note that the POSIX version as well shouldn't be in the CLI code; but the refactor to `os::runstatedir` will be part of the the longer task, #54335, and this review is just to unblock the Windows agent.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158050
-----------------------------------------------------------
On Dec. 5, 2016, 11:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 11:38 p.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default `runtime_dir` value was POSIX specific,
> and caused https://issues.apache.org/jira/browse/MESOS-6677.
>
> The constant was removed in preparation for `os::runstatedir` refactor.
>
> We unblock the Windows agent by guarding the POSIX code on Windows.
>
>
> 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 (testing second iteration now).
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 5, 2016, 7:56 p.m., Alex Clemmer wrote:
> > src/slave/flags.cpp, lines 213-231
> > <https://reviews.apache.org/r/54336/diff/1/?file=1575121#file1575121line213>
> >
> > In general, I do question whether this should be in the CLI code. It seems like this lambda should be wrapped up into a function, `Try<std::string> os::runstatedir` (as Jie suggests in #54335)? Then you can `CHECK_SOME` on that result here.
> >
> > Just as a note, since this review is blocking the agent code working, I do think it's fine to not have it block on the `os::var` review, and just have it return a directory in `temp` on Windows until we sort out the issues in the `os::var` review. The questions about what where we should put variable data on Windows is (IMO) worth considering somewhat carefully, but they are not necessary to complete here.
Agreed, rewrote to not be dependent on `os::var`. Waiting on tests.
> On Dec. 5, 2016, 7:56 p.m., Alex Clemmer wrote:
> > src/slave/flags.cpp, line 214
> > <https://reviews.apache.org/r/54336/diff/1/?file=1575121#file1575121line214>
> >
> > Hmm, shouldn't this be a `Try<std::string>`?
> >
> > Also, in #54336 the Unix version of `os::var` is returning a `std::string`, so unless I'm missing something important, it seems like this should actually not build on Unix?
Oops. I assumed Result<T> and Try<T> were equivalent, but only Result<Option<T>> is equivalent to Try<T>. Anyway, it was removed in the rewrite without `os::var`.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158050
-----------------------------------------------------------
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default `runtime_dir` value was POSIX specific,
> and caused https://issues.apache.org/jira/browse/MESOS-6677.
> Now uses `os::var()` to get `C:\ProgramData\mesos\runtime` for Windows
> and `/var/lib/mesos/runtime` for POSIX.
>
>
> Diffs
> -----
>
> src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57
> src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44
>
> Diff: https://reviews.apache.org/r/54336/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158050
-----------------------------------------------------------
src/slave/flags.cpp (lines 213 - 231)
<https://reviews.apache.org/r/54336/#comment228723>
In general, I do question whether this should be in the CLI code. It seems like this lambda should be wrapped up into a function, `Try<std::string> os::runstatedir` (as Jie suggests in #54335)? Then you can `CHECK_SOME` on that result here.
Just as a note, since this review is blocking the agent code working, I do think it's fine to not have it block on the `os::var` review, and just have it return a directory in `temp` on Windows until we sort out the issues in the `os::var` review. The questions about what where we should put variable data on Windows is (IMO) worth considering somewhat carefully, but they are not necessary to complete here.
src/slave/flags.cpp (line 214)
<https://reviews.apache.org/r/54336/#comment228722>
Hmm, shouldn't this be a `Try<std::string>`?
Also, in #54336 the Unix version of `os::var` is returning a `std::string`, so unless I'm missing something important, it seems like this should actually not build on Unix?
- Alex Clemmer
On Dec. 5, 2016, 5:38 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:38 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6677
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The default `runtime_dir` value was POSIX specific,
> and caused https://issues.apache.org/jira/browse/MESOS-6677.
> Now uses `os::var()` to get `C:\ProgramData\mesos\runtime` for Windows
> and `/var/lib/mesos/runtime` for POSIX.
>
>
> Diffs
> -----
>
> src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57
> src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44
>
> Diff: https://reviews.apache.org/r/54336/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158094
-----------------------------------------------------------
Ship it!
It is debatable that you would want to put `path::join(os::temp(), "mesos", "runtime")` into a variable; I don't think this particularly helps the clarity, and doesn't make the code safer (since the only two usages would be so close together), so I think it's fine the way it is, but I do think it's worth pointing out that some people would argue with me on this. :)
- Alex Clemmer
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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158146
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54336]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 5, 2016, 6:33 p.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?
>
> Alex Clemmer wrote:
> 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.
>
> Andrew Schwartzmeyer wrote:
> I would like to point out that, as far as I can tell, this is the only hardcoded absolute path in any of the `constants.hpp` headers. I think that it should be removed as a matter of preventing its accidental use on a platform for which it is incorrect.
>
> That said, I can see why you'd be not want to remove it given just this commit, as it only moves the definition into the `Flags::runtime_dir`, which is *also* not the right place for this. Perhaps it'd be appropriate to add `os::runstate_dir` as Alex and Jie suggest to hold this platform-depenent constant (keeping in mind that this commit also does not introduce the correct Windows location of `ProgramData`).
Ok, sounds like a reasonable workaround for now.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158103
-----------------------------------------------------------
On Dec. 5, 2016, 5:06 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:06 p.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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Alex Clemmer <cl...@gmail.com>.
> 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.
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?
> 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/
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?
- 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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> 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?
>
> Alex Clemmer wrote:
> 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.
To add to this, there is a subtle bug with *just* fixing `os::user()`. While that would prevent the crash, it would introduce a strange edge case where the folder used most of the time is `path::join(os::temp(), "mesos", "runtime")` except when the Windows user is named `root` (edge case, but valid), and it becomes `/var/run/mesos`. This is not what the the code was intending to mean, so I think it would be remiss to introduce this bug.
And I agree that `/var/run/mesos` is not the appropriate path on Windows. The analogous path would be `ProgramData`, which I'm working changing correctly.
> 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?
>
> Alex Clemmer wrote:
> 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.
I would like to point out that, as far as I can tell, this is the only hardcoded absolute path in any of the `constants.hpp` headers. I think that it should be removed as a matter of preventing its accidental use on a platform for which it is incorrect.
That said, I can see why you'd be not want to remove it given just this commit, as it only moves the definition into the `Flags::runtime_dir`, which is *also* not the right place for this. Perhaps it'd be appropriate to add `os::runstate_dir` as Alex and Jie suggest to hold this platform-depenent constant (keeping in mind that this commit also does not introduce the correct Windows location of `ProgramData`).
- Andrew
-----------------------------------------------------------
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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Alex Clemmer <cl...@gmail.com>.
> 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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158103
-----------------------------------------------------------
src/slave/constants.hpp
<https://reviews.apache.org/r/54336/#comment228810>
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.
src/slave/flags.cpp (lines 214 - 216)
<https://reviews.apache.org/r/54336/#comment228815>
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/
- Joseph Wu
On Dec. 5, 2016, 5:06 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:06 p.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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/#review158228
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/flags.cpp (lines 214 - 222)
<https://reviews.apache.org/r/54336/#comment229032>
Since we want to (eventually) add something like an `os::var()`, you should add a TODO here to note that.
- Joseph Wu
On Dec. 5, 2016, 5:06 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54336/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2016, 5:06 p.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
>
>
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
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.
Changes
-------
Remove dependency.
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
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/
-----------------------------------------------------------
(Updated Dec. 6, 2016, 12:39 a.m.)
Review request for mesos and Alex Clemmer.
Changes
-------
Fix comment format.
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 (updated)
-----
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
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/
-----------------------------------------------------------
(Updated Dec. 6, 2016, 12:31 a.m.)
Review request for mesos and Alex Clemmer.
Bugs: MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6677
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/
-----------------------------------------------------------
(Updated Dec. 6, 2016, 12:21 a.m.)
Review request for mesos and Alex Clemmer.
Changes
-------
Fixed replacement of `DEFAULT_ROOT_RUNTIME_DIRECTORY`.
Bugs: MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6677
Repository: mesos
Description
-------
The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.
The constant was removed in preparation for `os::runstatedir` refactor.
We unblock the Windows agent by guarding the POSIX code on Windows.
Diffs (updated)
-----
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
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/
-----------------------------------------------------------
(Updated Dec. 6, 2016, 12:10 a.m.)
Review request for mesos and Alex Clemmer.
Changes
-------
Updated with test results.
Bugs: MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6677
Repository: mesos
Description
-------
The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.
The constant was removed in preparation for `os::runstatedir` refactor.
We unblock the Windows agent by guarding the POSIX code on Windows.
Diffs
-----
src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57
src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44
Diff: https://reviews.apache.org/r/54336/diff/
Testing (updated)
-------
make && make check on Linux: 1411 tests passed, no failures.
msbuild and attached to Linux master: no runtime failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54336: Windows: Fix `Flags::runtime_dir` value.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54336/
-----------------------------------------------------------
(Updated Dec. 5, 2016, 11:38 p.m.)
Review request for mesos and Alex Clemmer.
Bugs: MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6677
Repository: mesos
Description (updated)
-------
The default `runtime_dir` value was POSIX specific,
and caused https://issues.apache.org/jira/browse/MESOS-6677.
The constant was removed in preparation for `os::runstatedir` refactor.
We unblock the Windows agent by guarding the POSIX code on Windows.
Diffs (updated)
-----
src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57
src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44
Diff: https://reviews.apache.org/r/54336/diff/
Testing (updated)
-------
make && make check on Linux: 1411 tests passed, no failures.
msbuild and attached to Linux master: no runtime failures (testing second iteration now).
Thanks,
Andrew Schwartzmeyer