You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2017/08/02 22:05:48 UTC
Review Request 61387: Made the rlimits isolator work with nested
containers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61387/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Jie Yu.
Bugs: MESOS-7849
https://issues.apache.org/jira/browse/MESOS-7849
Repository: mesos
Description
-------
Made the rlimits isolator support nesting. Without this patch rlimits
sets for tasks launched by the DefaultExecutor are silently ignored.
Diffs
-----
src/slave/containerizer/mesos/isolators/posix/rlimits.hpp ee36a24ae36179d3ff3141f8c3cdc768b1e399af
src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49
src/tests/containerizer/posix_rlimits_isolator_tests.cpp b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4
Diff: https://reviews.apache.org/r/61387/diff/1/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 61387: Made the rlimits isolator work with nested
containers.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/posix_rlimits_isolator_tests.cpp
> > Lines 380-381 (patched)
> > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line380>
> >
> > Indent by two spaces less.
The pattern in this file is to use 4 spaces, see the following tests (some of them added by you):
https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L70-L73
https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L147-L149
https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L227-L229
https://github.com/apache/mesos/blob/cdb90b91ce8ce02d6163e5e2ee5b46fb797b1dee/src/tests/containerizer/posix_rlimits_isolator_tests.cpp#L294-L296
clang-format suggests two spaces, and I agree with you both. So I'm thinking we should probably keep 4 spaces here and then have a different patch cleaning up the whitespaces in all these tests?
> On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/posix_rlimits_isolator_tests.cpp
> > Lines 388 (patched)
> > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line388>
> >
> > `ASSERT_FALSE(offers->empty())`.
I agree that your proposal is more readable. Yet the other tests in this file and a LOT of other tests use `ASSERT_NE(0u, offers->size());`.
I'm thinking again that we should keep it like this in this patch and then do a sweeping change?
> On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/posix_rlimits_isolator_tests.cpp
> > Lines 392-396 (patched)
> > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line392>
> >
> > Currently the declaration and first use are not on the same page for me. Let's move this right above its use,
> >
> > driver.acceptOffers({offer.id()}, {LAUNCH_GROUP(executorInfo, taskGroup)})
Moved this down. Now the `taskStatuses` declaration and expectations are kind of away from their use, but I think that's ok.
> On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/posix_rlimits_isolator_tests.cpp
> > Lines 449 (patched)
> > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line449>
> >
> > It would be nice to avoid hardcoding `4` here, e.g.,
> >
> > foreach (Future<TaskStatus> update, updates) {
> > EXPECT_CALL(sched, statusUpdate(&driver, _))
> > .WillOnce(FutureArg<1>(&update));
> > }
>
> Benjamin Bannier wrote:
> Dropped a ref here,
>
> foreach (Future<TaskStatus>& update, updates) {
> EXPECT_CALL(sched, statusUpdate(&driver, _))
> .WillOnce(FutureArg<1>(&update));
> }
Done (but with 2 spaces to stay consistent with the rest of the file).
> On Aug. 4, 2017, 8:58 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/posix_rlimits_isolator_tests.cpp
> > Lines 490 (patched)
> > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line490>
> >
> > `s/updates[1]/taskStatus/`.
Good catch =).
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61387/#review182189
-----------------------------------------------------------
On Aug. 4, 2017, 9:15 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61387/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2017, 9:15 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7849
> https://issues.apache.org/jira/browse/MESOS-7849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Make the rlimits isolator support nesting. Without this patch rlimits
> sets for tasks launched by the DefaultExecutor are silently ignored.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/rlimits.hpp ee36a24ae36179d3ff3141f8c3cdc768b1e399af
> src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49
> src/tests/containerizer/posix_rlimits_isolator_tests.cpp b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4
>
>
> Diff: https://reviews.apache.org/r/61387/diff/4/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61387: Made the rlimits isolator work with nested
containers.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Aug. 4, 2017, 10:58 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/posix_rlimits_isolator_tests.cpp
> > Lines 449 (patched)
> > <https://reviews.apache.org/r/61387/diff/2/?file=1789190#file1789190line449>
> >
> > It would be nice to avoid hardcoding `4` here, e.g.,
> >
> > foreach (Future<TaskStatus> update, updates) {
> > EXPECT_CALL(sched, statusUpdate(&driver, _))
> > .WillOnce(FutureArg<1>(&update));
> > }
Dropped a ref here,
foreach (Future<TaskStatus>& update, updates) {
EXPECT_CALL(sched, statusUpdate(&driver, _))
.WillOnce(FutureArg<1>(&update));
}
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61387/#review182189
-----------------------------------------------------------
On Aug. 4, 2017, 12:10 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61387/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2017, 12:10 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7849
> https://issues.apache.org/jira/browse/MESOS-7849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Make the rlimits isolator support nesting. Without this patch rlimits
> sets for tasks launched by the DefaultExecutor are silently ignored.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/rlimits.hpp ee36a24ae36179d3ff3141f8c3cdc768b1e399af
> src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49
> src/tests/containerizer/posix_rlimits_isolator_tests.cpp b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4
>
>
> Diff: https://reviews.apache.org/r/61387/diff/2/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61387: Made the rlimits isolator work with nested
containers.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61387/#review182189
-----------------------------------------------------------
LGTM. Left mostly style comments.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 354-358 (patched)
<https://reviews.apache.org/r/61387/#comment258059>
No extra indent because of preprocessor conditional; indent this like e.g., `flags.bounding_capabilities = ...`
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 368-371 (patched)
<https://reviews.apache.org/r/61387/#comment258060>
Indent by two spaces less.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 380-381 (patched)
<https://reviews.apache.org/r/61387/#comment258061>
Indent by two spaces less.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 388 (patched)
<https://reviews.apache.org/r/61387/#comment258062>
`ASSERT_FALSE(offers->empty())`.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 392-396 (patched)
<https://reviews.apache.org/r/61387/#comment258066>
Currently the declaration and first use are not on the same page for me. Let's move this right above its use,
driver.acceptOffers({offer.id()}, {LAUNCH_GROUP(executorInfo, taskGroup)})
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 402-407 (patched)
<https://reviews.apache.org/r/61387/#comment258063>
Indent by two spaces less.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 407 (patched)
<https://reviews.apache.org/r/61387/#comment258072>
Let's set a `TaskID` here to ease debugging in below loop over the status updates.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 416-417 (patched)
<https://reviews.apache.org/r/61387/#comment258067>
It seems very unlikely that the simple shell script we add every consume 10s of CPU time, but would I wonder if it makes sense to make this number truely unrealistically large, e.g., 1000000 and 2000000 instead of 10 and 20. That way we'd never fail the test because of flakiness but only due to global timeouts.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 423-428 (patched)
<https://reviews.apache.org/r/61387/#comment258064>
Indent by two spaces less.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 448 (patched)
<https://reviews.apache.org/r/61387/#comment258068>
Maybe `s/dummy/inSequence/`.
It is probably a good idea to add a comment explaining that it is fine that we never use this var.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 449 (patched)
<https://reviews.apache.org/r/61387/#comment258069>
It would be nice to avoid hardcoding `4` here, e.g.,
foreach (Future<TaskStatus> update, updates) {
EXPECT_CALL(sched, statusUpdate(&driver, _))
.WillOnce(FutureArg<1>(&update));
}
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 459 (patched)
<https://reviews.apache.org/r/61387/#comment258075>
A comment briefly summarizing the testing strategy (expected transitions, independent tracking of task status) would be great here.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 460 (patched)
<https://reviews.apache.org/r/61387/#comment258076>
We do not seem to put these on a single line and instead would use
enum class Stage
{
INITIAL,
RUNNING,
FINISHED
};
The only other case where we us a one-liner is in `check_tests.cpp` (also added by you).
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 463-464 (patched)
<https://reviews.apache.org/r/61387/#comment258070>
This seems to be equivalent to the shorter
taskStages[task1.task_id()] = Stage::INITIAL;
taskStages[task2.task_id()] = Stage::INITIAL;
Here and in all subsequent uses of `hashmap<K,V>::put`.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 466 (patched)
<https://reviews.apache.org/r/61387/#comment258065>
Stray space before closing paren.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 466-469 (patched)
<https://reviews.apache.org/r/61387/#comment258077>
Let's avoid hardcoding hardcoding `4` here, e.g., instead
foreach (const Future<TaskStatus>& update, updates) {
AWAIT_READY(update);
// Either bind `taskStatus` like you currently do, or use `update->` directly (would look nicer if `updates` was named `taskStatuses` so we'd have a future `taskStatus` instead of `update`).
}
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 476 (patched)
<https://reviews.apache.org/r/61387/#comment258074>
Let's output the full `taskStatus` when this assert fires.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 483 (patched)
<https://reviews.apache.org/r/61387/#comment258073>
Let's output the full `taskStatus` when this assert fires.
src/tests/containerizer/posix_rlimits_isolator_tests.cpp
Lines 490 (patched)
<https://reviews.apache.org/r/61387/#comment258071>
`s/updates[1]/taskStatus/`.
- Benjamin Bannier
On Aug. 4, 2017, 12:10 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61387/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2017, 12:10 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7849
> https://issues.apache.org/jira/browse/MESOS-7849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Make the rlimits isolator support nesting. Without this patch rlimits
> sets for tasks launched by the DefaultExecutor are silently ignored.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/rlimits.hpp ee36a24ae36179d3ff3141f8c3cdc768b1e399af
> src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49
> src/tests/containerizer/posix_rlimits_isolator_tests.cpp b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4
>
>
> Diff: https://reviews.apache.org/r/61387/diff/2/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61387: Made the rlimits isolator work with nested
containers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61387/#review182240
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Aug. 4, 2017, 9:15 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61387/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2017, 9:15 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-7849
> https://issues.apache.org/jira/browse/MESOS-7849
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Make the rlimits isolator support nesting. Without this patch rlimits
> sets for tasks launched by the DefaultExecutor are silently ignored.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/rlimits.hpp ee36a24ae36179d3ff3141f8c3cdc768b1e399af
> src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49
> src/tests/containerizer/posix_rlimits_isolator_tests.cpp b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4
>
>
> Diff: https://reviews.apache.org/r/61387/diff/4/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running GNU/Linux.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61387: Made the rlimits isolator work with nested
containers.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61387/
-----------------------------------------------------------
(Updated Aug. 4, 2017, 9:15 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed feedback.
Bugs: MESOS-7849
https://issues.apache.org/jira/browse/MESOS-7849
Repository: mesos
Description
-------
Make the rlimits isolator support nesting. Without this patch rlimits
sets for tasks launched by the DefaultExecutor are silently ignored.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/posix/rlimits.hpp ee36a24ae36179d3ff3141f8c3cdc768b1e399af
src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49
src/tests/containerizer/posix_rlimits_isolator_tests.cpp b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4
Diff: https://reviews.apache.org/r/61387/diff/3/
Changes: https://reviews.apache.org/r/61387/diff/2-3/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running GNU/Linux.
Thanks,
Gastón Kleiman
Re: Review Request 61387: Made the rlimits isolator work with nested
containers.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61387/
-----------------------------------------------------------
(Updated Aug. 3, 2017, 10:10 p.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Ported the test to the v0 API.
Bugs: MESOS-7849
https://issues.apache.org/jira/browse/MESOS-7849
Repository: mesos
Description (updated)
-------
Make the rlimits isolator support nesting. Without this patch rlimits
sets for tasks launched by the DefaultExecutor are silently ignored.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/posix/rlimits.hpp ee36a24ae36179d3ff3141f8c3cdc768b1e399af
src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49
src/tests/containerizer/posix_rlimits_isolator_tests.cpp b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4
Diff: https://reviews.apache.org/r/61387/diff/2/
Changes: https://reviews.apache.org/r/61387/diff/1-2/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running GNU/Linux.
Thanks,
Gastón Kleiman