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 2017/02/15 23:21:42 UTC
Review Request 56733: Mount all supported subsystems in the
containerizer tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/
-----------------------------------------------------------
Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
Bugs: MESOS-7049
https://issues.apache.org/jira/browse/MESOS-7049
Repository: mesos
Description
-------
Rather than a hard-coded list of subsystems, just mount everything we
find that is supported.
Diffs
-----
src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
Diff: https://reviews.apache.org/r/56733/diff/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56733: Mount all supported subsystems in the
containerizer tests.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On March 7, 2017, 11:28 a.m., Jiang Yan Xu wrote:
> > So I was thinking about this: the code here makes sure the OS has the minimum set of subsystems required by this test suite. If the system doesn't meet the requirement, we do want to terminate here. On the other hand, although mounting more subsystems isn't harmful, if we mount everything that comes out of `cgroups::subsystems()`, we should still check whether that list is a superset of the minimumally required set. At a result we still have to add items here every time a new subsystem is added so we are not saving much?
> >
> > Thoughts?
>
> James Peach wrote:
> I think you are reading too much into this. Is this really a contract about what subsystems the tests require? I'd argue that it is just an arbitrary selection from history. Mounting everything is robust and requires no future maintenance. Tests that need to verify specific subsystems are present can do that.
Thanks. I think you are right. There's not a minimum set, `perf_event` probably shouldn't be there at all since we actually filter on it.
I'll commit this but as a separate dicussion, I wonder if the `SetUp()` is needed at all? /cc @haosdent
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/#review168173
-----------------------------------------------------------
On March 6, 2017, 11:34 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56733/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 11:34 a.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than a hard-coded list of subsystems, just mount everything we
> find that is supported.
>
>
> Diffs
> -----
>
> src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
>
>
> Diff: https://reviews.apache.org/r/56733/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56733: Mount all supported subsystems in the
containerizer tests.
Posted by James Peach <jp...@apache.org>.
> On March 7, 2017, 7:28 p.m., Jiang Yan Xu wrote:
> > So I was thinking about this: the code here makes sure the OS has the minimum set of subsystems required by this test suite. If the system doesn't meet the requirement, we do want to terminate here. On the other hand, although mounting more subsystems isn't harmful, if we mount everything that comes out of `cgroups::subsystems()`, we should still check whether that list is a superset of the minimumally required set. At a result we still have to add items here every time a new subsystem is added so we are not saving much?
> >
> > Thoughts?
I think you are reading too much into this. Is this really a contract about what subsystems the tests require? I'd argue that it is just an arbitrary selection from history. Mounting everything is robust and requires no future maintenance. Tests that need to verify specific subsystems are present can do that.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/#review168173
-----------------------------------------------------------
On March 6, 2017, 7:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56733/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 7:34 p.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than a hard-coded list of subsystems, just mount everything we
> find that is supported.
>
>
> Diffs
> -----
>
> src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
>
>
> Diff: https://reviews.apache.org/r/56733/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56733: Mount all supported subsystems in the
containerizer tests.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/#review168173
-----------------------------------------------------------
So I was thinking about this: the code here makes sure the OS has the minimum set of subsystems required by this test suite. If the system doesn't meet the requirement, we do want to terminate here. On the other hand, although mounting more subsystems isn't harmful, if we mount everything that comes out of `cgroups::subsystems()`, we should still check whether that list is a superset of the minimumally required set. At a result we still have to add items here every time a new subsystem is added so we are not saving much?
Thoughts?
- Jiang Yan Xu
On March 6, 2017, 11:34 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56733/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 11:34 a.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than a hard-coded list of subsystems, just mount everything we
> find that is supported.
>
>
> Diffs
> -----
>
> src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
>
>
> Diff: https://reviews.apache.org/r/56733/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56733: Mount all supported subsystems in the
containerizer tests.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/#review168084
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56730, 56731, 56732, 56733]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 6, 2017, 7:34 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56733/
> -----------------------------------------------------------
>
> (Updated March 6, 2017, 7:34 p.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than a hard-coded list of subsystems, just mount everything we
> find that is supported.
>
>
> Diffs
> -----
>
> src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
>
>
> Diff: https://reviews.apache.org/r/56733/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56733: Mount all supported subsystems in the
containerizer tests.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/
-----------------------------------------------------------
(Updated March 6, 2017, 7:34 p.m.)
Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-7049
https://issues.apache.org/jira/browse/MESOS-7049
Repository: mesos
Description
-------
Rather than a hard-coded list of subsystems, just mount everything we
find that is supported.
Diffs (updated)
-----
src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
Diff: https://reviews.apache.org/r/56733/diff/2/
Changes: https://reviews.apache.org/r/56733/diff/1-2/
Testing
-------
sudo make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 56733: Mount all supported subsystems in the
containerizer tests.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/#review167771
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/mesos.cpp
Line 649 (original), 649 (patched)
<https://reviews.apache.org/r/56733/#comment239736>
s/std:://?
I wouldn't worry about being consistent with some of the variables in this file. It's already inconsistent and should be fixed.
- Jiang Yan Xu
On Feb. 15, 2017, 3:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56733/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 3:21 p.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than a hard-coded list of subsystems, just mount everything we
> find that is supported.
>
>
> Diffs
> -----
>
> src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
>
>
> Diff: https://reviews.apache.org/r/56733/diff/1/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 56733: Mount all supported subsystems in the
containerizer tests.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56733/#review165792
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56730, 56731, 56732, 56733]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 15, 2017, 11:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56733/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2017, 11:21 p.m.)
>
>
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
>
>
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than a hard-coded list of subsystems, just mount everything we
> find that is supported.
>
>
> Diffs
> -----
>
> src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb
>
> Diff: https://reviews.apache.org/r/56733/diff/
>
>
> Testing
> -------
>
> sudo make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>