mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From haosdent huang <haosd...@gmail.com>
Subject Re: Review Request 53253: Fixed the bug when search base hierarchy in `cgroups_tests.cpp`.
Date Wed, 02 Nov 2016 04:40:52 GMT


> On Nov. 1, 2016, 1:20 a.m., Jiang Yan Xu wrote:
> > We unfortunately have to copy the cleanup code from `ContainerizerTest<slave::MesosContainerizer>::SetUp()`
to here. FWIW the test fixture originally was written with co-mounted subsystems in a single
hierarchy (hence *Any*), later altered when Mesos started to use separate hierarchies for
each subsystem, but not in a clean way IMO.
> > 
> > The `CgroupsAnyHierarchyTest` fixture and its children AFAICT still works with one
hierarchy with multiple co-mounted subsystems. In fact Mesos does support co-mounted subsystems.
> > 
> > So IMO it wouldn't be a bad idea to have tests in cgroups_tests.cpp to only **create**
one hierarchy for all the specified subsystems (most tests assume one). Of course it's fine
if the subsystems are already mounted in separate hierarchies but the tests only manages (create/cleanup)
a single hierachy.
> > 
> > This would make fixes for related problems (e.g., MESOS-6422) cleaner as well.
> > 
> > Thoughts?
> 
> haosdent huang wrote:
>     To be honest, I prefer to follow what Linux do now which mount subsystems in different
folders. Suppose the Linux have already mounted `/sys/fs/cgroups/cpu`, `/sys/fs/cgroups/memory`.
And in our test case, we create `/sys/fs/cgroups/unified-mount` and mount rest subsystems(`devices`,
`net_cls`, `perf_event`) on it. Then it looks wried. If we choose to mount them separately
in our test cases, e.g, `/sys/fs/cgroups/devices`, `/sys/fs/cgroups/perf_event` and etc. This
is more consistent with current operate system behaviour.
>     
>     Co-mounted all hierarchies simplfy work if there is not any exist subsystems mounted
in current machine. But it would become tricky if some subystems have been mounted. Anything
I miss here?
> 
> Jiang Yan Xu wrote:
>     Let me clarify:
>     
>     1. Mesos works with co-mounted subsystems: https://github.com/apache/mesos/commit/31337348cef29719890bffb59fbf8df8b18b39d0
>     2. At the abstraction level of Mesos agent and Mesos containerizer, it's OK if we
**require** separately mounted subsystems in tests since it's what we **currently** prefer.
>     3. However, at the abstraction level of linux/cgroups.cpp and cgroups_tests.cpp,
it's **OK** for the tests to **create** a single hierarchy (so it only needs to cleanup one)
but **allow** existing hierarhies (e.g., `/sys/fs/cgroup/*`) with single subsystems because
neither the logic in linux/cgroups.cpp nor cgroups_tests.cpp require separately mounted subsystems;
that's above its abstraction level; linux/cgroups.cpp is just a utility and as such doesn't
have preferences on this.
>     
>     The benefit is the simplicity in cleanups in cgroups_tests.cpp: you only need to
recursively destroy `TEST_CGROUPS_ROOT` and you clean up `TEST_CGROUPS_HIERARCHY` (because
it's always a hierarchy and never a base hierarchy) and you are done. If `CgroupsAnyHierarchyTest`
and its descendants ever use existing hierarhies (e.g., `/sys/fs/cgroup/*`), easy, just destroy
`TEST_CGROUPS_ROOT`. It would be cleaner than copying the code over from mesos.cpp and MESOS-6422
is fixed as well.

Suppose we have mount `cpu` and `memory` subsystems, and we choose the single hierarchy way
in our test cases.
Then we still need to find out the base hierarchy first and use it to create `TEST_CGROUPS_HIERARCHY`
under that base hierarchy.
Because we don't support multiple cgroups base hierarchies now. Is this fine? If so, I would
update the patch chain and fix MESOS-6422 together.


- haosdent


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53253/#review154358
-----------------------------------------------------------


On Oct. 28, 2016, 5:13 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53253/
> -----------------------------------------------------------
> 
> (Updated Oct. 28, 2016, 5:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6035
>     https://issues.apache.org/jira/browse/MESOS-6035
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fixed the bug when search base hierarchy in `cgroups_tests.cpp`.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1

> 
> Diff: https://reviews.apache.org/r/53253/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


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