You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2018/04/16 15:03:20 UTC
Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
Bugs: MESOS-8786
https://issues.apache.org/jira/browse/MESOS-8786
Repository: mesos
Description
-------
Access to cgroups subsystem-specific isolators is modelled with actors
which e.g., dispatch to themself on asynchronous code paths. We
previously allowed callers to directly invoke functions on these
actors which could e.g., introduce races on instance-internal data.
In this patch we refactor the `Subsystem` base class to provide a safe
interface. The abstract base class and its concrete, derived classes
style are still libprocess processes, but we now use a template method
pattern where the base class only provides a non-`virtual` `public`
interface and all customization points are `protected`. With that we
can inject the needed `dispatch` in the base class, rendering the
class hierarchy safe to use in concurrent contexts.
Diffs
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
Diff: https://reviews.apache.org/r/66635/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202447
-----------------------------------------------------------
Patch looks great!
Reviews applied: [66634, 66635]
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 April 16, 2018, 3:03 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 3:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review201302
-----------------------------------------------------------
FAIL: Failed to get dependent review IDs for the current patch.
Failed command: `python.exe C:\Jenkins\workspace\mesos-reviewbot-testing\Mesos\utils\get-review-ids.py -r 66635 -o C:\Users\mesos\AppData\Local\Temp\mesos_dependent_review_ids`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635
- Mesos Reviewbot Windows
On April 16, 2018, 3:03 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 3:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 8, 2018, 5 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp
> > Line 174 (original), 236 (patched)
> > <https://reviews.apache.org/r/66635/diff/3/?file=2017556#file2017556line236>
> >
> > It seems `subsystemName` is only used in `Subsystem::name()`, so instead of
> > introducing this member variable, can we just call `process->name()` in `Subsystem::name()`?
I went this way because we do not really have a clear pattern on how to safely mark wrapped functions which are safe to access concurrently. I think it makes sense to expose the function directly (as opposed to accessing it through a `dispatch`) since the function itself is `const`, so I removed `subsystemName`.
> On May 8, 2018, 5 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
> > Lines 122 (patched)
> > <https://reviews.apache.org/r/66635/diff/3/?file=2017557#file2017557line122>
> >
> > Mind to explain why we need `std::move` here? Want to leverage move constructor?
Semantically an `Owned` cannot be copied since it models unique ownership. MESOS-5122 should address making it impossible to copy instances, but until it is fixed we need to use `Owned` carefully and manually `move` instead of copying it implictly.
> On May 8, 2018, 5 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
> > Lines 136 (patched)
> > <https://reviews.apache.org/r/66635/diff/3/?file=2017557#file2017557line136>
> >
> > What about just making this method's return type as `Try<Subsystem*>` and then `return new Subsystem(subsystemProcess.get())` at the end of this method?
What would the benefit of that be? It would require manual memory management I believe we should avoid since we have a library (`Owned`) doing it for us.
Please feel free to reopen if I am missing something.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202619
-----------------------------------------------------------
On May 8, 2018, 5:33 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 8, 2018, 5:33 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202619
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp
Line 174 (original), 236 (patched)
<https://reviews.apache.org/r/66635/#comment284559>
It seems `subsystemName` is only used in `Subsystem::name()`, so instead of
introducing this member variable, can we just call `process->name()` in `Subsystem::name()`?
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 121 (patched)
<https://reviews.apache.org/r/66635/#comment284525>
No need `process::`?
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 122 (patched)
<https://reviews.apache.org/r/66635/#comment284536>
Mind to explain why we need `std::move` here? Want to leverage move constructor?
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 136 (patched)
<https://reviews.apache.org/r/66635/#comment284565>
What about just making this method's return type as `Try<Subsystem*>` and then `return new Subsystem(subsystemProcess.get())` at the end of this method?
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Line 83 (original), 173 (patched)
<https://reviews.apache.org/r/66635/#comment284524>
No need `process::`? Ditto for all other places in this file.
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Line 92 (original), 175 (patched)
<https://reviews.apache.org/r/66635/#comment284523>
No need `std::`? Ditto for all other places in this file.
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Line 108 (original), 199 (patched)
<https://reviews.apache.org/r/66635/#comment284522>
These two parameters should be in two lines.
- Qian Zhang
On May 7, 2018, 11:56 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 7, 2018, 11:56 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202722
-----------------------------------------------------------
Ship it!
Ship It!
- Qian Zhang
On May 8, 2018, 11:33 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 8, 2018, 11:33 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202763
-----------------------------------------------------------
PASS: Mesos patch 66635 was successfully built and tested.
Reviews applied: `['66634', '66987', '66635']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635
- Mesos Reviewbot Windows
On May 9, 2018, 7:55 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 9, 2018, 7:55 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202817
-----------------------------------------------------------
Patch looks great!
Reviews applied: [66634, 66987, 66635]
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 May 9, 2018, 7:55 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 9, 2018, 7:55 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/
-----------------------------------------------------------
(Updated May 9, 2018, 9:55 a.m.)
Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
Changes
-------
Switch order as requested by Gilbert.
Bugs: MESOS-8786
https://issues.apache.org/jira/browse/MESOS-8786
Repository: mesos
Description
-------
Different cgroups subsystems are modelled as actors. In this patch we
introduce wrapper classes which `dispatch` to the processes. This
removes e.g., races from mixing naked and `dispatch`'ed method calls.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
Diff: https://reviews.apache.org/r/66635/diff/5/
Changes: https://reviews.apache.org/r/66635/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 9, 2018, 1:10 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp
> > Lines 98 (patched)
> > <https://reviews.apache.org/r/66635/diff/4/?file=2018097#file2018097line98>
> >
> > Could we switch the order with `SubsystemProcess`? This is the semantic we follow in the agent code for practice. A forward declaration is needed.
Done, also move the impl of `Subsystem::name` to the cpp file.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202701
-----------------------------------------------------------
On May 9, 2018, 9:55 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 9, 2018, 9:55 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202701
-----------------------------------------------------------
Fix it, then Ship it!
LGTM!
src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp
Lines 98 (patched)
<https://reviews.apache.org/r/66635/#comment284682>
Could we switch the order with `SubsystemProcess`? This is the semantic we follow in the agent code for practice. A forward declaration is needed.
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp
Lines 121-135 (patched)
<https://reviews.apache.org/r/66635/#comment284683>
ditto for the order in .cpp file.
- Gilbert Song
On May 8, 2018, 8:33 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 8, 2018, 8:33 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/
-----------------------------------------------------------
(Updated May 8, 2018, 5:33 p.m.)
Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
Changes
-------
Addressed issues raised by Qian.
Bugs: MESOS-8786
https://issues.apache.org/jira/browse/MESOS-8786
Repository: mesos
Description
-------
Different cgroups subsystems are modelled as actors. In this patch we
introduce wrapper classes which `dispatch` to the processes. This
removes e.g., races from mixing naked and `dispatch`'ed method calls.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
Diff: https://reviews.apache.org/r/66635/diff/4/
Changes: https://reviews.apache.org/r/66635/diff/3-4/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202562
-----------------------------------------------------------
PASS: Mesos patch 66635 was successfully built and tested.
Reviews applied: `['66634', '66987', '66635']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635
- Mesos Reviewbot Windows
On May 7, 2018, 8:56 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 7, 2018, 8:56 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202563
-----------------------------------------------------------
Patch looks great!
Reviews applied: [66634, 66987, 66635]
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 May 7, 2018, 3:56 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 7, 2018, 3:56 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/
-----------------------------------------------------------
(Updated May 7, 2018, 5:56 p.m.)
Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
Changes
-------
Reimplemented fix as suggested by Qian.
Summary (updated)
-----------------
Introduced wrapper for access to cgroups system access.
Bugs: MESOS-8786
https://issues.apache.org/jira/browse/MESOS-8786
Repository: mesos
Description (updated)
-------
Different cgroups subsystems are modelled as actors. In this patch we
introduce wrapper classes which `dispatch` to the processes. This
removes e.g., races from mixing naked and `dispatch`'ed method calls.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
Diff: https://reviews.apache.org/r/66635/diff/3/
Changes: https://reviews.apache.org/r/66635/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202420
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On April 16, 2018, 8:03 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 8:03 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Introduced wrapper for access to cgroups
system access.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 4, 2018, 10:53 a.m., Qian Zhang wrote:
> > Thanks for the patch!
> >
> > The `doXxx()` functions seem a bit strange to me. I am wondering if we can follow the similar way of how Mesos containerizer calls isolators, e.g., we could rename `Subsystem` to `SubsystemProcess` and all the `XxxSubsystem` to `XxxSubsystemProcess` since they are all actually `Process`, and then introduce a new class `Subsystem` which internally dispatches calls to `SubsystemProcess`, and `CgroupsIsolatorProcess` always directly calls the functions of `Subsystem`.
>
> Benjamin Bannier wrote:
> Like outlined in the ticket, that would be the other possibility. Note that we not only require dedicated subsystem processes, but also wrappers derived from `Subsystem` for each one (we could use a template here to remove duplicating classes). This could still lead to a lot of duplication in terms of e.g., functions to implement. I got the feeling that the change here was less invasive. Let me know if you think the code is too strange and I can reiterate.
>
> Gilbert Song wrote:
> Duplications are tolerable. Let's follow Qian's advice if possible.
>
> Qian Zhang wrote:
> > Note that we not only require dedicated subsystem processes, but also wrappers derived from Subsystem for each one.
>
> Mind to elaborate a bit on why we need wrappers derived from `Subsystem` class for each one? I think we only need a `Subsystem` class but create multiple objects of it (one for each subsystem), and `Subsystem` should have a member variable `process`, and member functions of `Subsystem` just dispatch calls to the related functions of `process`.
You are right, they are not needed. I updated the patch to use a single wrapper class. I also renamed the processes in the preceeding https://reviews.apache.org/r/66987/.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202424
-----------------------------------------------------------
On May 7, 2018, 5:56 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated May 7, 2018, 5:56 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Different cgroups subsystems are modelled as actors. In this patch we
> introduce wrapper classes which `dispatch` to the processes. This
> removes e.g., races from mixing naked and `dispatch`'ed method calls.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 5763c9880728f02e44116fd50e62b592a8ef69b6
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 4431ce13d28035b0c5c037b2848ae03aeaf65562
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
>
>
> Diff: https://reviews.apache.org/r/66635/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 4, 2018, 10:53 a.m., Qian Zhang wrote:
> > Thanks for the patch!
> >
> > The `doXxx()` functions seem a bit strange to me. I am wondering if we can follow the similar way of how Mesos containerizer calls isolators, e.g., we could rename `Subsystem` to `SubsystemProcess` and all the `XxxSubsystem` to `XxxSubsystemProcess` since they are all actually `Process`, and then introduce a new class `Subsystem` which internally dispatches calls to `SubsystemProcess`, and `CgroupsIsolatorProcess` always directly calls the functions of `Subsystem`.
Like outlined in the ticket, that would be the other possibility. Note that we not only require dedicated subsystem processes, but also wrappers derived from `Subsystem` for each one (we could use a template here to remove duplicating classes). This could still lead to a lot of duplication in terms of e.g., functions to implement. I got the feeling that the change here was less invasive. Let me know if you think the code is too strange and I can reiterate.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202424
-----------------------------------------------------------
On April 16, 2018, 5:03 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 5:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Qian Zhang <zh...@gmail.com>.
> On May 4, 2018, 4:53 p.m., Qian Zhang wrote:
> > Thanks for the patch!
> >
> > The `doXxx()` functions seem a bit strange to me. I am wondering if we can follow the similar way of how Mesos containerizer calls isolators, e.g., we could rename `Subsystem` to `SubsystemProcess` and all the `XxxSubsystem` to `XxxSubsystemProcess` since they are all actually `Process`, and then introduce a new class `Subsystem` which internally dispatches calls to `SubsystemProcess`, and `CgroupsIsolatorProcess` always directly calls the functions of `Subsystem`.
>
> Benjamin Bannier wrote:
> Like outlined in the ticket, that would be the other possibility. Note that we not only require dedicated subsystem processes, but also wrappers derived from `Subsystem` for each one (we could use a template here to remove duplicating classes). This could still lead to a lot of duplication in terms of e.g., functions to implement. I got the feeling that the change here was less invasive. Let me know if you think the code is too strange and I can reiterate.
>
> Gilbert Song wrote:
> Duplications are tolerable. Let's follow Qian's advice if possible.
> Note that we not only require dedicated subsystem processes, but also wrappers derived from Subsystem for each one.
Mind to elaborate a bit on why we need wrappers derived from `Subsystem` class for each one? I think we only need a `Subsystem` class but create multiple objects of it (one for each subsystem), and `Subsystem` should have a member variable `process`, and member functions of `Subsystem` just dispatch calls to the related functions of `process`.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202424
-----------------------------------------------------------
On April 16, 2018, 11:03 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 11:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Gilbert Song <so...@gmail.com>.
> On May 4, 2018, 1:53 a.m., Qian Zhang wrote:
> > Thanks for the patch!
> >
> > The `doXxx()` functions seem a bit strange to me. I am wondering if we can follow the similar way of how Mesos containerizer calls isolators, e.g., we could rename `Subsystem` to `SubsystemProcess` and all the `XxxSubsystem` to `XxxSubsystemProcess` since they are all actually `Process`, and then introduce a new class `Subsystem` which internally dispatches calls to `SubsystemProcess`, and `CgroupsIsolatorProcess` always directly calls the functions of `Subsystem`.
>
> Benjamin Bannier wrote:
> Like outlined in the ticket, that would be the other possibility. Note that we not only require dedicated subsystem processes, but also wrappers derived from `Subsystem` for each one (we could use a template here to remove duplicating classes). This could still lead to a lot of duplication in terms of e.g., functions to implement. I got the feeling that the change here was less invasive. Let me know if you think the code is too strange and I can reiterate.
Duplications are tolerable. Let's follow Qian's advice if possible.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202424
-----------------------------------------------------------
On April 16, 2018, 8:03 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 8:03 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review202424
-----------------------------------------------------------
Thanks for the patch!
The `doXxx()` functions seem a bit strange to me. I am wondering if we can follow the similar way of how Mesos containerizer calls isolators, e.g., we could rename `Subsystem` to `SubsystemProcess` and all the `XxxSubsystem` to `XxxSubsystemProcess` since they are all actually `Process`, and then introduce a new class `Subsystem` which internally dispatches calls to `SubsystemProcess`, and `CgroupsIsolatorProcess` always directly calls the functions of `Subsystem`.
- Qian Zhang
On April 16, 2018, 11:03 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 11:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review201238
-----------------------------------------------------------
Patch looks great!
Reviews applied: [66634, 66635]
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 April 16, 2018, 8:03 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 8:03 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> style are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review201213
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['66634', '66635']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635
Relevant logs:
- [mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635/logs/mesos-tests-cmake-stdout.log):
```
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496): warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256): warning C4090: 'function': different 'const' qualifiers [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166): warning C4716: 'pthread_cond_broadcast': must return a value [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205): warning C4716: 'pthread_cond_wait': must return a value [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
"D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
"D:\DCOS\mesos\src\mesos.vcxproj" (default target) (20) ->
(ClCompile target) ->
d:\dcos\mesos\mesos\src\csi\paths.cpp(34): error C2039: 'unix': is not a member of 'process::network' [D:\DCOS\mesos\src\mesos.vcxproj]
d:\dcos\mesos\mesos\src\csi\paths.cpp(34): error C2878: 'unix': a namespace or class of this name does not exist [D:\DCOS\mesos\src\mesos.vcxproj]
d:\dcos\mesos\mesos\src\csi\paths.cpp(191): error C2653: 'unix': is not a class or namespace name [D:\DCOS\mesos\src\mesos.vcxproj]
d:\dcos\mesos\mesos\src\csi\paths.cpp(191): error C2039: 'create': is not a member of 'mesos::Address' [D:\DCOS\mesos\src\mesos.vcxproj]
d:\dcos\mesos\mesos\src\csi\paths.cpp(191): error C3861: 'create': identifier not found [D:\DCOS\mesos\src\mesos.vcxproj]
172 Warning(s)
5 Error(s)
Time Elapsed 00:14:41.93
```
- Mesos Reviewbot Windows
On April 16, 2018, 8:03 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 8:03 a.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> style are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 66635: Fixed potential races when interacting with
cgroup subsystem isolators.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66635/#review201309
-----------------------------------------------------------
PASS: Mesos patch 66635 was successfully built and tested.
Reviews applied: `['66634', '66635']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66635
- Mesos Reviewbot Windows
On April 16, 2018, 3:03 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66635/
> -----------------------------------------------------------
>
> (Updated April 16, 2018, 3:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-8786
> https://issues.apache.org/jira/browse/MESOS-8786
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Access to cgroups subsystem-specific isolators is modelled with actors
> which e.g., dispatch to themself on asynchronous code paths. We
> previously allowed callers to directly invoke functions on these
> actors which could e.g., introduce races on instance-internal data.
>
> In this patch we refactor the `Subsystem` base class to provide a safe
> interface. The abstract base class and its concrete, derived classes
> are still libprocess processes, but we now use a template method
> pattern where the base class only provides a non-`virtual` `public`
> interface and all customization points are `protected`. With that we
> can inject the needed `dispatch` in the base class, rendering the
> class hierarchy safe to use in concurrent contexts.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp 65c1e47a569f320b63b54e5f4fc1da374d02ee0d
> src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 9afa02b207e6272836e5a36d69fb48f1f4d02150
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp ebddf4e9812c7e1eecec99215dfa5fd8e4e6e361
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp cba21ede59916b0a6e120ecd2b6348b8a946c507
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.hpp 282a76189579d3ddd61f4aad210ce8e876ff0c25
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 75fab0f1fc7e7403855de0786b8c1155d7599b7f
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp e4977da476a04a708a2e9b9dcc5a2f85da5867f4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 3be419ad477e9baa329b5388d4c12fa743c7f563
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp 4ab224e3b55a9d2b37d5e9abe94c069d89cd6d80
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 4c43191251d7411d14ed8b11c79a64ff5d6ccd76
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.hpp d79fda3cf852e50e75dbeeeaeae50b1f471a55a4
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 50348d63f6fffa7662e6b91b5ce4ff730380e708
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.hpp bcaa16142c4e9882dea88e70095cfb7f223401ef
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp f778a419e15940d92079d04884887615322791f5
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.hpp 14c0e79f045fb826d20e476772e017439977dded
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/perf_event.cpp d394d793c6bc38e249530c54ddb868ecff7f7865
>
>
> Diff: https://reviews.apache.org/r/66635/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>