You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2019/08/01 14:15:26 UTC
Re: 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/#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
>
>
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
>
>