Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/#review94376 --- Ship it! Thanks for the exploration, just a few comments on comments. src/tests/containerizer/memory_test_helper.cpp (line 76) https://reviews.apache.org/r/37065/#comment148954 This doesn't sound accurate to me: `to make sure allocated memory is mapped`. I think maybe something like `to make sure the mapped memory is locked` would be more accurate? src/tests/containerizer/memory_test_helper.cpp (lines 80 - 82) https://reviews.apache.org/r/37065/#comment148953 `mlockall` is not what we want ideally right? Can we mention that it's not ideal but that it exists temporarily to address `MESOS-3197`? src/tests/containerizer/memory_test_helper.cpp (lines 97 - 98) https://reviews.apache.org/r/37065/#comment148952 Could we add some additional comment here to describe why we need to do this? Something like: ``` Since `mlockall` is left unimplemented in OS X, we need to use `mlock` instead. ``` - Michael Park On Aug. 6, 2015, 5:54 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 6, 2015, 5:54 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/#review94393 --- Patch looks great! Reviews applied: [37065] All tests passed. - Mesos ReviewBot On Aug. 6, 2015, 5:54 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 6, 2015, 5:54 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/#review94394 --- src/tests/containerizer/memory_test_helper.cpp (lines 75 - 76) https://reviews.apache.org/r/37065/#comment148980 Not a big fan of explain how something will be used in the method description. What if we remove references to `mlockall()` and `mlock()` here. Something a long the lines of: This helper allocates memory and locks it to the physical memory so it won't get swapped by the OS. src/tests/containerizer/memory_test_helper.cpp (lines 79 - 86) https://reviews.apache.org/r/37065/#comment148981 After reading the man pages, `mlock()` exists in Linux too and frankly it looks safer to use than `mlockall()`. `mlock()` will only lock the memory passed to the call in the physical memory while `mlockall()` will lock every new allocation after we made this call (and that includes stack, heap and code sections) until we make a `munlockall()` call, which we don't. - Alexander Rojas On Aug. 6, 2015, 7:54 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 6, 2015, 7:54 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/#review94431 --- Patch looks great! Reviews applied: [37065] All tests passed. - Mesos ReviewBot On Aug. 6, 2015, 5:34 p.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 6, 2015, 5:34 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
On Aug. 6, 2015, 4:47 a.m., Alexander Rojas wrote: src/tests/containerizer/memory_test_helper.cpp, lines 79-87 https://reviews.apache.org/r/37065/diff/3/?file=1033300#file1033300line79 After reading the man pages, `mlock()` exists in Linux too and frankly it looks safer to use than `mlockall()`. `mlock()` will only lock the memory passed to the call in the physical memory while `mlockall()` will lock every new allocation after we made this call (and that includes stack, heap and code sections) until we make a `munlockall()` call, which we don't. It does exist on Linux, it just doesn't realiably work for our use case on Ubuntu. I am aware of the difference of mlock() and mlockall(), in the context of the OOM test it does not really mattter because these are short lived test processes that are either getting killed or are going away. On Aug. 6, 2015, 4:47 a.m., Alexander Rojas wrote: src/tests/containerizer/memory_test_helper.cpp, lines 75-76 https://reviews.apache.org/r/37065/diff/3/?file=1033300#file1033300line75 Not a big fan of explain how something will be used in the method description. What if we remove references to `mlockall()` and `mlock()` here. Something a long the lines of: This helper allocates memory and locks it to the physical memory so it won't get swapped by the OS. I've removed references to `mlockall()` and `mlock()`. - Artem --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/#review94394 --- On Aug. 6, 2015, 10:34 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 6, 2015, 10:34 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 6, 2015, 10:34 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Changes --- Addressed comments. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs (updated) - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
On Aug. 6, 2015, 1:21 a.m., Michael Park wrote: src/tests/containerizer/memory_test_helper.cpp, lines 81-83 https://reviews.apache.org/r/37065/diff/3/?file=1033300#file1033300line81 `mlockall` is not what we want ideally right? Can we mention that it's not ideal but that it exists temporarily to address `MESOS-3197`? I am not sure. For the purpose of this test (which is to artifficiialy drive memory utilization) mlockall() seems to be a safer choice. - Artem --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/#review94376 --- On Aug. 6, 2015, 10:34 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 6, 2015, 10:34 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 5, 2015, 10:54 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Changes --- Reverting refactor (see comments). Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs (updated) - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
On Aug. 3, 2015, 11:36 p.m., Michael Park wrote:
src/tests/containerizer/memory_test_helper.cpp, line 81
https://reviews.apache.org/r/37065/diff/1/?file=1028301#file1028301line81
What's the significance in the order in which these are called? I
would've expected something like:
```
// Make sure that all pages that are going to be mapped into the
// address space of this process become unevictable. This is needed
// for testing cgroup oom killer.
#ifdef __APPLE__
if (mlock(rss, size.bytes()) != 0)
#else
if (mlockall(MCL_FUTURE) != 0)
#endif
{
return ErrnoError(Failed to make pages to be mapped unevictable);
}
```
Artem Harutyunyan wrote:
The mlockall(MCL_FUTURE) has to be called before the memory allocation is
made (because it affects future allocations), whereas mlock() is called for
already allocated pages. I could probably change mlockall(MCL_FUTURE) to
mlockall(MCL_CURRENT) in your snippet and make it work that way. I'll test to
verify and will update the patch accordingly. Thanks!
James Peach wrote:
IMHO it would be better to add an autoconf check for ```mlockall```
availability that to check ```__APPLE__```.
Michael Park wrote:
@jpeach: Do you mean we should fail to configure if `mlockall` is not
available? If we did that, we wouldn't be able to compile on OS X though.
James Peach wrote:
In autoconf, do ```AC_CHECK_FUNC(mlockall)```, then make this code
conditional on ```#if HAVE_MLOCKALL```.
Michael Park wrote:
Ah I see, great! I agree that's better since there may be other platforms
aside from OS X that don't support `mlockall`. @hartem: what do you think?
I refactored following MPark's suggestion, however using
AC_CHECK_FUNC(mlockall) did not work, because the function is present on OSX,
it just prints the error message instead of actually locking memory pages.
- Artem
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/#review94001
---
On Aug. 5, 2015, 10:14 p.m., Artem Harutyunyan wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/
---
(Updated Aug. 5, 2015, 10:14 p.m.)
Review request for mesos, Benjamin Hindman and Michael Park.
Bugs: MESOS-3197
https://issues.apache.org/jira/browse/MESOS-3197
Repository: mesos
Description
---
See summary.
Diffs
-
src/tests/containerizer/memory_test_helper.cpp
5e40b747f4266e7532baf8fd02ea5db0955124d2
Diff: https://reviews.apache.org/r/37065/diff/
Testing
---
make check
Thanks,
Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- (Updated Aug. 5, 2015, 10:14 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Changes --- Addressed comments. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs (updated) - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/#review94001
---
Thanks for this Artem! I applied the patch and it works. Just a minor nit and a
question.
src/tests/containerizer/memory_test_helper.cpp (line 75)
https://reviews.apache.org/r/37065/#comment148500
`s/a/the/`
src/tests/containerizer/memory_test_helper.cpp (line 81)
https://reviews.apache.org/r/37065/#comment148501
What's the significance in the order in which these are called? I would've
expected something like:
```
// Make sure that all pages that are going to be mapped into the
// address space of this process become unevictable. This is needed
// for testing cgroup oom killer.
#ifdef __APPLE__
if (mlock(rss, size.bytes()) != 0)
#else
if (mlockall(MCL_FUTURE) != 0)
#endif
{
return ErrnoError(Failed to make pages to be mapped unevictable);
}
```
- Michael Park
On Aug. 4, 2015, 6:32 a.m., Artem Harutyunyan wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/
---
(Updated Aug. 4, 2015, 6:32 a.m.)
Review request for mesos, Benjamin Hindman and Michael Park.
Bugs: MESOS-3197
https://issues.apache.org/jira/browse/MESOS-3197
Repository: mesos
Description
---
See summary.
Diffs
-
src/tests/containerizer/memory_test_helper.cpp
5e40b747f4266e7532baf8fd02ea5db0955124d2
Diff: https://reviews.apache.org/r/37065/diff/
Testing
---
make check
Thanks,
Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
On Aug. 3, 2015, 11:36 p.m., Michael Park wrote:
src/tests/containerizer/memory_test_helper.cpp, line 81
https://reviews.apache.org/r/37065/diff/1/?file=1028301#file1028301line81
What's the significance in the order in which these are called? I
would've expected something like:
```
// Make sure that all pages that are going to be mapped into the
// address space of this process become unevictable. This is needed
// for testing cgroup oom killer.
#ifdef __APPLE__
if (mlock(rss, size.bytes()) != 0)
#else
if (mlockall(MCL_FUTURE) != 0)
#endif
{
return ErrnoError(Failed to make pages to be mapped unevictable);
}
```
The mlockall(MCL_FUTURE) has to be called before the memory allocation is made
(because it affects future allocations), whereas mlock() is called for already
allocated pages. I could probably change mlockall(MCL_FUTURE) to
mlockall(MCL_CURRENT) in your snippet and make it work that way. I'll test to
verify and will update the patch accordingly. Thanks!
- Artem
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/#review94001
---
On Aug. 3, 2015, 11:32 p.m., Artem Harutyunyan wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/
---
(Updated Aug. 3, 2015, 11:32 p.m.)
Review request for mesos, Benjamin Hindman and Michael Park.
Bugs: MESOS-3197
https://issues.apache.org/jira/browse/MESOS-3197
Repository: mesos
Description
---
See summary.
Diffs
-
src/tests/containerizer/memory_test_helper.cpp
5e40b747f4266e7532baf8fd02ea5db0955124d2
Diff: https://reviews.apache.org/r/37065/diff/
Testing
---
make check
Thanks,
Artem Harutyunyan
Re: Review Request 37065: Fixed MemIsolatorTest failure on OSX.
On Aug. 4, 2015, 6:36 a.m., Michael Park wrote:
src/tests/containerizer/memory_test_helper.cpp, line 81
https://reviews.apache.org/r/37065/diff/1/?file=1028301#file1028301line81
What's the significance in the order in which these are called? I
would've expected something like:
```
// Make sure that all pages that are going to be mapped into the
// address space of this process become unevictable. This is needed
// for testing cgroup oom killer.
#ifdef __APPLE__
if (mlock(rss, size.bytes()) != 0)
#else
if (mlockall(MCL_FUTURE) != 0)
#endif
{
return ErrnoError(Failed to make pages to be mapped unevictable);
}
```
Artem Harutyunyan wrote:
The mlockall(MCL_FUTURE) has to be called before the memory allocation is
made (because it affects future allocations), whereas mlock() is called for
already allocated pages. I could probably change mlockall(MCL_FUTURE) to
mlockall(MCL_CURRENT) in your snippet and make it work that way. I'll test to
verify and will update the patch accordingly. Thanks!
IMHO it would be better to add an autoconf check for ```mlockall```
availability that to check ```__APPLE__```.
- James
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/#review94001
---
On Aug. 4, 2015, 6:32 a.m., Artem Harutyunyan wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37065/
---
(Updated Aug. 4, 2015, 6:32 a.m.)
Review request for mesos, Benjamin Hindman and Michael Park.
Bugs: MESOS-3197
https://issues.apache.org/jira/browse/MESOS-3197
Repository: mesos
Description
---
See summary.
Diffs
-
src/tests/containerizer/memory_test_helper.cpp
5e40b747f4266e7532baf8fd02ea5db0955124d2
Diff: https://reviews.apache.org/r/37065/diff/
Testing
---
make check
Thanks,
Artem Harutyunyan
Review Request 37065: Fixed MemIsolatorTest failure on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37065/ --- Review request for mesos, Benjamin Hindman and switched to 'mcypark'. Bugs: MESOS-3197 https://issues.apache.org/jira/browse/MESOS-3197 Repository: mesos Description --- See summary. Diffs - src/tests/containerizer/memory_test_helper.cpp 5e40b747f4266e7532baf8fd02ea5db0955124d2 Diff: https://reviews.apache.org/r/37065/diff/ Testing --- make check Thanks, Artem Harutyunyan
