mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 51031: Added non-recursive version of `cgroups::get`.
Date Tue, 25 Oct 2016 19:28:31 GMT

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




src/tests/containerizer/cgroups_tests.cpp (lines 135 - 150)
<https://reviews.apache.org/r/51031/#comment223279>

    We might need to cleanup this part. It's likely that `baseHierarchy` is not the parent
of all the subsystems. Imagine a subsystem is not mounted initially. And we'll use TEST_CGROUPS_HIERARCHY
as the base. However, if all other subsystems are mounted under /sys/fs/cgroup, then this
test fixture will fail.
    
    We can address that in the separate patch, but I'd like to cleanup this tech debt.



src/tests/containerizer/cgroups_tests.cpp (lines 169 - 177)
<https://reviews.apache.org/r/51031/#comment223281>

    Looks like we can just call cgroups::destroy(TEST_CGROUPS_ROOT) here which recursively
kills cgroups?



src/tests/containerizer/cgroups_tests.cpp (lines 184 - 197)
<https://reviews.apache.org/r/51031/#comment223280>

    Looks like no one will try to cleanup TEST_CGROUPS_HIERARCHY? Again, I feel that we should
fix this test fixture.



src/tests/containerizer/cgroups_tests.cpp (lines 187 - 195)
<https://reviews.apache.org/r/51031/#comment223282>

    Ditto. Is this just recursive cgroups::destroy?



src/tests/containerizer/cgroups_tests.cpp (line 386)
<https://reviews.apache.org/r/51031/#comment223284>

    I'd prefer we declare them at the top:
    ```
    const string cgroup1 = path::join(...);
    const string cgroup2 = path::join(...);
    const string cgroup3 = path::join(...);
    ...
    ASSERT_SOME(cgroups::create(hierarchy, cgroup1, true));
    ...
    ```



src/tests/containerizer/cgroups_tests.cpp (lines 406 - 440)
<https://reviews.apache.org/r/51031/#comment223283>

    I'd prefer we convert cgroups into a hashset and use cgroups.contains here.


- Jie Yu


On Oct. 24, 2016, 2:39 p.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51031/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2016, 2:39 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, Zhengju Sha, and Jiang Yan
Xu.
> 
> 
> Bugs: MESOS-6035
>     https://issues.apache.org/jira/browse/MESOS-6035
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> In some cases, we just want to get the children cgroups instead of
> retrieve descendant cgroups recursively. We added an argument to
> `cgroups::get` to indicate whether to retrieve cgroups recursively but
> made recursive retrieve the default behaviour. This patch fixed some
> incorrect `TEST_CGROUPS_ROOT` checks as well.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.hpp cfce09cb57501f2c988a8d997d7c6150280ed53d 
>   src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 
>   src/tests/containerizer/cgroups_tests.cpp 0afaec6ae948cabf1472bf01103210d8f9809cb1

> 
> Diff: https://reviews.apache.org/r/51031/diff/
> 
> 
> Testing
> -------
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*" --verbose
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


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