Re: Review Request 53369: Agent cgroup assignment should precede agent initialization.

[Jiang Yan Xu]

---
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.

[Jiang Yan Xu]

> On Nov. 1, 2016, 10:08 p.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?
> 
> Jiang Yan Xu wrote:
> @Jie
> 
> If you are talking about the Mesos core, I think it already supports 
> being run by systemd? If you are talking about supporting 
> scripts/configs/packaging, then I agree the project can potentially do more 
> but that's a longer term goal towards better user experience of "stock" Mesos 
> packages. 
> 
> In the meantime, the flag has been here for a long time and we'd like to 
> fix this bug with it. Sounds good?
> 
> Jiang Yan Xu wrote:
> We picked this one up again. I agree that this doesn't feel like the 
> cleanest implementation but even if we deprecate it, IMO it's still a bug 
> that's worth fixing.
> 
> Jie Yu wrote:
> Having this fix sounds fine, but I'd like to see follow up actions to 
> deprecated the agent_subsystems flag and emails to dev list about that. Do 
> you guys have cycles to drive this?
> 
> Jiang Yan Xu wrote:
> Probably not at the moment but happy to JIRA it. I personally don't have 
> a strong feeling that we need to take it away immediately without 
> replacement. As I commented above, we don't have good support for such sample 
> configs and packaging (and it's a long standing issue) so this code at least 
> in a limited way can be useful to some people. (We could've used it if not 
> for this bug).
> 
> Thought?
> 
> Jie Yu wrote:
> Curious if you haven't used it yet, why not use the preferred way (config 
> systemd properly)?
> 
> Jiang Yan Xu wrote:
> We are fixing it just because it's a bug and it doesn't take much (at the 
> risk to complicating the codebase) to fix it and not because we couldn't use 
> alternative mechanisms. (We do)
> 
> I am just being cautious about taking it away from people (we know it's 
> actually used). But it doesn't affect us if someone goes ahead and do it. :)
> 
> Jie Yu wrote:
> OK, sounds good. Can you create a JIRA to track the deprecation of this 
> flag?

https://issues.apache.org/jira/browse/MESOS-7205


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


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.

[Jie Yu]

> On Nov. 2, 2016, 5:08 a.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?
> 
> Jiang Yan Xu wrote:
> @Jie
> 
> If you are talking about the Mesos core, I think it already supports 
> being run by systemd? If you are talking about supporting 
> scripts/configs/packaging, then I agree the project can potentially do more 
> but that's a longer term goal towards better user experience of "stock" Mesos 
> packages. 
> 
> In the meantime, the flag has been here for a long time and we'd like to 
> fix this bug with it. Sounds good?
> 
> Jiang Yan Xu wrote:
> We picked this one up again. I agree that this doesn't feel like the 
> cleanest implementation but even if we deprecate it, IMO it's still a bug 
> that's worth fixing.
> 
> Jie Yu wrote:
> Having this fix sounds fine, but I'd like to see follow up actions to 
> deprecated the agent_subsystems flag and emails to dev list about that. Do 
> you guys have cycles to drive this?
> 
> Jiang Yan Xu wrote:
> Probably not at the moment but happy to JIRA it. I personally don't have 
> a strong feeling that we need to take it away immediately without 
> replacement. As I commented above, we don't have good support for such sample 
> configs and packaging (and it's a long standing issue) so this code at least 
> in a limited way can be useful to some people. (We could've used it if not 
> for this bug).
> 
> Thought?
> 
> Jie Yu wrote:
> Curious if you haven't used it yet, why not use the preferred way (config 
> systemd properly)?
> 
> Jiang Yan Xu wrote:
> We are fixing it just because it's a bug and it doesn't take much (at the 
> risk to complicating the codebase) to fix it and not because we couldn't use 
> alternative mechanisms. (We do)
> 
> I am just being cautious about taking it away from people (we know it's 
> actually used). But it doesn't affect us if someone goes ahead and do it. :)

OK, sounds good. Can you create a JIRA to track the deprecation of this flag?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


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.

[Jiang Yan Xu]

