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/09/10 21:34:08 UTC
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/
-----------------------------------------------------------
(Updated Sept. 10, 2018, 9:34 p.m.)
Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
Bugs: MESOS-5158
https://issues.apache.org/jira/browse/MESOS-5158
Repository: mesos
Description
-------
Added persistent volume support to the `disk/xfs` isolator. This
implementation largely tracks the `disk/du` implementation in that
we now keep a map of paths in each container info structure. We now
defer quota clean up to project ID reclaimation time so that we can
use the same mechanism for sandbox and persistent volume paths.
We explicitly exclude mount disks from XFS project quotas, but we still
track them so that we can correctly publish their usage information in
the container `DiskStatistics` message. This means that mount disks are
not required to be XFS filesystems or have project quotas configured.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/xfs/disk.hpp 38c467b47cb7c04803b0709b8239458fb26abb61
src/slave/containerizer/mesos/isolators/xfs/disk.cpp 783da0407528c044035d18cc59a744353921d64c
src/tests/containerizer/xfs_quota_tests.cpp 2b3a2e25f5075357f090d47826698e7bb6fdf969
Diff: https://reviews.apache.org/r/68401/diff/4/
Changes: https://reviews.apache.org/r/68401/diff/3-4/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by James Peach <jp...@apache.org>.
> On Sept. 20, 2018, 12:59 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.hpp
> > Lines 83 (patched)
> > <https://reviews.apache.org/r/68401/diff/4/?file=2087665#file2087665line83>
> >
> > Nit: `const`?
Fixed.
> On Sept. 20, 2018, 12:59 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.hpp
> > Lines 87 (patched)
> > <https://reviews.apache.org/r/68401/diff/4/?file=2087665#file2087665line87>
> >
> > Nit: We can use parameter names without underscores here - there's no shadowing. Also, `explicit`? Unless we want to use `= {dir, projid}` for it :)
> Nit: We can use parameter names without underscores here - there's no shadowing
Agreed.
> Also, explicit? Unless we want to use = {dir, projid} for it :)
The previous `explicit` was a holdover from when the constructor took only a single argument. I don't think we still need `explicit` since there aren't any implicit conversions.
> On Sept. 20, 2018, 12:59 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 416 (original), 585-587 (patched)
> > <https://reviews.apache.org/r/68401/diff/4/?file=2087666#file2087666line611>
> >
> > Funny how in a case where a volume is shared by multiple containers we can kill all of them because of a single offender. Just thinking out loud. Probably worth noting that in the docs.
Yeh, IIRC the `disk/du` isolator has the same behaviour. Maybe returning a write failure makes more sense for persistent volumes. We could revisit that, though I would be concerned by the extra complexity.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/#review208778
-----------------------------------------------------------
On Sept. 10, 2018, 9:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68401/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2018, 9:34 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added persistent volume support to the `disk/xfs` isolator. This
> implementation largely tracks the `disk/du` implementation in that
> we now keep a map of paths in each container info structure. We now
> defer quota clean up to project ID reclaimation time so that we can
> use the same mechanism for sandbox and persistent volume paths.
>
> We explicitly exclude mount disks from XFS project quotas, but we still
> track them so that we can correctly publish their usage information in
> the container `DiskStatistics` message. This means that mount disks are
> not required to be XFS filesystems or have project quotas configured.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 07e68a777aefba4dd35066f2eb207bba7f199d83
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 2113f8665e4e84bdf12868f605b480522816a9ba
> src/tests/containerizer/xfs_quota_tests.cpp 11dc7f596671d373cd0c9b443a1d217053285a63
>
>
> Diff: https://reviews.apache.org/r/68401/diff/5/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by James Peach <jp...@apache.org>.
> On Sept. 20, 2018, 12:59 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 347-348 (patched)
> > <https://reviews.apache.org/r/68401/diff/4/?file=2087666#file2087666line356>
> >
> > Are we checking this in case multiple volumes have the same project ID? If that can be true, later we may falsely free a project ID that is still in use by the volume that we ignored here. Do we elect to ignore such misconfigurations?
Project IDs must always be assigned uniquely. I turned this into a `CHECK`.
> On Sept. 20, 2018, 12:59 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 484-487 (patched)
> > <https://reviews.apache.org/r/68401/diff/4/?file=2087666#file2087666line509>
> >
> > IMHO this can justify keeping dedicated `directory`, `projectId` and `quota` for container sandbox in `Info` :) But this is just my taste.
Dropping this, since I'd rather not flip it back now :)
> On Sept. 20, 2018, 12:59 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 438-440 (original), 609-611 (patched)
> > <https://reviews.apache.org/r/68401/diff/4/?file=2087666#file2087666line635>
> >
> > Add which quota, i.e. sandbox/volume?
Added the disk into to the resources, and included the path and container ID in a log message.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/#review208778
-----------------------------------------------------------
On Sept. 10, 2018, 9:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68401/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2018, 9:34 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added persistent volume support to the `disk/xfs` isolator. This
> implementation largely tracks the `disk/du` implementation in that
> we now keep a map of paths in each container info structure. We now
> defer quota clean up to project ID reclaimation time so that we can
> use the same mechanism for sandbox and persistent volume paths.
>
> We explicitly exclude mount disks from XFS project quotas, but we still
> track them so that we can correctly publish their usage information in
> the container `DiskStatistics` message. This means that mount disks are
> not required to be XFS filesystems or have project quotas configured.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 07e68a777aefba4dd35066f2eb207bba7f199d83
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 2113f8665e4e84bdf12868f605b480522816a9ba
> src/tests/containerizer/xfs_quota_tests.cpp 11dc7f596671d373cd0c9b443a1d217053285a63
>
>
> Diff: https://reviews.apache.org/r/68401/diff/5/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/#review208778
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/xfs/disk.hpp
Lines 83 (patched)
<https://reviews.apache.org/r/68401/#comment292961>
Nit: `const`?
src/slave/containerizer/mesos/isolators/xfs/disk.hpp
Lines 87 (patched)
<https://reviews.apache.org/r/68401/#comment292958>
Nit: We can use parameter names without underscores here - there's no shadowing. Also, `explicit`? Unless we want to use `= {dir, projid}` for it :)
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 342-345 (patched)
<https://reviews.apache.org/r/68401/#comment292968>
Follow up to the comment below: if multiple volumes can have the same project ID `project_ids_free` metric will be overdecremented here.
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 347-348 (patched)
<https://reviews.apache.org/r/68401/#comment292967>
Are we checking this in case multiple volumes have the same project ID? If that can be true, later we may falsely free a project ID that is still in use by the volume that we ignored here. Do we elect to ignore such misconfigurations?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 484-487 (patched)
<https://reviews.apache.org/r/68401/#comment292971>
IMHO this can justify keeping dedicated `directory`, `projectId` and `quota` for container sandbox in `Info` :) But this is just my taste.
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 529 (patched)
<https://reviews.apache.org/r/68401/#comment292972>
Add volume ID?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 416 (original), 585-587 (patched)
<https://reviews.apache.org/r/68401/#comment292975>
Funny how in a case where a volume is shared by multiple containers we can kill all of them because of a single offender. Just thinking out loud. Probably worth noting that in the docs.
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 421-422 (original), 592-593 (patched)
<https://reviews.apache.org/r/68401/#comment292973>
Log which directory since now we check several of them?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 438-440 (original), 609-611 (patched)
<https://reviews.apache.org/r/68401/#comment292974>
Add which quota, i.e. sandbox/volume?
- Ilya Pronin
On Sept. 10, 2018, 2:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68401/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2018, 2:34 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added persistent volume support to the `disk/xfs` isolator. This
> implementation largely tracks the `disk/du` implementation in that
> we now keep a map of paths in each container info structure. We now
> defer quota clean up to project ID reclaimation time so that we can
> use the same mechanism for sandbox and persistent volume paths.
>
> We explicitly exclude mount disks from XFS project quotas, but we still
> track them so that we can correctly publish their usage information in
> the container `DiskStatistics` message. This means that mount disks are
> not required to be XFS filesystems or have project quotas configured.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 38c467b47cb7c04803b0709b8239458fb26abb61
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 783da0407528c044035d18cc59a744353921d64c
> src/tests/containerizer/xfs_quota_tests.cpp 2b3a2e25f5075357f090d47826698e7bb6fdf969
>
>
> Diff: https://reviews.apache.org/r/68401/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/#review209080
-----------------------------------------------------------
Ship it!
Ship It!
- Chun-Hung Hsiao
On Sept. 27, 2018, 6:02 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68401/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2018, 6:02 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added persistent volume support to the `disk/xfs` isolator. This
> implementation largely tracks the `disk/du` implementation in that
> we now keep a map of paths in each container info structure. We now
> defer quota clean up to project ID reclaimation time so that we can
> use the same mechanism for sandbox and persistent volume paths.
>
> We explicitly exclude mount disks from XFS project quotas, but we still
> track them so that we can correctly publish their usage information in
> the container `DiskStatistics` message. This means that mount disks are
> not required to be XFS filesystems or have project quotas configured.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 38c467b47cb7c04803b0709b8239458fb26abb61
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 783da0407528c044035d18cc59a744353921d64c
> src/tests/containerizer/xfs_quota_tests.cpp 2b3a2e25f5075357f090d47826698e7bb6fdf969
>
>
> Diff: https://reviews.apache.org/r/68401/diff/7/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/
-----------------------------------------------------------
(Updated Sept. 27, 2018, 6:02 p.m.)
Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
Bugs: MESOS-5158
https://issues.apache.org/jira/browse/MESOS-5158
Repository: mesos
Description
-------
Added persistent volume support to the `disk/xfs` isolator. This
implementation largely tracks the `disk/du` implementation in that
we now keep a map of paths in each container info structure. We now
defer quota clean up to project ID reclaimation time so that we can
use the same mechanism for sandbox and persistent volume paths.
We explicitly exclude mount disks from XFS project quotas, but we still
track them so that we can correctly publish their usage information in
the container `DiskStatistics` message. This means that mount disks are
not required to be XFS filesystems or have project quotas configured.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/xfs/disk.hpp 38c467b47cb7c04803b0709b8239458fb26abb61
src/slave/containerizer/mesos/isolators/xfs/disk.cpp 783da0407528c044035d18cc59a744353921d64c
src/tests/containerizer/xfs_quota_tests.cpp 2b3a2e25f5075357f090d47826698e7bb6fdf969
Diff: https://reviews.apache.org/r/68401/diff/7/
Changes: https://reviews.apache.org/r/68401/diff/6-7/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by James Peach <jp...@apache.org>.
> On Sept. 26, 2018, 11:16 p.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 609-610 (original), 627-628 (patched)
> > <https://reviews.apache.org/r/68401/diff/5-6/?file=2090625#file2090625line631>
> >
> > This message can be displayed by the scheduler (e.g. Aurora) to its users. They will ask which quota this is. It might be worth adding some indication if this was a persistent volume quota.
I added the persistent volume ID to the message.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/#review209057
-----------------------------------------------------------
On Sept. 24, 2018, 4:58 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68401/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 4:58 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added persistent volume support to the `disk/xfs` isolator. This
> implementation largely tracks the `disk/du` implementation in that
> we now keep a map of paths in each container info structure. We now
> defer quota clean up to project ID reclaimation time so that we can
> use the same mechanism for sandbox and persistent volume paths.
>
> We explicitly exclude mount disks from XFS project quotas, but we still
> track them so that we can correctly publish their usage information in
> the container `DiskStatistics` message. This means that mount disks are
> not required to be XFS filesystems or have project quotas configured.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 38c467b47cb7c04803b0709b8239458fb26abb61
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 783da0407528c044035d18cc59a744353921d64c
> src/tests/containerizer/xfs_quota_tests.cpp 2b3a2e25f5075357f090d47826698e7bb6fdf969
>
>
> Diff: https://reviews.apache.org/r/68401/diff/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/#review209057
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 609-610 (original), 627-628 (patched)
<https://reviews.apache.org/r/68401/#comment293323>
This message can be displayed by the scheduler (e.g. Aurora) to its users. They will ask which quota this is. It might be worth adding some indication if this was a persistent volume quota.
- Ilya Pronin
On Sept. 24, 2018, 9:58 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68401/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2018, 9:58 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added persistent volume support to the `disk/xfs` isolator. This
> implementation largely tracks the `disk/du` implementation in that
> we now keep a map of paths in each container info structure. We now
> defer quota clean up to project ID reclaimation time so that we can
> use the same mechanism for sandbox and persistent volume paths.
>
> We explicitly exclude mount disks from XFS project quotas, but we still
> track them so that we can correctly publish their usage information in
> the container `DiskStatistics` message. This means that mount disks are
> not required to be XFS filesystems or have project quotas configured.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 38c467b47cb7c04803b0709b8239458fb26abb61
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 783da0407528c044035d18cc59a744353921d64c
> src/tests/containerizer/xfs_quota_tests.cpp 2b3a2e25f5075357f090d47826698e7bb6fdf969
>
>
> Diff: https://reviews.apache.org/r/68401/diff/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 68401: Added persistent volume support to the
`disk/xfs` isolator.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68401/
-----------------------------------------------------------
(Updated Sept. 24, 2018, 4:58 p.m.)
Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-5158
https://issues.apache.org/jira/browse/MESOS-5158
Repository: mesos
Description
-------
Added persistent volume support to the `disk/xfs` isolator. This
implementation largely tracks the `disk/du` implementation in that
we now keep a map of paths in each container info structure. We now
defer quota clean up to project ID reclaimation time so that we can
use the same mechanism for sandbox and persistent volume paths.
We explicitly exclude mount disks from XFS project quotas, but we still
track them so that we can correctly publish their usage information in
the container `DiskStatistics` message. This means that mount disks are
not required to be XFS filesystems or have project quotas configured.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/xfs/disk.hpp 38c467b47cb7c04803b0709b8239458fb26abb61
src/slave/containerizer/mesos/isolators/xfs/disk.cpp 783da0407528c044035d18cc59a744353921d64c
src/tests/containerizer/xfs_quota_tests.cpp 2b3a2e25f5075357f090d47826698e7bb6fdf969
Diff: https://reviews.apache.org/r/68401/diff/6/
Changes: https://reviews.apache.org/r/68401/diff/5-6/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach