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/02/02 19:32:25 UTC
Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to
use the `NetClsHandleMgr`.
-----------------------------------------------------------
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 `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