You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jian Qiu <qi...@cn.ibm.com> on 2015/09/11 05:05:16 UTC
Review Request 38287: Check if the futrue is failed before dispatch in
freeze()
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/
-----------------------------------------------------------
Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
Bugs: MESOS-3272
https://issues.apache.org/jira/browse/MESOS-3272
Repository: mesos
Description
-------
This check in cgroups::freezer::freeze() is necessary
since the freezer process can be terminated in its
initialize method
Diffs
-----
src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
Diff: https://reviews.apache.org/r/38287/diff/
Testing
-------
./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
Thanks,
Jian Qiu
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review98840
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38287]
All tests passed.
- Mesos ReviewBot
On Sept. 14, 2015, 7:28 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 7:28 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This check in cgroups::freezer::freeze() is necessary
> since the freezer process can be terminated in its
> initialize method
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review108403
-----------------------------------------------------------
See the comment below about the issue with this approach. I will commit a fix without the race.
src/linux/cgroups.cpp (lines 2431 - 2440)
<https://reviews.apache.org/r/38287/#comment167871>
The bug here is that when we pass a Process pointer to a managed=true call to spawn, this is passing ownership and we must not access the pointer any further.
The fix you've proposed here is racy, the future failure may occur at any time. If it occurs between your if condition and the dispatch, this will still crash.
I would suggest instead that we obtain the PID before we spawn, so that we can dispatch using the PID instead of a process pointer.
- Ben Mahler
On Nov. 23, 2015, 5:37 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Nov. 23, 2015, 5:37 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer, the freezer process is terminated in it initilize method, which causes the core dump when cgroups::freezer::freeze() calls dispatch(freezer, &internal::Freezer::freeze).
>
> This check is added in cgroups::freezer::freeze() to avoid calling dispatch if the freezer process is terminated.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp f67633e31884b21e943a06100191d2228a637b1a
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --gtest_repeat=1000 --gtest_break_on_failure
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review107558
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38287]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Nov. 23, 2015, 5:37 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Nov. 23, 2015, 5:37 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer, the freezer process is terminated in it initilize method, which causes the core dump when cgroups::freezer::freeze() calls dispatch(freezer, &internal::Freezer::freeze).
>
> This check is added in cgroups::freezer::freeze() to avoid calling dispatch if the freezer process is terminated.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp f67633e31884b21e943a06100191d2228a637b1a
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --gtest_repeat=1000 --gtest_break_on_failure
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Jian Qiu <qi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/
-----------------------------------------------------------
(Updated Nov. 23, 2015, 5:37 a.m.)
Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
Changes
-------
rebase
Bugs: MESOS-3272
https://issues.apache.org/jira/browse/MESOS-3272
Repository: mesos
Description
-------
In CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer, the freezer process is terminated in it initilize method, which causes the core dump when cgroups::freezer::freeze() calls dispatch(freezer, &internal::Freezer::freeze).
This check is added in cgroups::freezer::freeze() to avoid calling dispatch if the freezer process is terminated.
Diffs (updated)
-----
src/linux/cgroups.cpp f67633e31884b21e943a06100191d2228a637b1a
Diff: https://reviews.apache.org/r/38287/diff/
Testing
-------
./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --gtest_repeat=1000 --gtest_break_on_failure
Thanks,
Jian Qiu
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review100364
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38287]
All tests passed.
- Mesos ReviewBot
On Sept. 24, 2015, 3:15 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 3:15 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer, the freezer process is terminated in it initilize method, which causes the core dump when cgroups::freezer::freeze() calls dispatch(freezer, &internal::Freezer::freeze).
>
> This check is added in cgroups::freezer::freeze() to avoid calling dispatch if the freezer process is terminated.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --gtest_repeat=1000 --gtest_break_on_failure
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review101290
-----------------------------------------------------------
LGTM
- Paul Brett
On Sept. 24, 2015, 3:15 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Sept. 24, 2015, 3:15 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer, the freezer process is terminated in it initilize method, which causes the core dump when cgroups::freezer::freeze() calls dispatch(freezer, &internal::Freezer::freeze).
>
> This check is added in cgroups::freezer::freeze() to avoid calling dispatch if the freezer process is terminated.
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --gtest_repeat=1000 --gtest_break_on_failure
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Jian Qiu <qi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/
-----------------------------------------------------------
(Updated Sept. 24, 2015, 3:15 a.m.)
Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
Bugs: MESOS-3272
https://issues.apache.org/jira/browse/MESOS-3272
Repository: mesos
Description (updated)
-------
In CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer, the freezer process is terminated in it initilize method, which causes the core dump when cgroups::freezer::freeze() calls dispatch(freezer, &internal::Freezer::freeze).
This check is added in cgroups::freezer::freeze() to avoid calling dispatch if the freezer process is terminated.
Diffs (updated)
-----
src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
Diff: https://reviews.apache.org/r/38287/diff/
Testing (updated)
-------
./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --gtest_repeat=1000 --gtest_break_on_failure
Thanks,
Jian Qiu
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Jian Qiu <qi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/
-----------------------------------------------------------
(Updated 九月 14, 2015, 7:28 a.m.)
Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
Bugs: MESOS-3272
https://issues.apache.org/jira/browse/MESOS-3272
Repository: mesos
Description
-------
This check in cgroups::freezer::freeze() is necessary
since the freezer process can be terminated in its
initialize method
Diffs (updated)
-----
src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
Diff: https://reviews.apache.org/r/38287/diff/
Testing
-------
./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
Thanks,
Jian Qiu
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Jian Qiu <qi...@cn.ibm.com>.
> On 九月 11, 2015, 3:09 a.m., haosdent huang wrote:
> > src/linux/cgroups.cpp, line 2440
> > <https://reviews.apache.org/r/38287/diff/1/?file=1067958#file1067958line2440>
> >
> > Does this fall into a loop?
>
> Jian Qiu wrote:
> Does not quite catch your point :) if you mean the loop in internal::Freezer::freeze, I think it does not.
>
> haosdent huang wrote:
> Oh, I mean if future keep failed, does this fall into a infinite loop?
OK, I think it will not fall into a loop. Based on the test case
TEST_F(CgroupsAnyHierarchyWithCpuMemoryTest, ROOT_CGROUPS_FreezeNonFreezer)
{
string hierarchy = path::join(baseHierarchy, "cpu");
ASSERT_SOME(cgroups::create(hierarchy, TEST_CGROUPS_ROOT));
AWAIT_EXPECT_FAILED(cgroups::freezer::freeze(hierarchy, TEST_CGROUPS_ROOT));
AWAIT_EXPECT_FAILED(cgroups::freezer::thaw(hierarchy, TEST_CGROUPS_ROOT));
// The cgroup is empty so we should still be able to destroy it.
AWAIT_READY(cgroups::destroy(hierarchy, TEST_CGROUPS_ROOT));
}
This function will just return failed future.
- Jian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review98540
-----------------------------------------------------------
On 九月 11, 2015, 3:05 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated 九月 11, 2015, 3:05 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This check in cgroups::freezer::freeze() is necessary
> since the freezer process can be terminated in its
> initialize method
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by haosdent huang <ha...@gmail.com>.
> On Sept. 11, 2015, 3:09 a.m., haosdent huang wrote:
> > src/linux/cgroups.cpp, line 2440
> > <https://reviews.apache.org/r/38287/diff/1/?file=1067958#file1067958line2440>
> >
> > Does this fall into a loop?
>
> Jian Qiu wrote:
> Does not quite catch your point :) if you mean the loop in internal::Freezer::freeze, I think it does not.
Oh, I mean if future keep failed, does this fall into a infinite loop?
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review98540
-----------------------------------------------------------
On Sept. 11, 2015, 3:05 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2015, 3:05 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This check in cgroups::freezer::freeze() is necessary
> since the freezer process can be terminated in its
> initialize method
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Jian Qiu <qi...@cn.ibm.com>.
> On 九月 11, 2015, 3:09 a.m., haosdent huang wrote:
> > src/linux/cgroups.cpp, line 2440
> > <https://reviews.apache.org/r/38287/diff/1/?file=1067958#file1067958line2440>
> >
> > Does this fall into a loop?
Does not quite catch your point :) if you mean the loop in internal::Freezer::freeze, I think it does not.
- Jian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review98540
-----------------------------------------------------------
On 九月 11, 2015, 3:05 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated 九月 11, 2015, 3:05 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This check in cgroups::freezer::freeze() is necessary
> since the freezer process can be terminated in its
> initialize method
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review98540
-----------------------------------------------------------
src/linux/cgroups.cpp (line 2440)
<https://reviews.apache.org/r/38287/#comment155056>
Does this fall into a loop?
- haosdent huang
On Sept. 11, 2015, 3:05 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2015, 3:05 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This check in cgroups::freezer::freeze() is necessary
> since the freezer process can be terminated in its
> initialize method
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
>
>
> Thanks,
>
> Jian Qiu
>
>
Re: Review Request 38287: Check if the futrue is failed before
dispatch in freeze()
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38287/#review98611
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38287]
All tests passed.
- Mesos ReviewBot
On Sept. 11, 2015, 3:05 a.m., Jian Qiu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2015, 3:05 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
>
>
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This check in cgroups::freezer::freeze() is necessary
> since the freezer process can be terminated in its
> initialize method
>
>
> Diffs
> -----
>
> src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
>
> Diff: https://reviews.apache.org/r/38287/diff/
>
>
> Testing
> -------
>
> ./mesos-tests.sh --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer" --verbose
>
>
> Thanks,
>
> Jian Qiu
>
>