mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 71194: Add `disk/xfs` isolator support for ephemeral volumes.
Date Fri, 02 Aug 2019 02:27:32 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message