You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2017/06/23 20:16:02 UTC
Review Request 60397: Check perf version compatibility in tests with
disabled coredumps.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
Bugs: MESOS-7160
https://issues.apache.org/jira/browse/MESOS-7160
Repository: mesos
Description
-------
For autotools build, the docker-build script performs a 'distcheck'
build. This type of build warns if any unexpected files are left in
the build directory after an uninstall, mainly to detect broken
uninstall Makefile rules. The return status of the build container is
the result of the distcheck.
This fixes an issue where in some dockerized configurations
invocations of 'perf' segfaulted (producing a core file as a
side-effect), where the failure case was already anticipated and
handled by the caller.
Diffs
-----
src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
Diff: https://reviews.apache.org/r/60397/diff/1/
Testing
-------
1. make check (mac os x 10.12, fedora 25)
2. internal CI
Needs to be confirmed by Apache CI, e.g., reviewbot.
Thanks,
Andrei Budnik
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On July 7, 2017, 1:15 p.m., Alexander Rojas wrote:
> > src/linux/perf.cpp
> > Lines 243 (patched)
> > <https://reviews.apache.org/r/60397/diff/3/?file=1768325#file1768325line243>
> >
> > Why this command? two lines later `perf::version()` is called. Isn't there a way to merge this with the `perf::version()` call?
`perf::version()` also used in tests:
```cpp
// Returns the detected perf version. Exposed for testing.
process::Future<Version> version();
```
If I move this code to `perf::version()`, then its failure might be interpreted in multiple ways:
1. `perf --version` failed => perf isn’t installed or coredumped
2. `parseVersion()` failed
`PerfTest.Version` test checks whether we are able to parse perf version. If perf isn’t installed, the test shall pass. Quote from James:
“This drops the test that verifies we can actually parse perf versions that are found in the wild. That test is necessary to catch when new systems play games with the perf version; otherwise we would just detect that the perf version isn’t supported and it would be hard to diagnose why not.”
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review179868
-----------------------------------------------------------
On July 3, 2017, 10:57 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 10:57 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040
> src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
>
>
> Diff: https://reviews.apache.org/r/60397/diff/3/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On July 7, 2017, 1:15 p.m., Alexander Rojas wrote:
> > src/linux/perf.cpp
> > Lines 245 (patched)
> > <https://reviews.apache.org/r/60397/diff/3/?file=1768325#file1768325line245>
> >
> > In this case, `LOG(ERROR)` is the wrong log level, since you may expect in certain cases for the error to occur.
> >
> > `LOG(INFO)` seems more reasonable.
>
> Andrei Budnik wrote:
> But I see `LOG(ERROR)` below in the code in `supported()`. In addition, it looks like it's a real issue when perf is broken or isn't installed. If perf isn't available, then it's the issue that should be fixed, right?
Ok. Fixed.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review179868
-----------------------------------------------------------
On July 3, 2017, 10:57 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 10:57 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040
> src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
>
>
> Diff: https://reviews.apache.org/r/60397/diff/4/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On July 7, 2017, 1:15 p.m., Alexander Rojas wrote:
> > src/linux/perf.cpp
> > Lines 245 (patched)
> > <https://reviews.apache.org/r/60397/diff/3/?file=1768325#file1768325line245>
> >
> > In this case, `LOG(ERROR)` is the wrong log level, since you may expect in certain cases for the error to occur.
> >
> > `LOG(INFO)` seems more reasonable.
But I see `LOG(ERROR)` below in the code in `supported()`. In addition, it looks like it's a real issue when perf is broken or isn't installed. If perf isn't available, then it's the issue that should be fixed, right?
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review179868
-----------------------------------------------------------
On July 3, 2017, 10:57 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 10:57 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040
> src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
>
>
> Diff: https://reviews.apache.org/r/60397/diff/3/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review179868
-----------------------------------------------------------
src/linux/perf.cpp
Lines 243 (patched)
<https://reviews.apache.org/r/60397/#comment254769>
Why this command? two lines later `perf::version()` is called. Isn't there a way to merge this with the `perf::version()` call?
src/linux/perf.cpp
Lines 245 (patched)
<https://reviews.apache.org/r/60397/#comment254767>
In this case, `LOG(ERROR)` is the wrong log level, since you may expect in certain cases for the error to occur.
`LOG(INFO)` seems more reasonable.
- Alexander Rojas
On July 3, 2017, 12:57 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 12:57 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040
> src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
>
>
> Diff: https://reviews.apache.org/r/60397/diff/3/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review179642
-----------------------------------------------------------
Ship it!
I assume this makes the workaround from `fcf8618be59ada6d1607696b13bc362bf7e85b0b` unnecessary? In that case, could you submit a RR to revert it?
- Benjamin Bannier
On July 3, 2017, 12:57 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 12:57 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040
> src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
>
>
> Diff: https://reviews.apache.org/r/60397/diff/3/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/
-----------------------------------------------------------
(Updated July 3, 2017, 10:57 a.m.)
Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
Bugs: MESOS-7160
https://issues.apache.org/jira/browse/MESOS-7160
Repository: mesos
Description
-------
For autotools build, the docker-build script performs a 'distcheck'
build. This type of build warns if any unexpected files are left in
the build directory after an uninstall, mainly to detect broken
uninstall Makefile rules. The return status of the build container is
the result of the distcheck.
This fixes an issue where in some dockerized configurations
invocations of 'perf' segfaulted (producing a core file as a
side-effect), where the failure case was already anticipated and
handled by the caller.
Diffs (updated)
-----
src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040
src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
Diff: https://reviews.apache.org/r/60397/diff/3/
Changes: https://reviews.apache.org/r/60397/diff/2-3/
Testing
-------
1. make check (mac os x 10.12, fedora 25)
2. internal CI
Needs to be confirmed by Apache CI, e.g., reviewbot.
Thanks,
Andrei Budnik
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/
-----------------------------------------------------------
(Updated June 30, 2017, 5:41 p.m.)
Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
Summary (updated)
-----------------
Check perf version compatibility in tests with disabled coredumps.
Bugs: MESOS-7160
https://issues.apache.org/jira/browse/MESOS-7160
Repository: mesos
Description
-------
For autotools build, the docker-build script performs a 'distcheck'
build. This type of build warns if any unexpected files are left in
the build directory after an uninstall, mainly to detect broken
uninstall Makefile rules. The return status of the build container is
the result of the distcheck.
This fixes an issue where in some dockerized configurations
invocations of 'perf' segfaulted (producing a core file as a
side-effect), where the failure case was already anticipated and
handled by the caller.
Diffs (updated)
-----
src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
Diff: https://reviews.apache.org/r/60397/diff/2/
Changes: https://reviews.apache.org/r/60397/diff/1-2/
Testing
-------
1. make check (mac os x 10.12, fedora 25)
2. internal CI
Needs to be confirmed by Apache CI, e.g., reviewbot.
Thanks,
Andrei Budnik
Re: Review Request 60397: WIP: Check perf version compatibility in
tests with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On June 27, 2017, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/environment.cpp
> > Line 542 (original), 542-548 (patched)
> > <https://reviews.apache.org/r/60397/diff/1/?file=1761619#file1761619line542>
> >
> > Shouldn't this just be covered by `perf::suppported`?
> >
> > Would also be great to include some additional context here, like a pointer to the ticket or mentioning that this is more likely if running within a docker image?
Ticket is [MESOS-7160](https://issues.apache.org/jira/browse/MESOS-7160).
What I've got from the description of the problem is that `perf --version` segfaults on an incompatible kernel version.
The solution for this problem is provided in this patch. Then, we have found that it is more likely caused by incorrect `os::subprocess()` behavior. So, I have suspicion that `perf --version` might never really cause segfaults. I'm going to fix `os::subprocess()` and send a new patch. Afterwards, if the problem reproduces after the fix, then I'll continue to work on this patch.
`perf::suppported()` invokes `os::subprocess()` which calls `abort()` when path to a binary is invalid. SIGABRT causes coredumps.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review178952
-----------------------------------------------------------
On June 26, 2017, 8:07 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated June 26, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
>
>
> Diff: https://reviews.apache.org/r/60397/diff/1/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Benjamin Mahler <bm...@apache.org>.
> On June 27, 2017, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/environment.cpp
> > Line 542 (original), 542-548 (patched)
> > <https://reviews.apache.org/r/60397/diff/1/?file=1761619#file1761619line542>
> >
> > Shouldn't this just be covered by `perf::suppported`?
> >
> > Would also be great to include some additional context here, like a pointer to the ticket or mentioning that this is more likely if running within a docker image?
>
> Andrei Budnik wrote:
> Ticket is [MESOS-7160](https://issues.apache.org/jira/browse/MESOS-7160).
>
> What I've got from the description of the problem is that `perf --version` segfaults on an incompatible kernel version.
> The solution for this problem is provided in this patch. Then, we have found that it is more likely caused by incorrect `os::subprocess()` behavior. So, I have suspicion that `perf --version` might never really cause segfaults. I'm going to fix `os::subprocess()` and send a new patch. Afterwards, if the problem reproduces after the fix, then I'll continue to work on this patch.
>
> `perf::suppported()` invokes `os::subprocess()` which calls `abort()` when path to a binary is invalid. SIGABRT causes coredumps.
>
> James Peach wrote:
> Both of these are true :) `perf` will crash with kernel version mismatches, and I strongly suspect there is a race in the process supervisor that can cause a similar symptom when `perf` is not installed.
I think my comment was misunderstood, I'm asking why we wouldn't have perf::supported handle this correctly, rather than every caller having to write logic to defend against an implementation issue in perf::supported.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review178952
-----------------------------------------------------------
On June 30, 2017, 5:41 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated June 30, 2017, 5:41 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
> src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
>
>
> Diff: https://reviews.apache.org/r/60397/diff/2/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: WIP: Check perf version compatibility in
tests with disabled coredumps.
Posted by James Peach <jp...@apache.org>.
> On June 27, 2017, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/environment.cpp
> > Line 542 (original), 542-548 (patched)
> > <https://reviews.apache.org/r/60397/diff/1/?file=1761619#file1761619line542>
> >
> > Shouldn't this just be covered by `perf::suppported`?
> >
> > Would also be great to include some additional context here, like a pointer to the ticket or mentioning that this is more likely if running within a docker image?
>
> Andrei Budnik wrote:
> Ticket is [MESOS-7160](https://issues.apache.org/jira/browse/MESOS-7160).
>
> What I've got from the description of the problem is that `perf --version` segfaults on an incompatible kernel version.
> The solution for this problem is provided in this patch. Then, we have found that it is more likely caused by incorrect `os::subprocess()` behavior. So, I have suspicion that `perf --version` might never really cause segfaults. I'm going to fix `os::subprocess()` and send a new patch. Afterwards, if the problem reproduces after the fix, then I'll continue to work on this patch.
>
> `perf::suppported()` invokes `os::subprocess()` which calls `abort()` when path to a binary is invalid. SIGABRT causes coredumps.
Both of these are true :) `perf` will crash with kernel version mismatches, and I strongly suspect there is a race in the process supervisor that can cause a similar symptom when `perf` is not installed.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review178952
-----------------------------------------------------------
On June 26, 2017, 8:07 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated June 26, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
>
>
> Diff: https://reviews.apache.org/r/60397/diff/1/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On June 27, 2017, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/environment.cpp
> > Line 542 (original), 542-548 (patched)
> > <https://reviews.apache.org/r/60397/diff/1/?file=1761619#file1761619line542>
> >
> > Shouldn't this just be covered by `perf::suppported`?
> >
> > Would also be great to include some additional context here, like a pointer to the ticket or mentioning that this is more likely if running within a docker image?
>
> Andrei Budnik wrote:
> Ticket is [MESOS-7160](https://issues.apache.org/jira/browse/MESOS-7160).
>
> What I've got from the description of the problem is that `perf --version` segfaults on an incompatible kernel version.
> The solution for this problem is provided in this patch. Then, we have found that it is more likely caused by incorrect `os::subprocess()` behavior. So, I have suspicion that `perf --version` might never really cause segfaults. I'm going to fix `os::subprocess()` and send a new patch. Afterwards, if the problem reproduces after the fix, then I'll continue to work on this patch.
>
> `perf::suppported()` invokes `os::subprocess()` which calls `abort()` when path to a binary is invalid. SIGABRT causes coredumps.
>
> James Peach wrote:
> Both of these are true :) `perf` will crash with kernel version mismatches, and I strongly suspect there is a race in the process supervisor that can cause a similar symptom when `perf` is not installed.
>
> Benjamin Mahler wrote:
> I think my comment was misunderstood, I'm asking why we wouldn't have perf::supported handle this correctly, rather than every caller having to write logic to defend against an implementation issue in perf::supported.
I agree with you. Fixed.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review178952
-----------------------------------------------------------
On July 3, 2017, 10:57 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 10:57 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040
> src/tests/containerizer/perf_tests.cpp d8aab08eb131f974821fb85662cbc6cc685d2f3e
>
>
> Diff: https://reviews.apache.org/r/60397/diff/3/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: WIP: Check perf version compatibility in
tests with disabled coredumps.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review178952
-----------------------------------------------------------
src/tests/environment.cpp
Line 542 (original), 542-548 (patched)
<https://reviews.apache.org/r/60397/#comment253340>
Shouldn't this just be covered by `perf::suppported`?
Would also be great to include some additional context here, like a pointer to the ticket or mentioning that this is more likely if running within a docker image?
- Benjamin Mahler
On June 26, 2017, 8:07 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated June 26, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
>
>
> Diff: https://reviews.apache.org/r/60397/diff/1/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: WIP: Check perf version compatibility in
tests with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/
-----------------------------------------------------------
(Updated June 26, 2017, 8:07 p.m.)
Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
Summary (updated)
-----------------
WIP: Check perf version compatibility in tests with disabled coredumps.
Bugs: MESOS-7160
https://issues.apache.org/jira/browse/MESOS-7160
Repository: mesos
Description
-------
For autotools build, the docker-build script performs a 'distcheck'
build. This type of build warns if any unexpected files are left in
the build directory after an uninstall, mainly to detect broken
uninstall Makefile rules. The return status of the build container is
the result of the distcheck.
This fixes an issue where in some dockerized configurations
invocations of 'perf' segfaulted (producing a core file as a
side-effect), where the failure case was already anticipated and
handled by the caller.
Diffs
-----
src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
Diff: https://reviews.apache.org/r/60397/diff/1/
Testing
-------
1. make check (mac os x 10.12, fedora 25)
2. internal CI
Needs to be confirmed by Apache CI, e.g., reviewbot.
Thanks,
Andrei Budnik
Re: Review Request 60397: WIP: Check perf version compatibility in
tests with disabled coredumps.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On June 23, 2017, 11:49 p.m., James Peach wrote:
> > Great idea! What do you think about setting the core rlimit to 0 in the the `subprocess` clone lambda in `perf::execute()`?
According to discussion in the related ticket, crashes are caused by calling abort() on failures in os::subprocess().
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review178826
-----------------------------------------------------------
On June 26, 2017, 8:07 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated June 26, 2017, 8:07 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
>
>
> Diff: https://reviews.apache.org/r/60397/diff/1/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 60397: Check perf version compatibility in tests
with disabled coredumps.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60397/#review178826
-----------------------------------------------------------
Ship it!
Great idea! What do you think about setting the core rlimit to 0 in the the `subprocess` clone lambda in `perf::execute()`?
- James Peach
On June 23, 2017, 8:16 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> -----------------------------------------------------------
>
> (Updated June 23, 2017, 8:16 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
>
>
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
>
>
> Diffs
> -----
>
> src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5
>
>
> Diff: https://reviews.apache.org/r/60397/diff/1/
>
>
> Testing
> -------
>
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
>
> Needs to be confirmed by Apache CI, e.g., reviewbot.
>
>
> Thanks,
>
> Andrei Budnik
>
>