You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2019/08/02 22:17:52 UTC
Re: Review Request 71193: Supported multiple quota paths in the
`disk/xfs` isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217059
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 570 (original), 566 (patched)
<https://reviews.apache.org/r/71193/#comment304303>
You are not changing the logic here but could you remind me why this error doesn't fail the update?
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 776 (patched)
<https://reviews.apache.org/r/71193/#comment304304>
For `hashmap` there's already a `contains` defined.
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 778-779 (patched)
<https://reviews.apache.org/r/71193/#comment304305>
You could've used a `else if` followed by an `else` to avoid another level of nesting.
src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 807 (original)
<https://reviews.apache.org/r/71193/#comment304306>
We could move the logging about the `dir` up above `erase` if we still want to log it (maybe for `VLOG(1)`)?
- Jiang Yan Xu
On July 30, 2019, 12:52 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71193/
> -----------------------------------------------------------
>
> (Updated July 30, 2019, 12:52 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
> -------
>
> The `disk/xfs` isolator assumed that there would only be a single
> directory path for each project quota. When we apply project quotas
> to the overlayfs upperdir, that won't be true any more, since the
> upperdir will come under the same quota as the task sandbox.
>
> Update the quota reclamation tracking to keep a set of disk paths that
> the quota has been applied to, and only reclaim the project ID once all
> those paths have been removed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 94d44e7f39dc01037461015b499a1fc3169b24e8
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
>
>
> Diff: https://reviews.apache.org/r/71193/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71193: Supported multiple quota paths in the
`disk/xfs` isolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Aug. 2, 2019, 3:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 570 (original), 566 (patched)
> > <https://reviews.apache.org/r/71193/diff/3/?file=2158799#file2158799line570>
> >
> > You are not changing the logic here but could you remind me why this error doesn't fail the update?
>
> James Peach wrote:
> I think that we are just trying to be robust against host problems. This would only fail if there was a serious disk error, so we just try to maintain our internal consistency.
If it's btween continuing without being able to enforce quota and failing task + inititating clean up, I'd say the latter is better protecting consistency?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217059
-----------------------------------------------------------
On Aug. 5, 2019, 2:17 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71193/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2019, 2:17 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
> -------
>
> The `disk/xfs` isolator assumed that there would only be a single
> directory path for each project quota. When we apply project quotas
> to the overlayfs upperdir, that won't be true any more, since the
> upperdir will come under the same quota as the task sandbox.
>
> Update the quota reclamation tracking to keep a set of disk paths that
> the quota has been applied to, and only reclaim the project ID once all
> those paths have been removed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 94d44e7f39dc01037461015b499a1fc3169b24e8
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
>
>
> Diff: https://reviews.apache.org/r/71193/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71193: Supported multiple quota paths in the
`disk/xfs` isolator.
Posted by James Peach <jp...@apache.org>.
> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 570 (original), 566 (patched)
> > <https://reviews.apache.org/r/71193/diff/3/?file=2158799#file2158799line570>
> >
> > You are not changing the logic here but could you remind me why this error doesn't fail the update?
I think that we are just trying to be robust against host problems. This would only fail if there was a serious disk error, so we just try to maintain our internal consistency.
> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 776 (patched)
> > <https://reviews.apache.org/r/71193/diff/3/?file=2158799#file2158799line796>
> >
> > For `hashmap` there's already a `contains` defined.
I did this to avoid doing multiple hash lookups, but I agree that using `contains` improves the readability.
> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 778-779 (patched)
> > <https://reviews.apache.org/r/71193/diff/3/?file=2158799#file2158799line798>
> >
> > You could've used a `else if` followed by an `else` to avoid another level of nesting.
Aftew switching to `contains`, I think that this makes the most sense as is.
> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 807 (original)
> > <https://reviews.apache.org/r/71193/diff/3/?file=2158799#file2158799line841>
> >
> > We could move the logging about the `dir` up above `erase` if we still want to log it (maybe for `VLOG(1)`)?
Yeh, I think adding a `VLOG` makes sense here.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217059
-----------------------------------------------------------
On July 30, 2019, 7:52 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71193/
> -----------------------------------------------------------
>
> (Updated July 30, 2019, 7:52 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
> -------
>
> The `disk/xfs` isolator assumed that there would only be a single
> directory path for each project quota. When we apply project quotas
> to the overlayfs upperdir, that won't be true any more, since the
> upperdir will come under the same quota as the task sandbox.
>
> Update the quota reclamation tracking to keep a set of disk paths that
> the quota has been applied to, and only reclaim the project ID once all
> those paths have been removed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 94d44e7f39dc01037461015b499a1fc3169b24e8
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
>
>
> Diff: https://reviews.apache.org/r/71193/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 71193: Supported multiple quota paths in the
`disk/xfs` isolator.
Posted by James Peach <jp...@apache.org>.
> On Aug. 2, 2019, 10:17 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 570 (original), 566 (patched)
> > <https://reviews.apache.org/r/71193/diff/3/?file=2158799#file2158799line570>
> >
> > You are not changing the logic here but could you remind me why this error doesn't fail the update?
>
> James Peach wrote:
> I think that we are just trying to be robust against host problems. This would only fail if there was a serious disk error, so we just try to maintain our internal consistency.
>
> Jiang Yan Xu wrote:
> If it's btween continuing without being able to enforce quota and failing task + inititating clean up, I'd say the latter is better protecting consistency?
If we change the semantics of this, it should be in a separate review with some analysis. Let's not mix our concerns.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217059
-----------------------------------------------------------
On Aug. 5, 2019, 9:17 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71193/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2019, 9:17 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
> -------
>
> The `disk/xfs` isolator assumed that there would only be a single
> directory path for each project quota. When we apply project quotas
> to the overlayfs upperdir, that won't be true any more, since the
> upperdir will come under the same quota as the task sandbox.
>
> Update the quota reclamation tracking to keep a set of disk paths that
> the quota has been applied to, and only reclaim the project ID once all
> those paths have been removed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/xfs/disk.hpp 94d44e7f39dc01037461015b499a1fc3169b24e8
> src/slave/containerizer/mesos/isolators/xfs/disk.cpp 646330c65b24aa28801ec99d7909db08a3e05c79
>
>
> Diff: https://reviews.apache.org/r/71193/diff/4/
>
>
> Testing
> -------
>
> sudo make check (Frdora 30)
>
>
> Thanks,
>
> James Peach
>
>