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 2019/07/30 07:52:01 UTC
Review Request 71194: Add `disk/xfs` isolator support for ephemeral
volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/
-----------------------------------------------------------
Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
Bugs: MESOS-9900
https://issues.apache.org/jira/browse/MESOS-9900
Repository: mesos
Description
-------
Add support for labeling ephemeral volumes with the sandbox XFS
project ID. This makes changes to the container rootfs share the
same disk quota as the sandbox.
Diffs
-----
src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
Diff: https://reviews.apache.org/r/71194/diff/1/
Testing
-------
sudo make check (Frdora 30)
Thanks,
James Peach
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/
-----------------------------------------------------------
(Updated Aug. 7, 2019, 2:59 a.m.)
Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-9900
https://issues.apache.org/jira/browse/MESOS-9900
Repository: mesos
Description
-------
Add support for labeling ephemeral volumes with the sandbox XFS
project ID. This makes changes to the container rootfs share the
same disk quota as the sandbox.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
Diff: https://reviews.apache.org/r/71194/diff/4/
Changes: https://reviews.apache.org/r/71194/diff/3-4/
Testing
-------
sudo make check (Frdora 30)
Thanks,
James Peach
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/#review217103
-----------------------------------------------------------
Ship it!
Ship It!
- Andrei Budnik
On Авг. 2, 2019, 2:27 д.п., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> -----------------------------------------------------------
>
> (Updated Авг. 2, 2019, 2:27 д.п.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
> src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
> src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
>
>
> Diff: https://reviews.apache.org/r/71194/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by James Peach <jp...@apache.org>.
> On Aug. 2, 2019, 11:03 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 342 (patched)
> > <https://reviews.apache.org/r/71194/diff/3/?file=2158813#file2158813line342>
> >
> > Use `at()`?
Yup, updating to use `at()` everywhere.
> On Aug. 2, 2019, 11:03 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 346 (patched)
> > <https://reviews.apache.org/r/71194/diff/3/?file=2158813#file2158813line346>
> >
> > There are three experessions `foreachpair` so if we break them apart, put each one a line?
Fixed.
> On Aug. 2, 2019, 11:03 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 423 (patched)
> > <https://reviews.apache.org/r/71194/diff/3/?file=2158813#file2158813line423>
> >
> > Do we need to single out the overlay backend? Seems like we can just scan all backends?
That would have been an alternative. I made it specific to overlay to make it clear this this is (currently) only an overlay feature. If we present this as more general than it really is, this will be less obvious to future maintainers.
> On Aug. 2, 2019, 11:03 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 424 (patched)
> > <https://reviews.apache.org/r/71194/diff/3/?file=2158813#file2158813line424>
> >
> > FWIW sandbox scanning predates the container run state, now it does look like at least sandboxes (or any paths that can be deduced from the container ID) would be covered given it's checkpointed pretty early: https://github.com/apache/mesos/blob/e1176c453d04a8ef8f53cf23928b5bbb09173d78/src/slave/containerizer/mesos/containerizer.cpp#L1547
> >
> > but of course the emphemeral volumes are added later than that and there could be dirs with projectIDs set before `ephemeral_volumes` is persisted.
>
> Jiang Yan Xu wrote:
> Sorry I meant to add that the provisioner dir for a container can also be [deduced](https://github.com/apache/mesos/blob/e1176c453d04a8ef8f53cf23928b5bbb09173d78/src/slave/containerizer/mesos/provisioner/paths.hpp#L38) from the containerID so you can just scan the subdirs of the containers recovered.
>
> Up to you.
I think that you are suggesting that we could just scan directly, without adding a backend helper API? I added to helper API to make it clear that this was being done. If we glob directly from the isolator, the dependency may not be obvious to people changing the provisioner backends.
> On Aug. 2, 2019, 11:03 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 517 (patched)
> > <https://reviews.apache.org/r/71194/diff/3/?file=2158813#file2158813line517>
> >
> > Same question: why don't we fail here?
Yeh, looking at the call sites, it is probably reasonable to fail when `scheduleProjectRoot` fails. This basically means that somethiing terrible is wrong with disk quota and we are alreaady failing on `setProjectId`
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/#review217058
-----------------------------------------------------------
On Aug. 2, 2019, 2:27 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2019, 2:27 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
> src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
> src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
>
>
> Diff: https://reviews.apache.org/r/71194/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Aug. 2, 2019, 4:03 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 424 (patched)
> > <https://reviews.apache.org/r/71194/diff/3/?file=2158813#file2158813line424>
> >
> > FWIW sandbox scanning predates the container run state, now it does look like at least sandboxes (or any paths that can be deduced from the container ID) would be covered given it's checkpointed pretty early: https://github.com/apache/mesos/blob/e1176c453d04a8ef8f53cf23928b5bbb09173d78/src/slave/containerizer/mesos/containerizer.cpp#L1547
> >
> > but of course the emphemeral volumes are added later than that and there could be dirs with projectIDs set before `ephemeral_volumes` is persisted.
Sorry I meant to add that the provisioner dir for a container can also be [deduced](https://github.com/apache/mesos/blob/e1176c453d04a8ef8f53cf23928b5bbb09173d78/src/slave/containerizer/mesos/provisioner/paths.hpp#L38) from the containerID so you can just scan the subdirs of the containers recovered.
Up to you.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/#review217058
-----------------------------------------------------------
On Aug. 1, 2019, 7:27 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2019, 7:27 p.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
> src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
> src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
>
>
> Diff: https://reviews.apache.org/r/71194/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/#review217058
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 342 (patched)
<https://reviews.apache.org/r/71194/#comment304302>
Use `at()`?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 346 (patched)
<https://reviews.apache.org/r/71194/#comment304307>
There are three experessions `foreachpair` so if we break them apart, put each one a line?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 347 (patched)
<https://reviews.apache.org/r/71194/#comment304308>
s/container volumes/persistent volumes/?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 423 (patched)
<https://reviews.apache.org/r/71194/#comment304310>
Do we need to single out the overlay backend? Seems like we can just scan all backends?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 424 (patched)
<https://reviews.apache.org/r/71194/#comment304309>
FWIW sandbox scanning predates the container run state, now it does look like at least sandboxes (or any paths that can be deduced from the container ID) would be covered given it's checkpointed pretty early: https://github.com/apache/mesos/blob/e1176c453d04a8ef8f53cf23928b5bbb09173d78/src/slave/containerizer/mesos/containerizer.cpp#L1547
but of course the emphemeral volumes are added later than that and there could be dirs with projectIDs set before `ephemeral_volumes` is persisted.
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 517 (patched)
<https://reviews.apache.org/r/71194/#comment304311>
Same question: why don't we fail here?
- Jiang Yan Xu
On Aug. 1, 2019, 7:27 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2019, 7:27 p.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
> src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
> src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
>
>
> Diff: https://reviews.apache.org/r/71194/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/
-----------------------------------------------------------
(Updated Aug. 2, 2019, 2:27 a.m.)
Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-9900
https://issues.apache.org/jira/browse/MESOS-9900
Repository: mesos
Description
-------
Add support for labeling ephemeral volumes with the sandbox XFS
project ID. This makes changes to the container rootfs share the
same disk quota as the sandbox.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
Diff: https://reviews.apache.org/r/71194/diff/3/
Changes: https://reviews.apache.org/r/71194/diff/2-3/
Testing
-------
sudo make check (Frdora 30)
Thanks,
James Peach
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by James Peach <jp...@apache.org>.
> On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 335 (patched)
> > <https://reviews.apache.org/r/71194/diff/1/?file=2158186#file2158186line335>
> >
> > According to this comment https://github.com/apache/mesos/blob/d8155f8125e38d145d280331146b934c2bb7c842/src/slave/containerizer/mesos/isolators/xfs/disk.cpp#L294-L301, this condition can only be met for containers that were launched before enabling XFS-isolator? If so, should we use `WARNING` here?
Yep, I agree that `WARNING` seems appropriate here.
> On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 425-426 (patched)
> > <https://reviews.apache.org/r/71194/diff/1/?file=2158186#file2158186line425>
> >
> > Why doesn't `ephemeral_volumes` belonging to the `states` cover all directories? What is the case when we need this?
I'm not fully convinced that overlayfs directories are guaranteed to be in `ContainerState` at recovery time. For example, [here](https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1073) we might find that we have an unrecoverable container what won't get a container state, and [here](https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1060), we might find the `ContainerState` erroneously missing the ephemeral volumes because the config could not be read.
So I felt that we should follow the same strategy as for sandboxes and treat the on-disk state as the source of truth. The consequences of leaking a project ID here are most likely that we would assign a project ID that is in use to a new task and that task could be immediately out of quota. This window should be small since the overlay backend is cleaned up immediately, but it's best to avoid the possibility.
> On Aug. 1, 2019, 2:15 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/provisioner/backends/overlay.cpp
> > Lines 17 (patched)
> > <https://reviews.apache.org/r/71194/diff/1/?file=2158188#file2158188line17>
> >
> > Move it to the line after `#include "linux/fs.hpp"` (closer to the `#include "slave/containerizer/mesos/provisioner/constants.hpp"`)?
The counterpart header for the source file should come first since that helps to ensure that the header is self-contained. It's not really spelled out in the style guide, but the example does it:
https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#order-of-includes
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/#review217030
-----------------------------------------------------------
On Aug. 2, 2019, 2:27 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> -----------------------------------------------------------
>
> (Updated Aug. 2, 2019, 2:27 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
> src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
> src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
>
>
> Diff: https://reviews.apache.org/r/71194/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71194: Add `disk/xfs` isolator support for
ephemeral volumes.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71194/#review217030
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 335 (patched)
<https://reviews.apache.org/r/71194/#comment304253>
According to this comment https://github.com/apache/mesos/blob/d8155f8125e38d145d280331146b934c2bb7c842/src/slave/containerizer/mesos/isolators/xfs/disk.cpp#L294-L301, this condition can only be met for containers that were launched before enabling XFS-isolator? If so, should we use `WARNING` here?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 425-426 (patched)
<https://reviews.apache.org/r/71194/#comment304255>
Why doesn't `ephemeral_volumes` belonging to the `states` cover all directories? What is the case when we need this?
src/slave/containerizer/mesos/provisioner/backends/overlay.cpp
Lines 17 (patched)
<https://reviews.apache.org/r/71194/#comment304256>
Move it to the line after `#include "linux/fs.hpp"` (closer to the `#include "slave/containerizer/mesos/provisioner/constants.hpp"`)?
- Andrei Budnik
On Июль 30, 2019, 7:52 д.п., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71194/
> -----------------------------------------------------------
>
> (Updated Июль 30, 2019, 7:52 д.п.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9900
> https://issues.apache.org/jira/browse/MESOS-9900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add support for labeling ephemeral volumes with the sandbox XFS
> project ID. This makes changes to the container rootfs share the
> same disk quota as the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
> src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 362e02172d2fd8e6e241fb6f5689f569ba74a0d1
> src/slave/containerizer/mesos/provisioner/backends/overlay.cpp f2040cf36c601a13281a78ff844ebd41000a2d65
>
>
> Diff: https://reviews.apache.org/r/71194/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>