You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2018/08/23 11:02:38 UTC
Review Request 68484: Enforced disabling boost debug mode.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/
-----------------------------------------------------------
Review request for mesos and Alexander Rukletsov.
Repository: mesos
Description
-------
Enforced disabling boost debug mode.
Diffs
-----
src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
Diff: https://reviews.apache.org/r/68484/diff/1/
Testing
-------
Still todo.
Thanks,
Benno Evers
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 10:44 a.m., Benjamin Bannier wrote:
> > src/master/master.hpp
> > Lines 33-34 (patched)
> > <https://reviews.apache.org/r/68484/diff/1/?file=2076510#file2076510line33>
> >
> > IMO this solution is not maintable and too low level in Mesos proper. I believe we should instead do one of following:
> >
> > 1. Provide a header and type in `stout` wrapping `boost/circular_buffer.hpp` which performs this setup. That way Mesos does not have to worry about low-level details which are nicely encapsulated in `stout`.
> > 2. Add these defines whenever we make use of Boost (simple in cmake, slightly more messy in autotools). That way users cannot forget to add them when using `boost/circular_buffer.hpp`.
> >
> > I personally would strongly prefer 1 over 2.
We certainly shouldn't do (2), I just recently had a review where I removed exactly such code (https://reviews.apache.org/r/67632/) because it can lead to hard-to-understand build errors when trying to build things outside the Mesos, i.e. building Frameworks.
We might think about (1), but it's not as clear-cut as you suggest because Boost's behaviour is not unconditionally a bug, it only becomes one due to the specific interaction with the request batching code in master. If any of our public headers end up including this new `stout/circular_buffer.hpp`, this again might create problems for users including this header outside of the Mesos build system.
Either way, I would suggest not doing any of this in *this* review, since there are two many open design questions to be discussed (i.e. do we make `stout/boost.hpp` or one header for every individual feature used, do we provide methods to configure the default options, etc.), and we'd be holding up the 1.7 release while we argue over them.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207865
-----------------------------------------------------------
On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 4:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 10:44 a.m., Benjamin Bannier wrote:
> > src/master/master.hpp
> > Lines 33-34 (patched)
> > <https://reviews.apache.org/r/68484/diff/1/?file=2076510#file2076510line33>
> >
> > IMO this solution is not maintable and too low level in Mesos proper. I believe we should instead do one of following:
> >
> > 1. Provide a header and type in `stout` wrapping `boost/circular_buffer.hpp` which performs this setup. That way Mesos does not have to worry about low-level details which are nicely encapsulated in `stout`.
> > 2. Add these defines whenever we make use of Boost (simple in cmake, slightly more messy in autotools). That way users cannot forget to add them when using `boost/circular_buffer.hpp`.
> >
> > I personally would strongly prefer 1 over 2.
>
> Benno Evers wrote:
> We certainly shouldn't do (2), I just recently had a review where I removed exactly such code (https://reviews.apache.org/r/67632/) because it can lead to hard-to-understand build errors when trying to build things outside the Mesos, i.e. building Frameworks.
>
> We might think about (1), but it's not as clear-cut as you suggest because Boost's behaviour is not unconditionally a bug, it only becomes one due to the specific interaction with the request batching code in master. If any of our public headers end up including this new `stout/circular_buffer.hpp`, this again might create problems for users including this header outside of the Mesos build system.
>
> Either way, I would suggest not doing any of this in *this* review, since there are two many open design questions to be discussed (i.e. do we make `stout/boost.hpp` or one header for every individual feature used, do we provide methods to configure the default options, etc.), and we'd be holding up the 1.7 release while we argue over them.
After offline discussions, I created a follow-up review implementing (1).
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207865
-----------------------------------------------------------
On Aug. 24, 2018, 12:28 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 12:28 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Boost.CircularBuffer includes optional debugging code that counts the
> number of currently existing iterators to the container. Up to Boost
> 1.62, this code was enabled by default.
>
> However, the existing iterators were stored in a linked list that was
> updated without any synchronization, leading to potential segfaults
> when reading the same buffer from multiple threads.
>
> Given that the Master stores the completed tasks for each framework
> in a circular buffer, and that this can be read from multiple threads
> when multiple batched requests to the `/state` endpoint are processed
> (introduced by MESOS-9122), we have to unconditionally disable Boost's
> debug mode to prevent possible segfaults.
>
>
> Diffs
> -----
>
> src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
>
>
> Diff: https://reviews.apache.org/r/68484/diff/2/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207865
-----------------------------------------------------------
src/master/master.hpp
Lines 33-34 (patched)
<https://reviews.apache.org/r/68484/#comment291428>
IMO this solution is not maintable and too low level in Mesos proper. I believe we should instead do one of following:
1. Provide a header and type in `stout` wrapping `boost/circular_buffer.hpp` which performs this setup. That way Mesos does not have to worry about low-level details which are nicely encapsulated in `stout`.
2. Add these defines whenever we make use of Boost (simple in cmake, slightly more messy in autotools). That way users cannot forget to add them when using `boost/circular_buffer.hpp`.
I personally would strongly prefer 1 over 2.
- Benjamin Bannier
On Aug. 23, 2018, 6:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 6:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207823
-----------------------------------------------------------
Patch looks great!
Reviews applied: [68484]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 4:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 23, 2018, 8:08 p.m., Benjamin Mahler wrote:
> > I suppose we need a clear boost minimum version, it's a little surprising that we let a user run with boost 1.53?
> >
> > With our other bundled libraries, there are often clear reasons why we bundle specific versions (e.g. bug fixes), and going below them would be a problem.
I'd go with this patch for now to unblock the 1.7.0 release. Bundled vs. unbundled is an area where different opinions exist, I'm not sure we can impose a minimum for boost so short before the 1.7.0 release.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207832
-----------------------------------------------------------
On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 4:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 23, 2018, 8:08 p.m., Benjamin Mahler wrote:
> > I suppose we need a clear boost minimum version, it's a little surprising that we let a user run with boost 1.53?
> >
> > With our other bundled libraries, there are often clear reasons why we bundle specific versions (e.g. bug fixes), and going below them would be a problem.
>
> Alexander Rukletsov wrote:
> I'd go with this patch for now to unblock the 1.7.0 release. Bundled vs. unbundled is an area where different opinions exist, I'm not sure we can impose a minimum for boost so short before the 1.7.0 release.
Given that Mesos seems to work fine with Boost 1.53 and there's an organization actively putting resources into ensuring it continues to run fine, clearly the suggested minimum version shouldn't be higher than 1.53 if we add it?
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207832
-----------------------------------------------------------
On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 4:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207832
-----------------------------------------------------------
I suppose we need a clear boost minimum version, it's a little surprising that we let a user run with boost 1.53?
With our other bundled libraries, there are often clear reasons why we bundle specific versions (e.g. bug fixes), and going below them would be a problem.
- Benjamin Mahler
On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 4:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207821
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.hpp
Lines 31 (patched)
<https://reviews.apache.org/r/68484/#comment291345>
Nit: s/version/versions/
`slave.cpp` also includes `boost/circular_buffer.hpp`, we should do the same there.
- Gastón Kleiman
On Aug. 23, 2018, 9:25 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 9:25 a.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 7:52 a.m., Alexander Rukletsov wrote:
> > Maybe mention that folks should consider moving away from boost 1.53 in `upgrades.md`?
I dont think so, switching Boost version can be a huge pain, and if we get this fix in time for 1.7 then all supported Mesos versions actually work fine with 1.53.
Indeed, from a consumer perspective 1.53 seems to be the ideal version to be on right now, since any incompatibilities will be fixed with high priority.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207855
-----------------------------------------------------------
On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 4:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Added macros to unconditionally disable
boost debug mode.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 24, 2018, 7:52 a.m., Alexander Rukletsov wrote:
> > Maybe mention that folks should consider moving away from boost 1.53 in `upgrades.md`?
>
> Benno Evers wrote:
> I dont think so, switching Boost version can be a huge pain, and if we get this fix in time for 1.7 then all supported Mesos versions actually work fine with 1.53.
>
> Indeed, from a consumer perspective 1.53 seems to be the ideal version to be on right now, since any incompatibilities will be fixed with high priority.
Good point, Benno. Just checked that we use 1.53 in DC/OS, hence the sentiment that it is one of the most tested combination is correct : )
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207855
-----------------------------------------------------------
On Aug. 24, 2018, 11:51 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 11:51 a.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Boost.CircularBuffer includes optional debugging code that
> counts the number of currently existing iterators to the
> container. Up to Boost 1.62, this code was enabled by
> default.
>
> However, the existing iterators were stored in a linked list
> that was updated without any synchronization, leading to
> potential segfaults when reading the same buffer from multiple
> threads.
>
> Given that the Master stores the completed tasks for each framework
> in a circular buffer, and that this can be read from multiple threads
> when multiple batched requests to the `/state` endpoint are processed,
> we have to unconditionally disable Boost's debug mode to prevent
> possible segfaults.
>
>
> Diffs
> -----
>
> src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
>
>
> Diff: https://reviews.apache.org/r/68484/diff/2/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207855
-----------------------------------------------------------
Maybe mention that folks should consider moving away from boost 1.53 in `upgrades.md`?
src/master/master.hpp
Lines 30-31 (patched)
<https://reviews.apache.org/r/68484/#comment291411>
Why wrapping onto a new line? We can save one line ; )
src/master/master.hpp
Line 28 (original), 33-35 (patched)
<https://reviews.apache.org/r/68484/#comment291410>
Please the same in slave.hpp
- Alexander Rukletsov
On Aug. 23, 2018, 4:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 4:25 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Always disable boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 4:41 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.hpp
> > Lines 27 (patched)
> > <https://reviews.apache.org/r/68484/diff/2/?file=2077072#file2077072line27>
> >
> > Let's just copy the comments here so the reader doesn't need to refer to another file and also leave a TODO.
It sounds like a good idea, but both comments are unified anyways later on in the chain so I think it's fine to leave it as is here.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207892
-----------------------------------------------------------
On Aug. 24, 2018, 12:28 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 12:28 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Boost.CircularBuffer includes optional debugging code that counts the
> number of currently existing iterators to the container. Up to Boost
> 1.62, this code was enabled by default.
>
> However, the existing iterators were stored in a linked list that was
> updated without any synchronization, leading to potential segfaults
> when reading the same buffer from multiple threads.
>
> Given that the Master stores the completed tasks for each framework
> in a circular buffer, and that this can be read from multiple threads
> when multiple batched requests to the `/state` endpoint are processed
> (introduced by MESOS-9122), we have to unconditionally disable Boost's
> debug mode to prevent possible segfaults.
>
>
> Diffs
> -----
>
> src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
>
>
> Diff: https://reviews.apache.org/r/68484/diff/2/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 4:41 p.m., Chun-Hung Hsiao wrote:
> > src/master/master.hpp
> > Lines 32 (patched)
> > <https://reviews.apache.org/r/68484/diff/2/?file=2077071#file2077071line32>
> >
> > Let's leave a TODO here:
> > ```
> > // TODO(bevers): Remove this once we bump the minimum Boost version.
> > ```
Even for later boost versions we need to keep this definition to ensure the build fails due to conflicting definitions when a user tries to manually set the macro because he wants to use boost's debug features.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207892
-----------------------------------------------------------
On Aug. 24, 2018, 12:28 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 12:28 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Boost.CircularBuffer includes optional debugging code that counts the
> number of currently existing iterators to the container. Up to Boost
> 1.62, this code was enabled by default.
>
> However, the existing iterators were stored in a linked list that was
> updated without any synchronization, leading to potential segfaults
> when reading the same buffer from multiple threads.
>
> Given that the Master stores the completed tasks for each framework
> in a circular buffer, and that this can be read from multiple threads
> when multiple batched requests to the `/state` endpoint are processed
> (introduced by MESOS-9122), we have to unconditionally disable Boost's
> debug mode to prevent possible segfaults.
>
>
> Diffs
> -----
>
> src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
>
>
> Diff: https://reviews.apache.org/r/68484/diff/2/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207892
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.hpp
Lines 32 (patched)
<https://reviews.apache.org/r/68484/#comment291506>
Let's leave a TODO here:
```
// TODO(bevers): Remove this once we bump the minimum Boost version.
```
src/slave/slave.hpp
Lines 27 (patched)
<https://reviews.apache.org/r/68484/#comment291507>
Let's just copy the comments here so the reader doesn't need to refer to another file and also leave a TODO.
- Chun-Hung Hsiao
On Aug. 24, 2018, 12:28 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 12:28 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Boost.CircularBuffer includes optional debugging code that counts the
> number of currently existing iterators to the container. Up to Boost
> 1.62, this code was enabled by default.
>
> However, the existing iterators were stored in a linked list that was
> updated without any synchronization, leading to potential segfaults
> when reading the same buffer from multiple threads.
>
> Given that the Master stores the completed tasks for each framework
> in a circular buffer, and that this can be read from multiple threads
> when multiple batched requests to the `/state` endpoint are processed
> (introduced by MESOS-9122), we have to unconditionally disable Boost's
> debug mode to prevent possible segfaults.
>
>
> Diffs
> -----
>
> src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
>
>
> Diff: https://reviews.apache.org/r/68484/diff/2/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207882
-----------------------------------------------------------
PASS: Mesos patch 68484 was successfully built and tested.
Reviews applied: `['68484']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2234/mesos-review-68484
- Mesos Reviewbot Windows
On Aug. 24, 2018, 12:28 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 12:28 p.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9177
> https://issues.apache.org/jira/browse/MESOS-9177
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Boost.CircularBuffer includes optional debugging code that counts the
> number of currently existing iterators to the container. Up to Boost
> 1.62, this code was enabled by default.
>
> However, the existing iterators were stored in a linked list that was
> updated without any synchronization, leading to potential segfaults
> when reading the same buffer from multiple threads.
>
> Given that the Master stores the completed tasks for each framework
> in a circular buffer, and that this can be read from multiple threads
> when multiple batched requests to the `/state` endpoint are processed
> (introduced by MESOS-9122), we have to unconditionally disable Boost's
> debug mode to prevent possible segfaults.
>
>
> Diffs
> -----
>
> src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
> src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
>
>
> Diff: https://reviews.apache.org/r/68484/diff/2/
>
>
> Testing
> -------
>
> * Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
> * Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
> * Recompiled mesos with this patch applied.
> * Verified that the above script does not produce a segfault anymore.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68484: Unconditionally disabled boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/
-----------------------------------------------------------
(Updated Aug. 24, 2018, 12:28 p.m.)
Review request for mesos and Alexander Rukletsov.
Summary (updated)
-----------------
Unconditionally disabled boost debug mode.
Bugs: MESOS-9177
https://issues.apache.org/jira/browse/MESOS-9177
Repository: mesos
Description (updated)
-------
Boost.CircularBuffer includes optional debugging code that counts the
number of currently existing iterators to the container. Up to Boost
1.62, this code was enabled by default.
However, the existing iterators were stored in a linked list that was
updated without any synchronization, leading to potential segfaults
when reading the same buffer from multiple threads.
Given that the Master stores the completed tasks for each framework
in a circular buffer, and that this can be read from multiple threads
when multiple batched requests to the `/state` endpoint are processed
(introduced by MESOS-9122), we have to unconditionally disable Boost's
debug mode to prevent possible segfaults.
Diffs
-----
src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
Diff: https://reviews.apache.org/r/68484/diff/2/
Testing
-------
* Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
* Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
* Recompiled mesos with this patch applied.
* Verified that the above script does not produce a segfault anymore.
Thanks,
Benno Evers
Re: Review Request 68484: Added macros to unconditionally disable
boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/
-----------------------------------------------------------
(Updated Aug. 24, 2018, 11:51 a.m.)
Review request for mesos and Alexander Rukletsov.
Changes
-------
Expanded description.
Summary (updated)
-----------------
Added macros to unconditionally disable boost debug mode.
Bugs: MESOS-9177
https://issues.apache.org/jira/browse/MESOS-9177
Repository: mesos
Description (updated)
-------
Boost.CircularBuffer includes optional debugging code that
counts the number of currently existing iterators to the
container. Up to Boost 1.62, this code was enabled by
default.
However, the existing iterators were stored in a linked list
that was updated without any synchronization, leading to
potential segfaults when reading the same buffer from multiple
threads.
Given that the Master stores the completed tasks for each framework
in a circular buffer, and that this can be read from multiple threads
when multiple batched requests to the `/state` endpoint are processed,
we have to unconditionally disable Boost's debug mode to prevent
possible segfaults.
Diffs (updated)
-----
src/master/master.hpp dc0080b24f19b77a4de34ab24aece657726343b8
src/slave/slave.hpp 0420109ac93e1249906c52437e5859c5ee033fb6
Diff: https://reviews.apache.org/r/68484/diff/2/
Changes: https://reviews.apache.org/r/68484/diff/1-2/
Testing
-------
* Compiled mesos master against boost 1.53 from https://downloads.mesosphere.com/pkgpanda-artifact-cache/boost_1_53_0.tar.gz.
* Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
* Recompiled mesos with this patch applied.
* Verified that the above script does not produce a segfault anymore.
Thanks,
Benno Evers
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/
-----------------------------------------------------------
(Updated Aug. 23, 2018, 4:25 p.m.)
Review request for mesos and Alexander Rukletsov.
Bugs: MESOS-9177
https://issues.apache.org/jira/browse/MESOS-9177
Repository: mesos
Description (updated)
-------
Always disable boost debug mode.
Diffs
-----
src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
Diff: https://reviews.apache.org/r/68484/diff/1/
Testing (updated)
-------
* Compiled mesos master against boost 1.53.
* Wrote a script that can trigger a segfault by reading from a circular buffer in parallel (pasted in the linked ticket) and verified that it crashes mesos master.
* Recompiled mesos with this patch applied.
* Verified that the above script does not produce a segfault anymore.
Thanks,
Benno Evers
Re: Review Request 68484: Enforced disabling boost debug mode.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68484/#review207812
-----------------------------------------------------------
PASS: Mesos patch 68484 was successfully built and tested.
Reviews applied: `['68484']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2225/mesos-review-68484
- Mesos Reviewbot Windows
On Aug. 23, 2018, 11:02 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68484/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 11:02 a.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enforced disabling boost debug mode.
>
>
> Diffs
> -----
>
> src/master/master.hpp 85ef14c1cc72180b746a5f4375769b653cbe511d
>
>
> Diff: https://reviews.apache.org/r/68484/diff/1/
>
>
> Testing
> -------
>
> Still todo.
>
>
> Thanks,
>
> Benno Evers
>
>