Review Request 56733: Mount all supported subsystems in the containerizer tests.

[James Peach]

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

[Mesos Reviewbot]

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

Re: Review Request 56733: Mount all supported subsystems in the containerizer tests.

[Jiang Yan Xu]

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


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.

[James Peach]

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

[Mesos Reviewbot]

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

[Jiang Yan Xu]

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

[James Peach]

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

[Jiang Yan Xu]

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