You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2012/09/28 07:33:45 UTC
Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Description
-------
The recent refactor changes break the assumptions in the cgroups code.
Diffs
-----
src/linux/cgroups.cpp cdafe6e
third_party/libprocess/include/stout/os.hpp 13dbc71
Diff: https://reviews.apache.org/r/7338/diff/
Testing
-------
make check.
Tested on my vm (latest ubuntu 12.04)
Thanks,
Jie Yu
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Benjamin Mahler <bm...@twitter.com>.
CHECKs sound good to me.
On Fri, Sep 28, 2012 at 1:49 PM, Jie Yu <yu...@gmail.com> wrote:
> two possible solutions:
>
> 1) have a CHECK when cgroup initializing.
>
> 2) disable cgroup in configure if the kernel version is too old.
>
> I liked (1) because the runtime environment might be different from the
> compile environment.
>
> - Jie
>
> On Fri, Sep 28, 2012 at 4:38 PM, Benjamin Mahler <bm...@twitter.com>wrote:
>
>> I also like the tests being red if it's not available, like you said it
>> indicates that they shouldn't use cgroups.
>>
>> However, we should really have a way to _prevent_ them from using cgroups
>> if it's not available, have we thought about this yet?
>>
>>
>> On Fri, Sep 28, 2012 at 12:29 PM, Jie Yu <yu...@gmail.com> wrote:
>>
>>> That's the reason. Try to install a newer version of ubuntu (e.g. ubuntu
>>> 11.10, 12.04) so that you can test the oom_control functionality.
>>>
>>> Probably we should disable these tests if oom_control is not available.
>>> But seems that gtest does not provide a clean way to do this (dynamically
>>> disable a test).
>>>
>>> We can add another PREFIX to those test cases (like ROOT_CGROUP), but
>>> that seems to be tedious.
>>>
>>> Also, the break of these tests indicates a good thing: we do require
>>> oom_control capability in cgroups isolation module. If they cannot pass the
>>> unit test, they should not use the cgroups module.
>>>
>>> - Jie
>>>
>>>
>>> On Fri, Sep 28, 2012 at 3:23 PM, Benjamin Mahler <bm...@twitter.com>wrote:
>>>
>>>> $ uname -a
>>>> Linux ubuntu 2.6.32-38-generic #83-Ubuntu SMP Wed Jan 4 11:12:07 UTC
>>>> 2012 x86_64 GNU/Linux
>>>>
>>>> On Fri, Sep 28, 2012 at 12:22 PM, Jie Yu <yu...@gmail.com> wrote:
>>>>
>>>>>
>>>>>
>>>>> > On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
>>>>> > > I've patched this in and I'm seeing a a failing test:
>>>>> > >
>>>>> > > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
>>>>> > > ../../src/tests/cgroups_tests.cpp:365: Failure
>>>>> > > Value of: cgroups::writeControl(hierarchy, "/prof",
>>>>> "memory.oom_control", "1").isSome()
>>>>> > > Actual: false
>>>>> > > Expected: true
>>>>> > > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>>>>>
>>>>> What's your kernel version? Please do a 'uname -a'.
>>>>>
>>>>> We saw this before, the reason is you are using an old kernel which
>>>>> does not have oom_control capability.
>>>>>
>>>>>
>>>>> - Jie
>>>>>
>>>>>
>>>>> -----------------------------------------------------------
>>>>>
>>>>> This is an automatically generated e-mail. To reply, visit:
>>>>> https://reviews.apache.org/r/7338/#review12031
>>>>> -----------------------------------------------------------
>>>>>
>>>>>
>>>>> On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>>>>> >
>>>>> > -----------------------------------------------------------
>>>>> > This is an automatically generated e-mail. To reply, visit:
>>>>> > https://reviews.apache.org/r/7338/
>>>>> > -----------------------------------------------------------
>>>>> >
>>>>> > (Updated Sept. 28, 2012, 5:33 a.m.)
>>>>>
>>>>> >
>>>>> >
>>>>> > Review request for mesos and Benjamin Hindman.
>>>>> >
>>>>> >
>>>>> > Description
>>>>> > -------
>>>>>
>>>>> >
>>>>> > The recent refactor changes break the assumptions in the cgroups
>>>>> code.
>>>>> >
>>>>> >
>>>>> > Diffs
>>>>> > -----
>>>>> >
>>>>> > src/linux/cgroups.cpp cdafe6e
>>>>> > third_party/libprocess/include/stout/os.hpp 13dbc71
>>>>> >
>>>>> > Diff: https://reviews.apache.org/r/7338/diff/
>>>>> >
>>>>> >
>>>>> > Testing
>>>>> > -------
>>>>> >
>>>>> > make check.
>>>>> >
>>>>> > Tested on my vm (latest ubuntu 12.04)
>>>>> >
>>>>> >
>>>>> > Thanks,
>>>>> >
>>>>> > Jie Yu
>>>>> >
>>>>> >
>>>>>
>>>>>
>>>>
>>>
>>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Jie Yu <yu...@gmail.com>.
two possible solutions:
1) have a CHECK when cgroup initializing.
2) disable cgroup in configure if the kernel version is too old.
I liked (1) because the runtime environment might be different from the
compile environment.
- Jie
On Fri, Sep 28, 2012 at 4:38 PM, Benjamin Mahler <bm...@twitter.com>wrote:
> I also like the tests being red if it's not available, like you said it
> indicates that they shouldn't use cgroups.
>
> However, we should really have a way to _prevent_ them from using cgroups
> if it's not available, have we thought about this yet?
>
>
> On Fri, Sep 28, 2012 at 12:29 PM, Jie Yu <yu...@gmail.com> wrote:
>
>> That's the reason. Try to install a newer version of ubuntu (e.g. ubuntu
>> 11.10, 12.04) so that you can test the oom_control functionality.
>>
>> Probably we should disable these tests if oom_control is not available.
>> But seems that gtest does not provide a clean way to do this (dynamically
>> disable a test).
>>
>> We can add another PREFIX to those test cases (like ROOT_CGROUP), but
>> that seems to be tedious.
>>
>> Also, the break of these tests indicates a good thing: we do require
>> oom_control capability in cgroups isolation module. If they cannot pass the
>> unit test, they should not use the cgroups module.
>>
>> - Jie
>>
>>
>> On Fri, Sep 28, 2012 at 3:23 PM, Benjamin Mahler <bm...@twitter.com>wrote:
>>
>>> $ uname -a
>>> Linux ubuntu 2.6.32-38-generic #83-Ubuntu SMP Wed Jan 4 11:12:07 UTC
>>> 2012 x86_64 GNU/Linux
>>>
>>> On Fri, Sep 28, 2012 at 12:22 PM, Jie Yu <yu...@gmail.com> wrote:
>>>
>>>>
>>>>
>>>> > On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
>>>> > > I've patched this in and I'm seeing a a failing test:
>>>> > >
>>>> > > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
>>>> > > ../../src/tests/cgroups_tests.cpp:365: Failure
>>>> > > Value of: cgroups::writeControl(hierarchy, "/prof",
>>>> "memory.oom_control", "1").isSome()
>>>> > > Actual: false
>>>> > > Expected: true
>>>> > > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>>>>
>>>> What's your kernel version? Please do a 'uname -a'.
>>>>
>>>> We saw this before, the reason is you are using an old kernel which
>>>> does not have oom_control capability.
>>>>
>>>>
>>>> - Jie
>>>>
>>>>
>>>> -----------------------------------------------------------
>>>>
>>>> This is an automatically generated e-mail. To reply, visit:
>>>> https://reviews.apache.org/r/7338/#review12031
>>>> -----------------------------------------------------------
>>>>
>>>>
>>>> On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>>>> >
>>>> > -----------------------------------------------------------
>>>> > This is an automatically generated e-mail. To reply, visit:
>>>> > https://reviews.apache.org/r/7338/
>>>> > -----------------------------------------------------------
>>>> >
>>>> > (Updated Sept. 28, 2012, 5:33 a.m.)
>>>>
>>>> >
>>>> >
>>>> > Review request for mesos and Benjamin Hindman.
>>>> >
>>>> >
>>>> > Description
>>>> > -------
>>>>
>>>> >
>>>> > The recent refactor changes break the assumptions in the cgroups code.
>>>> >
>>>> >
>>>> > Diffs
>>>> > -----
>>>> >
>>>> > src/linux/cgroups.cpp cdafe6e
>>>> > third_party/libprocess/include/stout/os.hpp 13dbc71
>>>> >
>>>> > Diff: https://reviews.apache.org/r/7338/diff/
>>>> >
>>>> >
>>>> > Testing
>>>> > -------
>>>> >
>>>> > make check.
>>>> >
>>>> > Tested on my vm (latest ubuntu 12.04)
>>>> >
>>>> >
>>>> > Thanks,
>>>> >
>>>> > Jie Yu
>>>> >
>>>> >
>>>>
>>>>
>>>
>>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Benjamin Mahler <bm...@twitter.com>.
I also like the tests being red if it's not available, like you said it
indicates that they shouldn't use cgroups.
However, we should really have a way to _prevent_ them from using cgroups
if it's not available, have we thought about this yet?
On Fri, Sep 28, 2012 at 12:29 PM, Jie Yu <yu...@gmail.com> wrote:
> That's the reason. Try to install a newer version of ubuntu (e.g. ubuntu
> 11.10, 12.04) so that you can test the oom_control functionality.
>
> Probably we should disable these tests if oom_control is not available.
> But seems that gtest does not provide a clean way to do this (dynamically
> disable a test).
>
> We can add another PREFIX to those test cases (like ROOT_CGROUP), but that
> seems to be tedious.
>
> Also, the break of these tests indicates a good thing: we do require
> oom_control capability in cgroups isolation module. If they cannot pass the
> unit test, they should not use the cgroups module.
>
> - Jie
>
>
> On Fri, Sep 28, 2012 at 3:23 PM, Benjamin Mahler <bm...@twitter.com>wrote:
>
>> $ uname -a
>> Linux ubuntu 2.6.32-38-generic #83-Ubuntu SMP Wed Jan 4 11:12:07 UTC 2012
>> x86_64 GNU/Linux
>>
>> On Fri, Sep 28, 2012 at 12:22 PM, Jie Yu <yu...@gmail.com> wrote:
>>
>>>
>>>
>>> > On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
>>> > > I've patched this in and I'm seeing a a failing test:
>>> > >
>>> > > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
>>> > > ../../src/tests/cgroups_tests.cpp:365: Failure
>>> > > Value of: cgroups::writeControl(hierarchy, "/prof",
>>> "memory.oom_control", "1").isSome()
>>> > > Actual: false
>>> > > Expected: true
>>> > > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>>>
>>> What's your kernel version? Please do a 'uname -a'.
>>>
>>> We saw this before, the reason is you are using an old kernel which does
>>> not have oom_control capability.
>>>
>>>
>>> - Jie
>>>
>>>
>>> -----------------------------------------------------------
>>>
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/7338/#review12031
>>> -----------------------------------------------------------
>>>
>>>
>>> On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>>> >
>>> > -----------------------------------------------------------
>>> > This is an automatically generated e-mail. To reply, visit:
>>> > https://reviews.apache.org/r/7338/
>>> > -----------------------------------------------------------
>>> >
>>> > (Updated Sept. 28, 2012, 5:33 a.m.)
>>>
>>> >
>>> >
>>> > Review request for mesos and Benjamin Hindman.
>>> >
>>> >
>>> > Description
>>> > -------
>>>
>>> >
>>> > The recent refactor changes break the assumptions in the cgroups code.
>>> >
>>> >
>>> > Diffs
>>> > -----
>>> >
>>> > src/linux/cgroups.cpp cdafe6e
>>> > third_party/libprocess/include/stout/os.hpp 13dbc71
>>> >
>>> > Diff: https://reviews.apache.org/r/7338/diff/
>>> >
>>> >
>>> > Testing
>>> > -------
>>> >
>>> > make check.
>>> >
>>> > Tested on my vm (latest ubuntu 12.04)
>>> >
>>> >
>>> > Thanks,
>>> >
>>> > Jie Yu
>>> >
>>> >
>>>
>>>
>>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Jie Yu <yu...@gmail.com>.
That's the reason. Try to install a newer version of ubuntu (e.g. ubuntu
11.10, 12.04) so that you can test the oom_control functionality.
Probably we should disable these tests if oom_control is not available. But
seems that gtest does not provide a clean way to do this (dynamically
disable a test).
We can add another PREFIX to those test cases (like ROOT_CGROUP), but that
seems to be tedious.
Also, the break of these tests indicates a good thing: we do require
oom_control capability in cgroups isolation module. If they cannot pass the
unit test, they should not use the cgroups module.
- Jie
On Fri, Sep 28, 2012 at 3:23 PM, Benjamin Mahler <bm...@twitter.com>wrote:
> $ uname -a
> Linux ubuntu 2.6.32-38-generic #83-Ubuntu SMP Wed Jan 4 11:12:07 UTC 2012
> x86_64 GNU/Linux
>
> On Fri, Sep 28, 2012 at 12:22 PM, Jie Yu <yu...@gmail.com> wrote:
>
>>
>>
>> > On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
>> > > I've patched this in and I'm seeing a a failing test:
>> > >
>> > > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
>> > > ../../src/tests/cgroups_tests.cpp:365: Failure
>> > > Value of: cgroups::writeControl(hierarchy, "/prof",
>> "memory.oom_control", "1").isSome()
>> > > Actual: false
>> > > Expected: true
>> > > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>>
>> What's your kernel version? Please do a 'uname -a'.
>>
>> We saw this before, the reason is you are using an old kernel which does
>> not have oom_control capability.
>>
>>
>> - Jie
>>
>>
>> -----------------------------------------------------------
>>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/7338/#review12031
>> -----------------------------------------------------------
>>
>>
>> On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>> >
>> > -----------------------------------------------------------
>> > This is an automatically generated e-mail. To reply, visit:
>> > https://reviews.apache.org/r/7338/
>> > -----------------------------------------------------------
>> >
>> > (Updated Sept. 28, 2012, 5:33 a.m.)
>>
>> >
>> >
>> > Review request for mesos and Benjamin Hindman.
>> >
>> >
>> > Description
>> > -------
>>
>> >
>> > The recent refactor changes break the assumptions in the cgroups code.
>> >
>> >
>> > Diffs
>> > -----
>> >
>> > src/linux/cgroups.cpp cdafe6e
>> > third_party/libprocess/include/stout/os.hpp 13dbc71
>> >
>> > Diff: https://reviews.apache.org/r/7338/diff/
>> >
>> >
>> > Testing
>> > -------
>> >
>> > make check.
>> >
>> > Tested on my vm (latest ubuntu 12.04)
>> >
>> >
>> > Thanks,
>> >
>> > Jie Yu
>> >
>> >
>>
>>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Benjamin Mahler <bm...@twitter.com>.
$ uname -a
Linux ubuntu 2.6.32-38-generic #83-Ubuntu SMP Wed Jan 4 11:12:07 UTC 2012
x86_64 GNU/Linux
On Fri, Sep 28, 2012 at 12:22 PM, Jie Yu <yu...@gmail.com> wrote:
>
>
> > On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
> > > I've patched this in and I'm seeing a a failing test:
> > >
> > > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
> > > ../../src/tests/cgroups_tests.cpp:365: Failure
> > > Value of: cgroups::writeControl(hierarchy, "/prof",
> "memory.oom_control", "1").isSome()
> > > Actual: false
> > > Expected: true
> > > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>
> What's your kernel version? Please do a 'uname -a'.
>
> We saw this before, the reason is you are using an old kernel which does
> not have oom_control capability.
>
>
> - Jie
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/#review12031
> -----------------------------------------------------------
>
>
> On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/7338/
> > -----------------------------------------------------------
> >
> > (Updated Sept. 28, 2012, 5:33 a.m.)
> >
> >
> > Review request for mesos and Benjamin Hindman.
> >
> >
> > Description
> > -------
> >
> > The recent refactor changes break the assumptions in the cgroups code.
> >
> >
> > Diffs
> > -----
> >
> > src/linux/cgroups.cpp cdafe6e
> > third_party/libprocess/include/stout/os.hpp 13dbc71
> >
> > Diff: https://reviews.apache.org/r/7338/diff/
> >
> >
> > Testing
> > -------
> >
> > make check.
> >
> > Tested on my vm (latest ubuntu 12.04)
> >
> >
> > Thanks,
> >
> > Jie Yu
> >
> >
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
> > I've patched this in and I'm seeing a a failing test:
> >
> > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
> > ../../src/tests/cgroups_tests.cpp:365: Failure
> > Value of: cgroups::writeControl(hierarchy, "/prof", "memory.oom_control", "1").isSome()
> > Actual: false
> > Expected: true
> > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
What's your kernel version? Please do a 'uname -a'.
We saw this before, the reason is you are using an old kernel which does not have oom_control capability.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12031
-----------------------------------------------------------
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
> > I've patched this in and I'm seeing a a failing test:
> >
> > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
> > ../../src/tests/cgroups_tests.cpp:365: Failure
> > Value of: cgroups::writeControl(hierarchy, "/prof", "memory.oom_control", "1").isSome()
> > Actual: false
> > Expected: true
> > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>
> Jie Yu wrote:
> What's your kernel version? Please do a 'uname -a'.
>
> We saw this before, the reason is you are using an old kernel which does not have oom_control capability.
>
> Ben Mahler wrote:
> Ok, they're all green on 12.04 for me too.
Can we detect this via a CHECK at runtime?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12031
-----------------------------------------------------------
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
> > I've patched this in and I'm seeing a a failing test:
> >
> > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
> > ../../src/tests/cgroups_tests.cpp:365: Failure
> > Value of: cgroups::writeControl(hierarchy, "/prof", "memory.oom_control", "1").isSome()
> > Actual: false
> > Expected: true
> > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>
> Jie Yu wrote:
> What's your kernel version? Please do a 'uname -a'.
>
> We saw this before, the reason is you are using an old kernel which does not have oom_control capability.
Ok, they're all green on 12.04 for me too.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12031
-----------------------------------------------------------
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
> > I've patched this in and I'm seeing a a failing test:
> >
> > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
> > ../../src/tests/cgroups_tests.cpp:365: Failure
> > Value of: cgroups::writeControl(hierarchy, "/prof", "memory.oom_control", "1").isSome()
> > Actual: false
> > Expected: true
> > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>
> Jie Yu wrote:
> What's your kernel version? Please do a 'uname -a'.
>
> We saw this before, the reason is you are using an old kernel which does not have oom_control capability.
>
> Ben Mahler wrote:
> Ok, they're all green on 12.04 for me too.
>
> Benjamin Hindman wrote:
> Can we detect this via a CHECK at runtime?
Sorry, I don't get it. A CHECK will abort the entire test run I presume?
I think probably we need a print saying that oom_control is not available.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12031
-----------------------------------------------------------
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 28, 2012, 7:16 p.m., Ben Mahler wrote:
> > I've patched this in and I'm seeing a a failing test:
> >
> > [ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
> > ../../src/tests/cgroups_tests.cpp:365: Failure
> > Value of: cgroups::writeControl(hierarchy, "/prof", "memory.oom_control", "1").isSome()
> > Actual: false
> > Expected: true
> > [ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
>
> Jie Yu wrote:
> What's your kernel version? Please do a 'uname -a'.
>
> We saw this before, the reason is you are using an old kernel which does not have oom_control capability.
>
> Ben Mahler wrote:
> Ok, they're all green on 12.04 for me too.
>
> Benjamin Hindman wrote:
> Can we detect this via a CHECK at runtime?
>
> Jie Yu wrote:
> Sorry, I don't get it. A CHECK will abort the entire test run I presume?
>
> I think probably we need a print saying that oom_control is not available.
Either we:
(a) Don't run the tests if we know that they'll fail (because we've checked the kernel version).
(b) Fail the tests with a useful error message that explains why they are failing.
Right now when the tests fail someone will send an email to mesos-dev and say, why are the tests failing? And then you, or me, or someone else, will have to respond and say: "What's your kernel version? Please do a 'uname -a'. We saw this before, the reason is you are using an old kernel which does not have oom_control capability."
I would obviously prefer (a) but I'm okay with a savvy user getting the error message from (b) and then explicitly doing '--gtest_filter=-Cgroups*' (which one could imagine the error message suggests).
When it comes to running one of the binaries (e.g., mesos-slave, mesos-local) it's imperative that we keep a user from launching a slave with '--isolation=cgroups' if we don't have all the functionality. Otherwise, we're probably going to be introducing semantics we haven't thought about and debugging cases that are very esoteric (which could waste a lot of time).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12031
-----------------------------------------------------------
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12031
-----------------------------------------------------------
I've patched this in and I'm seeing a a failing test:
[ RUN ] CgroupsTest.ROOT_CGROUPS_ListenEvent
../../src/tests/cgroups_tests.cpp:365: Failure
Value of: cgroups::writeControl(hierarchy, "/prof", "memory.oom_control", "1").isSome()
Actual: false
Expected: true
[ FAILED ] CgroupsTest.ROOT_CGROUPS_ListenEvent (290 ms)
- Ben Mahler
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 28, 2012, 5:43 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 459
> > <https://reviews.apache.org/r/7338/diff/1/?file=160437#file160437line459>
> >
> > I like your note above about "NOT deleting recursively", and I think this case is special enough for you to use ::rmdir inline in the cgroups code rather adding the flag here.
> >
> > I missed it in the refactor since I wasn't sure why you used ::rmdir instead of os::rmdir, but the notes you added will make sure no one else makes this mistake :)
Having an extra bool is more powerful than just the comment. Now, in a future refactor, the programmer will have to wonder, "what is this extra argument?", and actually do something about it to get the code to compile (versus forgetting to read a comment). Having 'rmdirr' (extra 'r' at end) or 'mkdirp' accomplishes the same thing but since we already have os::mkdir and os::rmdir I think adding the argument is perfectly fine.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12025
-----------------------------------------------------------
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12025
-----------------------------------------------------------
Thanks jie, this was my fault. I did the big Try<Nothing> refactor and didn't realize that you didn't want to call the os:: recursive functions.
Can you provide background on why this breaks the tests?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/7338/#comment25643>
I find it surprising that we had mkdir behaving as mkdir -p. I think mkdirp makes more sense, but changing that is yet another big refactor, so maybe add a TODO.
For now, I think you could just leave os::mkdir as is, and do ::mkdir in your cgroups code as before, since the note you added clears things up.
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/7338/#comment25644>
I like your note above about "NOT deleting recursively", and I think this case is special enough for you to use ::rmdir inline in the cgroups code rather adding the flag here.
I missed it in the refactor since I wasn't sure why you used ::rmdir instead of os::rmdir, but the notes you added will make sure no one else makes this mistake :)
- Ben Mahler
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Jie Yu <yu...@gmail.com>.
Two reasons:
1) Files in a cgroup are not regular files. You cannot 'rm' them. You can
only remove the directory using 'rmdir'
2) We want the creation/removal of a cgroup to be atomic. For example,
assume the cgroup hierarchy is like the following:
/cgroup -- hierarchy root
/cgroup/a
/cgroup/a/1
/cgroup/a/2
You want to remove the cgroup 'a'. If you use the recursive version, here
is what will potentially happen:
1. cgroup 'a/1' is removed
2. cgroup 'a/2' cannot be removed because some tasks are still in the
cgroup
3. cgroup 'a' cannot be removed since 'a/2' is not removed yet.
In that case, we have a partial removal, which we don't expect.
- Jie
On Fri, Sep 28, 2012 at 2:10 PM, Vinod Kone <vi...@gmail.com> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/#review12026
> -----------------------------------------------------------
>
>
>
> src/linux/cgroups.cpp
> <https://reviews.apache.org/r/7338/#comment25646>
>
> I don't understand why recursive is not good here?
>
>
>
> src/linux/cgroups.cpp
> <https://reviews.apache.org/r/7338/#comment25645>
>
> ditto
>
>
> - Vinod Kone
>
>
> On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/7338/
> > -----------------------------------------------------------
> >
> > (Updated Sept. 28, 2012, 5:33 a.m.)
> >
> >
> > Review request for mesos and Benjamin Hindman.
> >
> >
> > Description
> > -------
> >
> > The recent refactor changes break the assumptions in the cgroups code.
> >
> >
> > Diffs
> > -----
> >
> > src/linux/cgroups.cpp cdafe6e
> > third_party/libprocess/include/stout/os.hpp 13dbc71
> >
> > Diff: https://reviews.apache.org/r/7338/diff/
> >
> >
> > Testing
> > -------
> >
> > make check.
> >
> > Tested on my vm (latest ubuntu 12.04)
> >
> >
> > Thanks,
> >
> > Jie Yu
> >
> >
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12026
-----------------------------------------------------------
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7338/#comment25646>
I don't understand why recursive is not good here?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/7338/#comment25645>
ditto
- Vinod Kone
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12295
-----------------------------------------------------------
Ship it!
I'm going to commit this for now with the intent to soon after clean up these semantics.
- Benjamin Hindman
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request: Fixed a bug in cgroups due to mkdir/rmdir.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7338/#review12027
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Sept. 28, 2012, 5:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7338/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2012, 5:33 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> The recent refactor changes break the assumptions in the cgroups code.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp cdafe6e
> third_party/libprocess/include/stout/os.hpp 13dbc71
>
> Diff: https://reviews.apache.org/r/7338/diff/
>
>
> Testing
> -------
>
> make check.
>
> Tested on my vm (latest ubuntu 12.04)
>
>
> Thanks,
>
> Jie Yu
>
>