> On Nov. 1, 2016, 10:08 p.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?
> 
> Jiang Yan Xu wrote:
> @Jie
> 
> If you are talking about the Mesos core, I think it already supports 
> being run by systemd? If you are talking about supporting 
> scripts/configs/packaging, then I agree the project can potentially do more 
> but that's a longer term goal towards better user experience of "stock" Mesos 
> packages. 
> 
> In the meantime, the flag has been here for a long time and we'd like to 
> fix this bug with it. Sounds good?
> 
> Jiang Yan Xu wrote:
> We picked this one up again. I agree that this doesn't feel like the 
> cleanest implementation but even if we deprecate it, IMO it's still a bug 
> that's worth fixing.
> 
> Jie Yu wrote:
> Having this fix sounds fine, but I'd like to see follow up actions to 
> deprecated the agent_subsystems flag and emails to dev list about that. Do 
> you guys have cycles to drive this?
> 
> Jiang Yan Xu wrote:
> Probably not at the moment but happy to JIRA it. I personally don't have 
> a strong feeling that we need to take it away immediately without 
> replacement. As I commented above, we don't have good support for such sample 
> configs and packaging (and it's a long standing issue) so this code at least 
> in a limited way can be useful to some people. (We could've used it if not 
> for this bug).
> 
> Thought?
> 
> Jie Yu wrote:
> Curious if you haven't used it yet, why not use the preferred way (config 
> systemd properly)?

We are fixing it just because it's a bug and it doesn't take much (at the risk 
to complicating the codebase) to fix it and not because we couldn't use 
alternative mechanisms. (We do)

I am just being cautious about taking it away from people (we know it's 
actually used). But it doesn't affect us if someone goes ahead and do it. :)


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


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.

[Jie Yu]

> On Nov. 2, 2016, 5:08 a.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?
> 
> Jiang Yan Xu wrote:
> @Jie
> 
> If you are talking about the Mesos core, I think it already supports 
> being run by systemd? If you are talking about supporting 
> scripts/configs/packaging, then I agree the project can potentially do more 
> but that's a longer term goal towards better user experience of "stock" Mesos 
> packages. 
> 
> In the meantime, the flag has been here for a long time and we'd like to 
> fix this bug with it. Sounds good?
> 
> Jiang Yan Xu wrote:
> We picked this one up again. I agree that this doesn't feel like the 
> cleanest implementation but even if we deprecate it, IMO it's still a bug 
> that's worth fixing.
> 
> Jie Yu wrote:
> Having this fix sounds fine, but I'd like to see follow up actions to 
> deprecated the agent_subsystems flag and emails to dev list about that. Do 
> you guys have cycles to drive this?
> 
> Jiang Yan Xu wrote:
> Probably not at the moment but happy to JIRA it. I personally don't have 
> a strong feeling that we need to take it away immediately without 
> replacement. As I commented above, we don't have good support for such sample 
> configs and packaging (and it's a long standing issue) so this code at least 
> in a limited way can be useful to some people. (We could've used it if not 
> for this bug).
> 
> Thought?

Curious if you haven't used it yet, why not use the preferred way (config 
systemd properly)?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


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.

[Jiang Yan Xu]

> On Nov. 1, 2016, 10:08 p.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?
> 
> Jiang Yan Xu wrote:
> @Jie
> 
> If you are talking about the Mesos core, I think it already supports 
> being run by systemd? If you are talking about supporting 
> scripts/configs/packaging, then I agree the project can potentially do more 
> but that's a longer term goal towards better user experience of "stock" Mesos 
> packages. 
> 
> In the meantime, the flag has been here for a long time and we'd like to 
> fix this bug with it. Sounds good?
> 
> Jiang Yan Xu wrote:
> We picked this one up again. I agree that this doesn't feel like the 
> cleanest implementation but even if we deprecate it, IMO it's still a bug 
> that's worth fixing.
> 
> Jie Yu wrote:
> Having this fix sounds fine, but I'd like to see follow up actions to 
> deprecated the agent_subsystems flag and emails to dev list about that. Do 
> you guys have cycles to drive this?

