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:15 UTC
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/
-----------------------------------------------------------
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/1/
Testing
-------
sudo make check (Frdora 30)
Thanks,
James Peach
Re: Review Request 71193: Supported multiple quota paths in the
`disk/xfs` isolator.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71193/#review217001
-----------------------------------------------------------
Ship it!
Ship It!
- 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/71193/
> -----------------------------------------------------------
>
> (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
> -------
>
> 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/1/
>
>
> 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>.
-----------------------------------------------------------
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.
Changes
-------
Address review feedback.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/71193/diff/3-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 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?
>
> 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
>
>
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 Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>