You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Avinash sridharan <av...@mesosphere.io> on 2016/01/26 07:35:45 UTC
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 6:35 a.m.)
Review request for mesos and Jie Yu.
Summary (updated)
-----------------
Modified the `cgroups/net_cls` isolator to use the `NetClsHandleMgr`.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Modified the `cgroups/net_cls` isolator to use the `NetClsHandleMgr` to manage net_cls handles.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 7ce5243ed7476a1cfd1371b5ce25b62dc07432db
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 0798de747627ccc45b01a2668e16693757dc69a8
Diff: https://reviews.apache.org/r/42618/diff/
Testing (updated)
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Feb. 1, 2016, 9:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 454-455
> > <https://reviews.apache.org/r/42618/diff/3/?file=1227395#file1227395line454>
> >
> > In fact, i think we should add a helper in src/linux/cgroup.hpp (like we did for other cgroups controls like cpu.shares, etc.)
> >
> > ```
> > Try<Nothing> cgroups::net_cls::classid(uint32_t classid)
> > ```
Will do. Will create a separate patch and make this dependent on the new patch.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117275
-----------------------------------------------------------
On Jan. 31, 2016, 8:06 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2016, 8:06 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `NetClsHandleMgr` will be responsible for keeping track of the net_cls
> handles that will be used by the net_cls cgroup subsystem.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 1, 2016, 9:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 454-455
> > <https://reviews.apache.org/r/42618/diff/3/?file=1227395#file1227395line454>
> >
> > In fact, i think we should add a helper in src/linux/cgroup.hpp (like we did for other cgroups controls like cpu.shares, etc.)
> >
> > ```
> > Try<Nothing> cgroups::net_cls::classid(uint32_t classid)
> > ```
>
> Avinash sridharan wrote:
> Will do. Will create a separate patch and make this dependent on the new patch.
Yeah, that's fine.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117275
-----------------------------------------------------------
On Jan. 31, 2016, 8:06 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2016, 8:06 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `NetClsHandleMgr` will be responsible for keeping track of the net_cls
> handles that will be used by the net_cls cgroup subsystem.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117275
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 454 - 455)
<https://reviews.apache.org/r/42618/#comment178426>
In fact, i think we should add a helper in src/linux/cgroup.hpp (like we did for other cgroups controls like cpu.shares, etc.)
```
Try<Nothing> cgroups::net_cls::classid(uint32_t classid)
```
- Jie Yu
On Jan. 31, 2016, 8:06 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2016, 8:06 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `NetClsHandleMgr` will be responsible for keeping track of the net_cls
> handles that will be used by the net_cls cgroup subsystem.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Feb. 1, 2016, 8:14 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 38-39
> > <https://reviews.apache.org/r/42618/diff/3/?file=1227394#file1227394line38>
> >
> > What do we need this? Should we expose that using agent flags.
This is the default major handle we are using for this commit. In MESOS-4344 we are moving this to an agent flag. However for MESOS-4345 we need this be a #define.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117243
-----------------------------------------------------------
On Jan. 31, 2016, 8:06 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2016, 8:06 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `NetClsHandleMgr` will be responsible for keeping track of the net_cls
> handles that will be used by the net_cls cgroup subsystem.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Feb. 1, 2016, 8:14 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 440-443
> > <https://reviews.apache.org/r/42618/diff/3/?file=1227395#file1227395line440>
> >
> > Can you try to see if writing a decimal number if fine as well? If not, we should move the helper function into cpp as a file local helper.
Writing a decimal number works. Have moved this into a helper function in cgroups, as a separate patch.
> On Feb. 1, 2016, 8:14 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 491-497
> > <https://reviews.apache.org/r/42618/diff/3/?file=1227395#file1227395line491>
> >
> > I don't think this is necessary.
Agreed.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117243
-----------------------------------------------------------
On Feb. 2, 2016, 6:32 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2016, 6:32 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Feb. 1, 2016, 8:14 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 499-503
> > <https://reviews.apache.org/r/42618/diff/3/?file=1227395#file1227395line499>
> >
> > You should do this after the cgroup has been destroyed.
Makes sense. Planning to convert the lamdba being used in the defer for cleanup into a function, since we are adding more logic to the Lambda now. Hope that's fine?
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117243
-----------------------------------------------------------
On Jan. 31, 2016, 8:06 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2016, 8:06 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `NetClsHandleMgr` will be responsible for keeping track of the net_cls
> handles that will be used by the net_cls cgroup subsystem.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117243
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 38 - 39)
<https://reviews.apache.org/r/42618/#comment178408>
What do we need this? Should we expose that using agent flags.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 282 - 294)
<https://reviews.apache.org/r/42618/#comment178368>
I would pull this into a helper function in src/linux/cgroup.hpp|cpp
```
Try<uint32_t> cgroups::net_cls::classid(hierachy, cgroup);
```
This can be in a separate patch.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 285 - 286)
<https://reviews.apache.org/r/42618/#comment178372>
Please align the error message accordingly.
```
return Failure(
"Unable to ..." +
stringify(...));
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 292 - 293)
<https://reviews.apache.org/r/42618/#comment178373>
Ditto here.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 300 - 301)
<https://reviews.apache.org/r/42618/#comment178376>
Ditto on alignment.
Do you also need to clear 'infos' to be consistent?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 329 - 334)
<https://reviews.apache.org/r/42618/#comment178377>
For known orphans, you want to recovery the classid as well. Otherwise, those handles will be allocated to others while the known orphan containers are still using it.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 413)
<https://reviews.apache.org/r/42618/#comment178378>
s/netHandle/handle/
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 418)
<https://reviews.apache.org/r/42618/#comment178379>
We typically put '+' at the end of the previous line.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 440 - 443)
<https://reviews.apache.org/r/42618/#comment178403>
Can you try to see if writing a decimal number if fine as well? If not, we should move the helper function into cpp as a file local helper.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 454)
<https://reviews.apache.org/r/42618/#comment178404>
s/allocate/write/
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 457 - 458)
<https://reviews.apache.org/r/42618/#comment178405>
Ditto on error message alignment.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 491 - 497)
<https://reviews.apache.org/r/42618/#comment178406>
I don't think this is necessary.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 499 - 503)
<https://reviews.apache.org/r/42618/#comment178407>
You should do this after the cgroup has been destroyed.
- Jie Yu
On Jan. 31, 2016, 8:06 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2016, 8:06 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `NetClsHandleMgr` will be responsible for keeping track of the net_cls
> handles that will be used by the net_cls cgroup subsystem.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Feb. 5, 2016, 12:31 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, line 200
> > <https://reviews.apache.org/r/42618/diff/7/?file=1231964#file1231964line200>
> >
> > What's defaultPrimary? Who is the user? Can you just do the following:
> >
> > ```
> > if (!primaries.empty()) {
> > handleManager = NetClsHandleManager(primaries.get());
> > }
> > ```
Have converted defaultPrimary as a helper function in `CgroupsNetClsIsolatorProcess` . Since the defaultPrimary is a concept specific to the isolator, and it needs to store the ranges, on which the defaultPrimary operates, as well. Hence, have introduced an `IntervalSet<uint16_t>` field in `CgroupsNetClsIsolatorProcess` as well.
> On Feb. 5, 2016, 12:31 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, line 408
> > <https://reviews.apache.org/r/42618/diff/7/?file=1231964#file1231964line408>
> >
> > Can you make 'defaultPrimiary' a helper method instead of a field member. The field member will go away in the future once we introduce other allocation policies like one primary per framweork.
Please see my comments above about introducing an IntervalSet<uint16_t> field in `CgroupsNetClsIsolatorProcess`.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117916
-----------------------------------------------------------
On Feb. 5, 2016, 3:31 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 3:31 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 85004aea4c1ee4b25e106f3ce40025c69f1ce030
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp bf6c88341dedc0a37546c04f38197c892b498684
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117916
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 38)
<https://reviews.apache.org/r/42618/#comment179186>
No need for this.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 160)
<https://reviews.apache.org/r/42618/#comment179180>
kill this line
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 161)
<https://reviews.apache.org/r/42618/#comment179202>
Hum, this should be Option<NetClsHandle> handle.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 167)
<https://reviews.apache.org/r/42618/#comment179182>
Can you use an empty IntervetSet to represent the None case? I.e., no need to use Option here.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 174)
<https://reviews.apache.org/r/42618/#comment179178>
No need for the leading underscore.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 200)
<https://reviews.apache.org/r/42618/#comment179184>
What's defaultPrimary? Who is the user? Can you just do the following:
```
if (!primaries.empty()) {
handleManager = NetClsHandleManager(primaries.get());
}
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 243 - 245)
<https://reviews.apache.org/r/42618/#comment179187>
No need to for this. Can you set the default behavior to be not allocating handles?
You may need to set a default value for 'primaries' in CgroupsNetClsIsolatorProcess constructor.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 265 - 267)
<https://reviews.apache.org/r/42618/#comment179188>
Make it less jagged. Also, no need to quote containerId because it's generated by Mesos (guaranteed no space in it, you only need to quote something that user provides).
```
return Failure(
"Failed to check cgroup for container " +
stringify(containerId));
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 284 - 285)
<https://reviews.apache.org/r/42618/#comment179189>
Ditto on remove quote for containerId.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 320 - 322)
<https://reviews.apache.org/r/42618/#comment179190>
Ditto.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 407 - 408)
<https://reviews.apache.org/r/42618/#comment179191>
This fits one line?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 408)
<https://reviews.apache.org/r/42618/#comment179193>
Can you make 'defaultPrimiary' a helper method instead of a field member. The field member will go away in the future once we introduce other allocation policies like one primary per framweork.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 412)
<https://reviews.apache.org/r/42618/#comment179197>
space after ':'
s/for the container//
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 441)
<https://reviews.apache.org/r/42618/#comment179198>
Ditto on removing quote.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 449)
<https://reviews.apache.org/r/42618/#comment179200>
I would rather use handle.isSome as the condition here.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 503)
<https://reviews.apache.org/r/42618/#comment179201>
I would use handle.isSOme() as the condition here.
- Jie Yu
On Feb. 4, 2016, 1:42 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 1:42 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review118133
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Feb. 5, 2016, 10:24 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 10:24 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp d8cebd7be97170c799016f3c60d91d2a4de9510d
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp f96c3274610269eacc9e91fec21a47a06bfeea62
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review118122
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42982, 42983, 43096, 42618]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 5, 2016, 10:24 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 10:24 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp d8cebd7be97170c799016f3c60d91d2a4de9510d
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp f96c3274610269eacc9e91fec21a47a06bfeea62
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 10:24 p.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp d8cebd7be97170c799016f3c60d91d2a4de9510d
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp f96c3274610269eacc9e91fec21a47a06bfeea62
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 3:31 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
Making sure comments are tw=70
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 85004aea4c1ee4b25e106f3ce40025c69f1ce030
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp bf6c88341dedc0a37546c04f38197c892b498684
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 3:27 p.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 85004aea4c1ee4b25e106f3ce40025c69f1ce030
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp bf6c88341dedc0a37546c04f38197c892b498684
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Feb. 4, 2016, 1:42 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/#review117646
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 171)
<https://reviews.apache.org/r/42618/#comment178925>
s/netClsHandle/handle/
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 194)
<https://reviews.apache.org/r/42618/#comment178909>
s/netClsHandleMgt/handleManager/
`netCls` is already part of the class name, no need to state that in the variable name again.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 407)
<https://reviews.apache.org/r/42618/#comment178924>
I would just say: "Failed to allocate a net_cls handle for ..."
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 438)
<https://reviews.apache.org/r/42618/#comment178929>
This is not allocation, right? This is assignment. So
```
// Assign the handle to the cgroup.
```
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 439)
<https://reviews.apache.org/r/42618/#comment178928>
No need for this temp variable.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 480)
<https://reviews.apache.org/r/42618/#comment178930>
Why onAny here? Shouldn't you use .then here?
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 481)
<https://reviews.apache.org/r/42618/#comment178913>
we align paramters with the first parameter, not '('
```
return ...
.onAny(defer(...),
&CgroupsNet...
...);
```
Please do a sweep to fix all such occurances.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 486)
<https://reviews.apache.org/r/42618/#comment178914>
2 lines apart.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 488 - 490)
<https://reviews.apache.org/r/42618/#comment178915>
indentation.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 492)
<https://reviews.apache.org/r/42618/#comment178932>
s/freeHandle/free/
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 495)
<https://reviews.apache.org/r/42618/#comment178911>
Indentation.
s/for this cgroup//
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 502)
<https://reviews.apache.org/r/42618/#comment178920>
`s/_reserveNetClsHandle/recoverHandle/`
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 506 - 522)
<https://reviews.apache.org/r/42618/#comment178912>
Please fix the indentation.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 507)
<https://reviews.apache.org/r/42618/#comment178916>
s/handle/classid
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 511)
<https://reviews.apache.org/r/42618/#comment178919>
No need to print hierarchy or cgroup in error message here. It's alwasy callers job to print parameters. That's the protocol we use in the code base.
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 514)
<https://reviews.apache.org/r/42618/#comment178917>
s/netHandle/handle/
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 518 - 519)
<https://reviews.apache.org/r/42618/#comment178918>
Error message indentation.
- Jie Yu
On Feb. 3, 2016, 7:46 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 7:46 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
> src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
>
> Diff: https://reviews.apache.org/r/42618/diff/
>
>
> Testing
> -------
>
> make and make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 7:46 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 12:48 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
Rebased.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description
-------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Feb. 2, 2016, 6:32 p.m.)
Review request for mesos and Jie Yu.
Summary (updated)
-----------------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description (updated)
-------
Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan
Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to
use the `NetClsHandleMgr`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42618/
-----------------------------------------------------------
(Updated Jan. 31, 2016, 8:06 p.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4345
https://issues.apache.org/jira/browse/MESOS-4345
Repository: mesos
Description (updated)
-------
The `NetClsHandleMgr` will be responsible for keeping track of the net_cls
handles that will be used by the net_cls cgroup subsystem.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp b4bc52114389d1c1efce2830f4292bd89bb0de7c
src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp ddc1bf0939e5e8995e6f34fe7b8509b51704f63e
Diff: https://reviews.apache.org/r/42618/diff/
Testing
-------
make and make check
Thanks,
Avinash sridharan