You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2018/10/24 23:49:17 UTC
Review Request 69149: Automatically remounted read-only bind mounts.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69149/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.
Bugs: MESOS-9354
https://issues.apache.org/jira/browse/MESOS-9354
Repository: mesos
Description
-------
To make a bind mount read-only, you have to first make the bind mount,
then remount it with the read-only flag. This is a bit arcane, which is
why `mount(8)` does it automatically.
This change updates `fs::mount()` to do the read-only remount
automatically when it is making a read-only bind mount so that every
caller doesn't have to carry special code to make it work correctly. All
the callers that make an explicit remount are updated to simply pass
the `MS_READONLY` flag if necessary.
Diffs
-----
src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8
src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 24c9fd6beb9657b80b33dc31c2939083c1aa9110
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 64271dfbb5c074ad3ac8d2a64c3943d739c0fffa
src/slave/containerizer/mesos/isolators/volume/host_path.cpp 2e03ef50a290c046ae2b02b332d3d007b572429d
src/slave/containerizer/mesos/isolators/volume/image.cpp 53cbaefeef7a6e10149e241e07d6e9cb8d510fc9
src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 21d9528c23d9142eccec456184b42d085b57d12c
src/slave/containerizer/mesos/provisioner/backends/bind.cpp 7d564dc7de3e3de8726c7fd42a301d979c4a2574
src/tests/containerizer/fs_tests.cpp 23cad35b5db81a70b43bec1c1dbafe008c8dd4da
Diff: https://reviews.apache.org/r/69149/diff/1/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69149: Automatically remounted read-only bind
mounts.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69149/#review210125
-----------------------------------------------------------
PASS: Mesos patch 69149 was successfully built and tested.
Reviews applied: `['69149']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2542/mesos-review-69149
- Mesos Reviewbot Windows
On Oct. 26, 2018, 10:58 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69149/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2018, 10:58 p.m.)
>
>
> Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9354
> https://issues.apache.org/jira/browse/MESOS-9354
>
>
> Repository: mesos
>
>
> Description
> -------
>
> To make a bind mount read-only, you have to first make the bind mount,
> then remount it with the read-only flag. This is a bit arcane, which is
> why `mount(8)` does it automatically.
>
> This change updates `fs::mount()` to do the read-only remount
> automatically when it is making a read-only bind mount so that every
> caller doesn't have to carry special code to make it work correctly. All
> the callers that make an explicit remount are updated to simply pass
> the `MS_READONLY` flag if necessary.
>
>
> Diffs
> -----
>
> src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 24c9fd6beb9657b80b33dc31c2939083c1aa9110
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 64271dfbb5c074ad3ac8d2a64c3943d739c0fffa
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp 2e03ef50a290c046ae2b02b332d3d007b572429d
> src/slave/containerizer/mesos/isolators/volume/image.cpp 53cbaefeef7a6e10149e241e07d6e9cb8d510fc9
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 21d9528c23d9142eccec456184b42d085b57d12c
> src/slave/containerizer/mesos/provisioner/backends/bind.cpp 7d564dc7de3e3de8726c7fd42a301d979c4a2574
> src/tests/containerizer/fs_tests.cpp 23cad35b5db81a70b43bec1c1dbafe008c8dd4da
>
>
> Diff: https://reviews.apache.org/r/69149/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69149: Automatically remounted read-only bind
mounts.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69149/
-----------------------------------------------------------
(Updated Oct. 26, 2018, 10:58 p.m.)
Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-9354
https://issues.apache.org/jira/browse/MESOS-9354
Repository: mesos
Description
-------
To make a bind mount read-only, you have to first make the bind mount,
then remount it with the read-only flag. This is a bit arcane, which is
why `mount(8)` does it automatically.
This change updates `fs::mount()` to do the read-only remount
automatically when it is making a read-only bind mount so that every
caller doesn't have to carry special code to make it work correctly. All
the callers that make an explicit remount are updated to simply pass
the `MS_READONLY` flag if necessary.
Diffs (updated)
-----
src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8
src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 24c9fd6beb9657b80b33dc31c2939083c1aa9110
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 64271dfbb5c074ad3ac8d2a64c3943d739c0fffa
src/slave/containerizer/mesos/isolators/volume/host_path.cpp 2e03ef50a290c046ae2b02b332d3d007b572429d
src/slave/containerizer/mesos/isolators/volume/image.cpp 53cbaefeef7a6e10149e241e07d6e9cb8d510fc9
src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 21d9528c23d9142eccec456184b42d085b57d12c
src/slave/containerizer/mesos/provisioner/backends/bind.cpp 7d564dc7de3e3de8726c7fd42a301d979c4a2574
src/tests/containerizer/fs_tests.cpp 23cad35b5db81a70b43bec1c1dbafe008c8dd4da
Diff: https://reviews.apache.org/r/69149/diff/2/
Changes: https://reviews.apache.org/r/69149/diff/1-2/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69149: Automatically remounted read-only bind
mounts.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69149/#review210014
-----------------------------------------------------------
PASS: Mesos patch 69149 was successfully built and tested.
Reviews applied: `['69149']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2526/mesos-review-69149
- Mesos Reviewbot Windows
On Oct. 24, 2018, 11:49 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69149/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 11:49 p.m.)
>
>
> Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9354
> https://issues.apache.org/jira/browse/MESOS-9354
>
>
> Repository: mesos
>
>
> Description
> -------
>
> To make a bind mount read-only, you have to first make the bind mount,
> then remount it with the read-only flag. This is a bit arcane, which is
> why `mount(8)` does it automatically.
>
> This change updates `fs::mount()` to do the read-only remount
> automatically when it is making a read-only bind mount so that every
> caller doesn't have to carry special code to make it work correctly. All
> the callers that make an explicit remount are updated to simply pass
> the `MS_READONLY` flag if necessary.
>
>
> Diffs
> -----
>
> src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 24c9fd6beb9657b80b33dc31c2939083c1aa9110
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 64271dfbb5c074ad3ac8d2a64c3943d739c0fffa
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp 2e03ef50a290c046ae2b02b332d3d007b572429d
> src/slave/containerizer/mesos/isolators/volume/image.cpp 53cbaefeef7a6e10149e241e07d6e9cb8d510fc9
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 21d9528c23d9142eccec456184b42d085b57d12c
> src/slave/containerizer/mesos/provisioner/backends/bind.cpp 7d564dc7de3e3de8726c7fd42a301d979c4a2574
> src/tests/containerizer/fs_tests.cpp 23cad35b5db81a70b43bec1c1dbafe008c8dd4da
>
>
> Diff: https://reviews.apache.org/r/69149/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69149: Automatically remounted read-only bind
mounts.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69149/#review210109
-----------------------------------------------------------
Fix it, then Ship it!
Looks good to me! Nice simplification.
src/linux/fs.cpp
Lines 510 (patched)
<https://reviews.apache.org/r/69149/#comment294742>
Should not be critical, but maybe check that the user is not trying to remount in the first place? Like checking that `!(flags & MS_REMOUNT)`?
src/linux/fs.cpp
Line 738 (original), 739-740 (patched)
<https://reviews.apache.org/r/69149/#comment294743>
Delete the old line with `MS_BIND`.
src/linux/fs.cpp
Line 752 (original), 747-748 (patched)
<https://reviews.apache.org/r/69149/#comment294744>
Ditto.
- Ilya Pronin
On Oct. 24, 2018, 4:49 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69149/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2018, 4:49 p.m.)
>
>
> Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9354
> https://issues.apache.org/jira/browse/MESOS-9354
>
>
> Repository: mesos
>
>
> Description
> -------
>
> To make a bind mount read-only, you have to first make the bind mount,
> then remount it with the read-only flag. This is a bit arcane, which is
> why `mount(8)` does it automatically.
>
> This change updates `fs::mount()` to do the read-only remount
> automatically when it is making a read-only bind mount so that every
> caller doesn't have to carry special code to make it work correctly. All
> the callers that make an explicit remount are updated to simply pass
> the `MS_READONLY` flag if necessary.
>
>
> Diffs
> -----
>
> src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 24c9fd6beb9657b80b33dc31c2939083c1aa9110
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 64271dfbb5c074ad3ac8d2a64c3943d739c0fffa
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp 2e03ef50a290c046ae2b02b332d3d007b572429d
> src/slave/containerizer/mesos/isolators/volume/image.cpp 53cbaefeef7a6e10149e241e07d6e9cb8d510fc9
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 21d9528c23d9142eccec456184b42d085b57d12c
> src/slave/containerizer/mesos/provisioner/backends/bind.cpp 7d564dc7de3e3de8726c7fd42a301d979c4a2574
> src/tests/containerizer/fs_tests.cpp 23cad35b5db81a70b43bec1c1dbafe008c8dd4da
>
>
> Diff: https://reviews.apache.org/r/69149/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>