You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anindya Sinha <an...@apple.com> on 2017/02/27 17:47:34 UTC
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/
-----------------------------------------------------------
(Updated Feb. 27, 2017, 5:47 p.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Rebase.
Bugs: MESOS-6523
https://issues.apache.org/jira/browse/MESOS-6523
Repository: mesos
Description
-------
This is to ensure that we do not accept traffic on the agent by opening
up libprocess port only after cgroup assigment for the agent is done.
Diffs (updated)
-----
src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a
Diff: https://reviews.apache.org/r/53369/diff/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review167767
-----------------------------------------------------------
Fix it, then Ship it!
The suggested snippet should fix the reviewbot error.
src/slave/main.cpp
Lines 170 (patched)
<https://reviews.apache.org/r/53369/#comment239732>
Nit: make it `static`?
```
static Try<Nothing> assignCgroups(const ::Flags& flags)
```
src/slave/main.cpp
Lines 384-387 (patched)
<https://reviews.apache.org/r/53369/#comment239731>
I feel that I can understand the flow better without looking at the implementation of `assignCgroups` if it's enclosed by `if (flags.agent_subsystems.isSome())`.
```
if (flags.agent_subsystems.isSome()) {
Try<Nothing> assign = assignCgroups(flags);
if (assign.isError()) {
EXIT(EXIT_FAILURE) << assign.error()();
}
}
```
This along with "if necessary" in the comments makes it more clear this is a conditional thing.
```
// Move the agent process into its own cgroup for each of the specified
// subsystems if necessary before the process is initialized.
```
Within the method we just need to do a CHECK_SOME.
```
static Try<Nothing> assignCgroups(const ::Flags& flags)
{
CHECK_SOME(flags.agent_subsystems);
...
}
```
- Jiang Yan Xu
On March 2, 2017, 12:36 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 12:36 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
>
>
> Diffs
> -----
>
> src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
> src/slave/slave.cpp 6ae9458cc81a7623a1837cd636156434a972004b
>
>
> Diff: https://reviews.apache.org/r/53369/diff/4/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review167806
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53369]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 3, 2017, 6:07 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> -----------------------------------------------------------
>
> (Updated March 3, 2017, 6:07 a.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
>
>
> Diffs
> -----
>
> src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
> src/slave/slave.cpp 6ae9458cc81a7623a1837cd636156434a972004b
>
>
> Diff: https://reviews.apache.org/r/53369/diff/5/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review169049
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
On March 2, 2017, 10:07 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 10:07 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
>
>
> Diffs
> -----
>
> src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
> src/slave/slave.cpp 6ae9458cc81a7623a1837cd636156434a972004b
>
>
> Diff: https://reviews.apache.org/r/53369/diff/5/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/
-----------------------------------------------------------
(Updated March 3, 2017, 6:07 a.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Fixed build.
Bugs: MESOS-6523
https://issues.apache.org/jira/browse/MESOS-6523
Repository: mesos
Description
-------
This is to ensure that we do not accept traffic on the agent by opening
up libprocess port only after cgroup assigment for the agent is done.
Diffs (updated)
-----
src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
src/slave/slave.cpp 6ae9458cc81a7623a1837cd636156434a972004b
Diff: https://reviews.apache.org/r/53369/diff/5/
Changes: https://reviews.apache.org/r/53369/diff/4-5/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/
-----------------------------------------------------------
(Updated March 2, 2017, 8:36 p.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Addressed comments.
Bugs: MESOS-6523
https://issues.apache.org/jira/browse/MESOS-6523
Repository: mesos
Description
-------
This is to ensure that we do not accept traffic on the agent by opening
up libprocess port only after cgroup assigment for the agent is done.
Diffs (updated)
-----
src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
src/slave/slave.cpp 6ae9458cc81a7623a1837cd636156434a972004b
Diff: https://reviews.apache.org/r/53369/diff/4/
Changes: https://reviews.apache.org/r/53369/diff/3-4/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review167640
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53369]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 2, 2017, 1:57 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 1:57 a.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
>
>
> Diffs
> -----
>
> src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
> src/slave/slave.cpp 892ce1938ac695e7913aa9139536d0787f3f5ea7
>
>
> Diff: https://reviews.apache.org/r/53369/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Anindya Sinha <an...@apple.com>.
> On March 2, 2017, 9:51 a.m., Jiang Yan Xu wrote:
> > src/slave/main.cpp
> > Lines 170 (patched)
> > <https://reviews.apache.org/r/53369/diff/3/?file=1652785#file1652785line170>
> >
> > The following is cleaner:
> >
> > ```
> > Try<Nothing> assignCgroups(const string& subsystems);
> > ```
I would need the other flags as well (viz. `cgroups_hierarchy` and `cgroups_root`), so I modified it as follows:
`Try<Nothing> assignCgroups(const ::Flags& flags)`
> On March 2, 2017, 9:51 a.m., Jiang Yan Xu wrote:
> > src/slave/main.cpp
> > Lines 382 (patched)
> > <https://reviews.apache.org/r/53369/diff/3/?file=1652785#file1652785line382>
> >
> > ```
> > if (flags.agent_subsystems.isSome()) {
> > Try<Nothing> assign = assignCgroups(flags.agent_subsystems.get());
> > if (assign.isError()) {
> > EXIT(EXIT_FAILURE) << assign.error();
> > }
> > }
> > ```
Modified based on the updated signature of `Try<Nothing> assignCgroups()`.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review167662
-----------------------------------------------------------
On March 2, 2017, 8:36 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> -----------------------------------------------------------
>
> (Updated March 2, 2017, 8:36 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
>
>
> Diffs
> -----
>
> src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
> src/slave/slave.cpp 6ae9458cc81a7623a1837cd636156434a972004b
>
>
> Diff: https://reviews.apache.org/r/53369/diff/4/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review167662
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/main.cpp
Lines 170 (patched)
<https://reviews.apache.org/r/53369/#comment239574>
The following is cleaner:
```
Try<Nothing> assignCgroups(const string& subsystems);
```
src/slave/main.cpp
Lines 174 (patched)
<https://reviews.apache.org/r/53369/#comment239573>
Not yours, but move this 1 space to the right.
src/slave/main.cpp
Lines 380-381 (patched)
<https://reviews.apache.org/r/53369/#comment239576>
Here we can explain the timing:
```
// Move the agent process into its own cgroup for each of the specified
// subsystems before the process is initialized.
```
src/slave/main.cpp
Lines 382 (patched)
<https://reviews.apache.org/r/53369/#comment239575>
```
if (flags.agent_subsystems.isSome()) {
Try<Nothing> assign = assignCgroups(flags.agent_subsystems.get());
if (assign.isError()) {
EXIT(EXIT_FAILURE) << assign.error();
}
}
```
- Jiang Yan Xu
On March 1, 2017, 9:57 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> -----------------------------------------------------------
>
> (Updated March 1, 2017, 9:57 a.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
>
>
> Diffs
> -----
>
> src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
> src/slave/slave.cpp 892ce1938ac695e7913aa9139536d0787f3f5ea7
>
>
> Diff: https://reviews.apache.org/r/53369/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/
-----------------------------------------------------------
(Updated March 1, 2017, 5:57 p.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Moved the handling of the cgroups subsystems to a separate function.
Bugs: MESOS-6523
https://issues.apache.org/jira/browse/MESOS-6523
Repository: mesos
Description
-------
This is to ensure that we do not accept traffic on the agent by opening
up libprocess port only after cgroup assigment for the agent is done.
Diffs (updated)
-----
src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
src/slave/slave.cpp 892ce1938ac695e7913aa9139536d0787f3f5ea7
Diff: https://reviews.apache.org/r/53369/diff/3/
Changes: https://reviews.apache.org/r/53369/diff/2-3/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 53369: Agent cgroup assignment should precede agent
initialization.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review166908
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53369]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 27, 2017, 5:47 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2017, 5:47 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
>
>
> Diffs
> -----
>
> src/slave/main.cpp a124d2e0cfa3e39e2400183f9523486196b9816d
> src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a
>
> Diff: https://reviews.apache.org/r/53369/diff/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>