You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2017/01/20 23:32:17 UTC
Review Request 55795: Fixed `umountAll` to optionally also clean up
mtab.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
Bugs: MESOS-6478
https://issues.apache.org/jira/browse/MESOS-6478
Repository: mesos
Description
-------
- Without this, Mesos tests leave garbage in /etc/mtab.
Diffs
-----
src/linux/fs.hpp da49c9ebfa938d169152ed3b6e4df7378711b013
src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
src/tests/containerizer/docker_volume_isolator_tests.cpp 4040d36d1dbf4968f62fa593024936858f28b4df
src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135
src/tests/persistent_volume_tests.cpp 468a85b4a6ce09592341afd07ce12a03f5fc4f73
Diff: https://reviews.apache.org/r/55795/diff/
Testing
-------
sudo make check.
Verified that no garbage is left in mtab.
Thanks,
Jiang Yan Xu
Re: Review Request 55795: Fixed `umountAll` to optionally also clean
up mtab.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/#review162887
-----------------------------------------------------------
Ship it!
Ship It!
- James Peach
On Jan. 25, 2017, 12:45 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55795/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 12:45 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
>
>
> Bugs: MESOS-6478
> https://issues.apache.org/jira/browse/MESOS-6478
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Without this, Mesos tests leave garbage in /etc/mtab.
>
>
> Diffs
> -----
>
> src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
>
> Diff: https://reviews.apache.org/r/55795/diff/
>
>
> Testing
> -------
>
> sudo make check.
>
> Verified that no garbage is left in mtab.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 55795: Fixed `umountAll` to optionally also clean
up mtab.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/
-----------------------------------------------------------
(Updated Jan. 24, 2017, 4:45 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
Changes
-------
Review comments.
Bugs: MESOS-6478
https://issues.apache.org/jira/browse/MESOS-6478
Repository: mesos
Description
-------
- Without this, Mesos tests leave garbage in /etc/mtab.
Diffs (updated)
-----
src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
Diff: https://reviews.apache.org/r/55795/diff/
Testing
-------
sudo make check.
Verified that no garbage is left in mtab.
Thanks,
Jiang Yan Xu
Re: Review Request 55795: Fixed `umountAll` to optionally also clean
up mtab.
Posted by James Peach <jp...@apache.org>.
> On Jan. 23, 2017, 9:39 p.m., James Peach wrote:
> > src/linux/fs.cpp, line 414
> > <https://reviews.apache.org/r/55795/diff/1/?file=1611031#file1611031line414>
> >
> > Why do you need `cleanUpMtab`. All the callers pass `true` and it is pretty hard to imagine a scenario where you *want* the mtab to be out of sync.
>
> Jiang Yan Xu wrote:
> Yeah I don't think anyone would want that but if the caller mounts everything exclusively using the syscall under `target` then they don't *need* to clean up mtab. (There's a minor cost to it and potential failure factors but admittedly I haven't seen it fail in cases including non-existent /etc/mtab, non-existent entry, etc.)
>
> So maybe it's fine to leave out the boolean argument and fix things later if anything breaks in any environments.
>
> James Peach wrote:
> This is already subject to partial failure, but I'd also be OK with ignoring the return from executing `umount`, or refactoring this to rewrite `mtab` using `setmntent(3)`. I think that this patch is an existence proof that no-one really knows they don't need to clean up :)
>
> Jiang Yan Xu wrote:
> re `setmntent(3)`: sounds worthy of a TODO but given that they are currently used mainly for tests I would take this short cut for now :)
> re partial failure: I guess it's unavoidable. I don't think this method itself knows if it should ignore the failure, right now it returns an error when `fs::unmount` fails on a mount entry so I guess returning an error when `umount --fake` fails is basically treating the two as two equal steps taken for each mount.
Yup I agree. Leave it as is but remove the `cleanUpMtab` arg?
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/#review162706
-----------------------------------------------------------
On Jan. 20, 2017, 11:32 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55795/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 11:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
>
>
> Bugs: MESOS-6478
> https://issues.apache.org/jira/browse/MESOS-6478
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Without this, Mesos tests leave garbage in /etc/mtab.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp da49c9ebfa938d169152ed3b6e4df7378711b013
> src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
> src/tests/containerizer/docker_volume_isolator_tests.cpp 4040d36d1dbf4968f62fa593024936858f28b4df
> src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135
> src/tests/persistent_volume_tests.cpp 468a85b4a6ce09592341afd07ce12a03f5fc4f73
>
> Diff: https://reviews.apache.org/r/55795/diff/
>
>
> Testing
> -------
>
> sudo make check.
>
> Verified that no garbage is left in mtab.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 55795: Fixed `umountAll` to optionally also clean
up mtab.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Jan. 23, 2017, 1:39 p.m., James Peach wrote:
> > src/linux/fs.cpp, line 414
> > <https://reviews.apache.org/r/55795/diff/1/?file=1611031#file1611031line414>
> >
> > Why do you need `cleanUpMtab`. All the callers pass `true` and it is pretty hard to imagine a scenario where you *want* the mtab to be out of sync.
>
> Jiang Yan Xu wrote:
> Yeah I don't think anyone would want that but if the caller mounts everything exclusively using the syscall under `target` then they don't *need* to clean up mtab. (There's a minor cost to it and potential failure factors but admittedly I haven't seen it fail in cases including non-existent /etc/mtab, non-existent entry, etc.)
>
> So maybe it's fine to leave out the boolean argument and fix things later if anything breaks in any environments.
>
> James Peach wrote:
> This is already subject to partial failure, but I'd also be OK with ignoring the return from executing `umount`, or refactoring this to rewrite `mtab` using `setmntent(3)`. I think that this patch is an existence proof that no-one really knows they don't need to clean up :)
re `setmntent(3)`: sounds worthy of a TODO but given that they are currently used mainly for tests I would take this short cut for now :)
re partial failure: I guess it's unavoidable. I don't think this method itself knows if it should ignore the failure, right now it returns an error when `fs::unmount` fails on a mount entry so I guess returning an error when `umount --fake` fails is basically treating the two as two equal steps taken for each mount.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/#review162706
-----------------------------------------------------------
On Jan. 20, 2017, 3:32 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55795/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 3:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
>
>
> Bugs: MESOS-6478
> https://issues.apache.org/jira/browse/MESOS-6478
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Without this, Mesos tests leave garbage in /etc/mtab.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp da49c9ebfa938d169152ed3b6e4df7378711b013
> src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
> src/tests/containerizer/docker_volume_isolator_tests.cpp 4040d36d1dbf4968f62fa593024936858f28b4df
> src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135
> src/tests/persistent_volume_tests.cpp 468a85b4a6ce09592341afd07ce12a03f5fc4f73
>
> Diff: https://reviews.apache.org/r/55795/diff/
>
>
> Testing
> -------
>
> sudo make check.
>
> Verified that no garbage is left in mtab.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 55795: Fixed `umountAll` to optionally also clean
up mtab.
Posted by James Peach <jp...@apache.org>.
> On Jan. 23, 2017, 9:39 p.m., James Peach wrote:
> > src/linux/fs.cpp, line 414
> > <https://reviews.apache.org/r/55795/diff/1/?file=1611031#file1611031line414>
> >
> > Why do you need `cleanUpMtab`. All the callers pass `true` and it is pretty hard to imagine a scenario where you *want* the mtab to be out of sync.
>
> Jiang Yan Xu wrote:
> Yeah I don't think anyone would want that but if the caller mounts everything exclusively using the syscall under `target` then they don't *need* to clean up mtab. (There's a minor cost to it and potential failure factors but admittedly I haven't seen it fail in cases including non-existent /etc/mtab, non-existent entry, etc.)
>
> So maybe it's fine to leave out the boolean argument and fix things later if anything breaks in any environments.
This is already subject to partial failure, but I'd also be OK with ignoring the return from executing `umount`, or refactoring this to rewrite `mtab` using `setmntent(3)`. I think that this patch is an existence proof that no-one really knows they don't need to clean up :)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/#review162706
-----------------------------------------------------------
On Jan. 20, 2017, 11:32 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55795/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 11:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
>
>
> Bugs: MESOS-6478
> https://issues.apache.org/jira/browse/MESOS-6478
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Without this, Mesos tests leave garbage in /etc/mtab.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp da49c9ebfa938d169152ed3b6e4df7378711b013
> src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
> src/tests/containerizer/docker_volume_isolator_tests.cpp 4040d36d1dbf4968f62fa593024936858f28b4df
> src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135
> src/tests/persistent_volume_tests.cpp 468a85b4a6ce09592341afd07ce12a03f5fc4f73
>
> Diff: https://reviews.apache.org/r/55795/diff/
>
>
> Testing
> -------
>
> sudo make check.
>
> Verified that no garbage is left in mtab.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 55795: Fixed `umountAll` to optionally also clean
up mtab.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Jan. 23, 2017, 1:39 p.m., James Peach wrote:
> > src/linux/fs.cpp, line 414
> > <https://reviews.apache.org/r/55795/diff/1/?file=1611031#file1611031line414>
> >
> > Why do you need `cleanUpMtab`. All the callers pass `true` and it is pretty hard to imagine a scenario where you *want* the mtab to be out of sync.
Yeah I don't think anyone would want that but if the caller mounts everything exclusively using the syscall under `target` then they don't *need* to clean up mtab. (There's a minor cost to it and potential failure factors but admittedly I haven't seen it fail in cases including non-existent /etc/mtab, non-existent entry, etc.)
So maybe it's fine to leave out the boolean argument and fix things later if anything breaks in any environments.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/#review162706
-----------------------------------------------------------
On Jan. 20, 2017, 3:32 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55795/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 3:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
>
>
> Bugs: MESOS-6478
> https://issues.apache.org/jira/browse/MESOS-6478
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Without this, Mesos tests leave garbage in /etc/mtab.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp da49c9ebfa938d169152ed3b6e4df7378711b013
> src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
> src/tests/containerizer/docker_volume_isolator_tests.cpp 4040d36d1dbf4968f62fa593024936858f28b4df
> src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135
> src/tests/persistent_volume_tests.cpp 468a85b4a6ce09592341afd07ce12a03f5fc4f73
>
> Diff: https://reviews.apache.org/r/55795/diff/
>
>
> Testing
> -------
>
> sudo make check.
>
> Verified that no garbage is left in mtab.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 55795: Fixed `umountAll` to optionally also clean
up mtab.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55795/#review162706
-----------------------------------------------------------
src/linux/fs.cpp (line 414)
<https://reviews.apache.org/r/55795/#comment234035>
Why do you need `cleanUpMtab`. All the callers pass `true` and it is pretty hard to imagine a scenario where you *want* the mtab to be out of sync.
- James Peach
On Jan. 20, 2017, 11:32 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55795/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 11:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
>
>
> Bugs: MESOS-6478
> https://issues.apache.org/jira/browse/MESOS-6478
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Without this, Mesos tests leave garbage in /etc/mtab.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp da49c9ebfa938d169152ed3b6e4df7378711b013
> src/linux/fs.cpp 913e23317291db164fe6bdf77f3eca146dedec9b
> src/tests/containerizer/docker_volume_isolator_tests.cpp 4040d36d1dbf4968f62fa593024936858f28b4df
> src/tests/environment.cpp a9e217e8beb5a6aca456ff5379893953cafca135
> src/tests/persistent_volume_tests.cpp 468a85b4a6ce09592341afd07ce12a03f5fc4f73
>
> Diff: https://reviews.apache.org/r/55795/diff/
>
>
> Testing
> -------
>
> sudo make check.
>
> Verified that no garbage is left in mtab.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>