Probably not at the moment but happy to JIRA it. I personally don't have a 
strong feeling that we need to take it away immediately without replacement. As 
I commented above, we don't have good support for such sample configs and 
packaging (and it's a long standing issue) so this code at least in a limited 
way can be useful to some people. (We could've used it if not for this bug).

Thought?


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


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.

[Jie Yu]

> On Nov. 2, 2016, 5:08 a.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?
> 
> Jiang Yan Xu wrote:
> @Jie
> 
> If you are talking about the Mesos core, I think it already supports 
> being run by systemd? If you are talking about supporting 
> scripts/configs/packaging, then I agree the project can potentially do more 
> but that's a longer term goal towards better user experience of "stock" Mesos 
> packages. 
> 
> In the meantime, the flag has been here for a long time and we'd like to 
> fix this bug with it. Sounds good?
> 
> Jiang Yan Xu wrote:
> We picked this one up again. I agree that this doesn't feel like the 
> cleanest implementation but even if we deprecate it, IMO it's still a bug 
> that's worth fixing.

Having this fix sounds fine, but I'd like to see follow up actions to 
deprecated the agent_subsystems flag and emails to dev list about that. Do you 
guys have cycles to drive this?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


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.

[Mesos Reviewbot]

---
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.

[Anindya Sinha]

---
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.

[Jiang Yan Xu]

---
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)


Nit: make it `static`?

```
static Try assignCgroups(const ::Flags& flags)
```



src/slave/main.cpp
Lines 384-387 (patched)


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 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 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.

[Anindya Sinha]

> On March 2, 2017, 9:51 a.m., Jiang Yan Xu wrote:
> > src/slave/main.cpp
> > Lines 170 (patched)
> > 
> >
> > The following is cleaner:
> > 
> > ```
> > Try 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 assignCgroups(const ::Flags& flags)`


> On March 2, 2017, 9:51 a.m., Jiang Yan Xu wrote:
> > src/slave/main.cpp
> > Lines 382 (patched)
> > 
> >
> > ```
> > if (flags.agent_subsystems.isSome()) {
> >   Try assign = assignCgroups(flags.agent_subsystems.get());
> >   if (assign.isError()) {
> > EXIT(EXIT_FAILURE) << assign.error();
> >   }
> > }
> > ```

Modified based on the updated signature of `Try 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.

[Anindya Sinha]

---
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.

[Jiang Yan Xu]

> On Nov. 1, 2016, 10:08 p.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?
> 
> Jiang Yan Xu wrote:
> @Jie
> 
> If you are talking about the Mesos core, I think it already supports 
> being run by systemd? If you are talking about supporting 
> scripts/configs/packaging, then I agree the project can potentially do more 
> but that's a longer term goal towards better user experience of "stock" Mesos 
> packages. 
> 
> In the meantime, the flag has been here for a long time and we'd like to 
> fix this bug with it. Sounds good?

We picked this one up again. I agree that this doesn't feel like the cleanest 
implementation but even if we deprecate it, IMO it's still a bug that's worth 
fixing.


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


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.

[Jiang Yan Xu]

---
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)


The following is cleaner:

```
Try assignCgroups(const string& subsystems);
```



src/slave/main.cpp
Lines 174 (patched)


Not yours, but move this 1 space to the right.



src/slave/main.cpp
Lines 380-381 (patched)


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)


```
if (flags.agent_subsystems.isSome()) {
  Try 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.

[Mesos Reviewbot]

---
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.

[Anindya Sinha]

---
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.

[Mesos Reviewbot]

---
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
> 
>

Re: Review Request 53369: Agent cgroup assignment should precede agent initialization.

[Anindya Sinha]

---
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.

[Jiang Yan Xu]

> On Nov. 1, 2016, 10:08 p.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.
> 
> Anindya Sinha wrote:
> Agreed. But since this flag exists, I think we should address this 
> condition? Or are you suggesting to deprecate this flag?

@Jie

If you are talking about the Mesos core, I think it already supports being run 
by systemd? If you are talking about supporting scripts/configs/packaging, then 
I agree the project can potentially do more but that's a longer term goal 
towards better user experience of "stock" Mesos packages. 

In the meantime, the flag has been here for a long time and we'd like to fix 
this bug with it. Sounds good?


- Jiang Yan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


On Nov. 1, 2016, 9:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> ---
> 
> (Updated Nov. 1, 2016, 9:43 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 8010f8e229e2d820649750c9db0456ecd1b854d3 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> 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.

[Anindya Sinha]

> On Nov. 2, 2016, 5:08 a.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.

Agreed. But since this flag exists, I think we should address this condition? 
Or are you suggesting to deprecate this flag?


- Anindya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---


On Nov. 2, 2016, 4:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> ---
> 
> (Updated Nov. 2, 2016, 4:43 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 8010f8e229e2d820649750c9db0456ecd1b854d3 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> 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.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154537
---



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 Nov. 2, 2016, 4:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> ---
> 
> (Updated Nov. 2, 2016, 4:43 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 8010f8e229e2d820649750c9db0456ecd1b854d3 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> 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.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/#review154512
---



The agent subsystems is a hack to me. I think we should consider support 
running systemd (or other init system) to manage agent process and put it under 
proper cgroup using the init system, rather than doing it ourself.

- Jie Yu


On Nov. 2, 2016, 4:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> ---
> 
> (Updated Nov. 2, 2016, 4:43 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 8010f8e229e2d820649750c9db0456ecd1b854d3 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> Diff: https://reviews.apache.org/r/53369/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 53369: Agent cgroup assignment should precede agent initialization.

[Anindya Sinha]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53369/
---

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 8010f8e229e2d820649750c9db0456ecd1b854d3 
  src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 

Diff: https://reviews.apache.org/r/53369/diff/


Testing
---

All tests passed.


Thanks,

Anindya Sinha