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 Wed, 07 Aug 2019 02:36:36 GMT


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


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