Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Timothy Chen]

> On Sept. 1, 2015, 2:52 p.m., Mesos ReviewBot wrote:
> > Patch looks great!
> > 
> > Reviews applied: [36612, 36620]
> > 
> > All tests passed.
> 
> Timothy Chen wrote:
> Jie does this look good to you?
> 
> Jie Yu wrote:
> Should we close this given https://reviews.apache.org/r/37967/?

Looks like we can do that. I'll close this.


- Timothy


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


On Sept. 1, 2015, 2:12 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Sept. 1, 2015, 2:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

> On Sept. 1, 2015, 2:52 p.m., Mesos ReviewBot wrote:
> > Patch looks great!
> > 
> > Reviews applied: [36612, 36620]
> > 
> > All tests passed.
> 
> Timothy Chen wrote:
> Jie does this look good to you?

Should we close this given https://reviews.apache.org/r/37967/?


- Jie


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


On Sept. 1, 2015, 2:12 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Sept. 1, 2015, 2:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Timothy Chen]

> On Sept. 1, 2015, 2:52 p.m., Mesos ReviewBot wrote:
> > Patch looks great!
> > 
> > Reviews applied: [36612, 36620]
> > 
> > All tests passed.

Jie does this look good to you?


- Timothy


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


On Sept. 1, 2015, 2:12 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Sept. 1, 2015, 2:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36612, 36620]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 2:12 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Sept. 1, 2015, 2:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Sept. 1, 2015, 2:12 p.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086


Repository: mesos


Description
---

Added Non-Freezeer Task Killer.


Diffs (updated)
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Sept. 1, 2015, 12:14 p.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086


Repository: mesos


Description
---

Added Non-Freezeer Task Killer.


Diffs (updated)
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Sept. 1, 2015, 9:39 a.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086


Repository: mesos


Description
---

Added Non-Freezeer Task Killer.


Diffs (updated)
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
> Hi jie, thanks for chiming in. This should be ready after the comments 
> are addressed. I'll try to merge this as well when it's done. I'll ping Joerg 
> about this.
> 
> Joerg Schad wrote:
> Hi Jie,
> I am not sure whether I should move the entire logic further into 
> cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how 
> to destroy the cgroups, which in my understanding makes sense to have in 
> seperate TasksKiller entities. Secondly cgroups::kill currently is little 
> more than trying to kill the cgroup, when moving the logic in here 
> cgroups::kill would have to call cgroups::freeze or call the iterative logic. 
> Secondly, in my understanding the interface here is the destroy() call. I 
> actually would like to refactor this piece of code in general, but this is 
> outside the scope of this patch. If we want to discuss this further -or 
> misunderstand something here- I would be happy to discuss it further!
> 
> Jie Yu wrote:
> Hi Joerg,
> 
> I think you misunderstood what I propose here. I am not asking you to 
> merge the logics of TasksKiller and FreezerTasksKiller. I think it's great 
> that you separate these two Processes. What I am sugguesting here is that you 
> pull the code from 1777-1790 to a separate function (like the following). We 
> may want to reuse that function in Mesos containerizer launcher in the future 
> (i.e., not rely on freezer). It also makes the logics in Destroyer cleaner. 
> Let me know your thoughts! Thank you for taking on this!
> 
> ```
> virtual void initialize()
> {
>   // Stop when no one cares.
>   promise.future().onDiscard(lambda::bind(
>   static_cast(terminate), self(), true));
> 
>   // Kill tasks in the given cgroups in parallel. Use collect mechanism to
>   // wait until all kill processes finish.
>   foreach (const string& cgroup, cgroups) {
> killers.push_back(cgroups:kill(hierarchy, cgroup));
>   }
> 
>   collect(killers)
> .onAny(defer(self(), &Destroyer::killed, lambda::_1));
> }
> 
> 
> // Kill all processes in the given cgroup.
> Future cgroups::kill(
> const string& hierarchy,
> const string& cgroup)
> {
>   Future future;
>   
>   // Use the freezer subsystem if available.
>   Option freezerCheckError =
> verify(hierarchy, cgroup, "freezer.state");
>   
>   if (freezerCheckError.isNone()) {
> internal::FreezerTasksKiller* killer =
>   new internal::FreezerTasksKiller(hierarchy, cgroup);
>  
> future = killer->future();
> 
> spawn(killer, true);
>   } else {
> internal::TasksKiller* killer =
>   new internal::TasksKiller(hierarchy, cgroup);
>   
> future = killer->future();
> 
> spawn(killer, true);
>   }
>   
>   return future;
> }
> ```
> 
> Joerg Schad wrote:
> As the renaming seems to be controversial 
> (https://reviews.apache.org/r/37894/), I would right now introduce 
> cgroups::killProcesses. Does that work for you?

yeah, that sounds good to me.


- Jie


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


On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 28, 2015, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
> Hi jie, thanks for chiming in. This should be ready after the comments 
> are addressed. I'll try to merge this as well when it's done. I'll ping Joerg 
> about this.
> 
> Joerg Schad wrote:
> Hi Jie,
> I am not sure whether I should move the entire logic further into 
> cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how 
> to destroy the cgroups, which in my understanding makes sense to have in 
> seperate TasksKiller entities. Secondly cgroups::kill currently is little 
> more than trying to kill the cgroup, when moving the logic in here 
> cgroups::kill would have to call cgroups::freeze or call the iterative logic. 
> Secondly, in my understanding the interface here is the destroy() call. I 
> actually would like to refactor this piece of code in general, but this is 
> outside the scope of this patch. If we want to discuss this further -or 
> misunderstand something here- I would be happy to discuss it further!
> 
> Jie Yu wrote:
> Hi Joerg,
> 
> I think you misunderstood what I propose here. I am not asking you to 
> merge the logics of TasksKiller and FreezerTasksKiller. I think it's great 
> that you separate these two Processes. What I am sugguesting here is that you 
> pull the code from 1777-1790 to a separate function (like the following). We 
> may want to reuse that function in Mesos containerizer launcher in the future 
> (i.e., not rely on freezer). It also makes the logics in Destroyer cleaner. 
> Let me know your thoughts! Thank you for taking on this!
> 
> ```
> virtual void initialize()
> {
>   // Stop when no one cares.
>   promise.future().onDiscard(lambda::bind(
>   static_cast(terminate), self(), true));
> 
>   // Kill tasks in the given cgroups in parallel. Use collect mechanism to
>   // wait until all kill processes finish.
>   foreach (const string& cgroup, cgroups) {
> killers.push_back(cgroups:kill(hierarchy, cgroup));
>   }
> 
>   collect(killers)
> .onAny(defer(self(), &Destroyer::killed, lambda::_1));
> }
> 
> 
> // Kill all processes in the given cgroup.
> Future cgroups::kill(
> const string& hierarchy,
> const string& cgroup)
> {
>   Future future;
>   
>   // Use the freezer subsystem if available.
>   Option freezerCheckError =
> verify(hierarchy, cgroup, "freezer.state");
>   
>   if (freezerCheckError.isNone()) {
> internal::FreezerTasksKiller* killer =
>   new internal::FreezerTasksKiller(hierarchy, cgroup);
>  
> future = killer->future();
> 
> spawn(killer, true);
>   } else {
> internal::TasksKiller* killer =
>   new internal::TasksKiller(hierarchy, cgroup);
>   
> future = killer->future();
> 
> spawn(killer, true);
>   }
>   
>   return future;
> }
> ```

As the renaming seems to be controversial 
(https://reviews.apache.org/r/37894/), I would right now introduce 
cgroups::killProcesses. Does that work for you?


- Joerg


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


On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 28, 2015, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

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



src/linux/cgroups.cpp (line 1676)


Hum, what if 'cgroups::processes' fails the first time (i.e., 'chain' 
hasn't been set up yet)?

I would suggest terminating the process if error is encountered:

```
Try> processes = ...;
if (processes.isError()) {
  promise.fail("Failed to ...: " + processes.error());
  terminate(self());
  return;
}
```



src/linux/cgroups.cpp (line 1681)


First of all, no need to pass in 'chain' because it's a member field of 
this class.

Second, I don't think we should call 'finished' here because you can simply 
do the following and save a call to 'cgroups::procsses'

```
if (processes.get().empty()) {
  promise.set(Nothing());
  terminate(self());
  return;
}
```



src/linux/cgroups.cpp (lines 1685 - 1686)


It's not really a chain here. In fact, we don't need a 'chain' variable 
here. The 'kill' (or 'signal') is a synchronour operation. ALso, no need to 
pull 'collect(statuses)' into a separate function. Here is my suggestion:

```
statuses.clear();

// Send SIGKILL to all processes.
foreach (pid_t pid, processes.get()) {
  ::kill(...);
  
  statuses.push_back(process::reap(pid));
}

collect(statuses)
  .onAny(defer(self(), &Self::_killTasksIteration));
  
...

void _killTasksIteration(const Future<...>& future)
{
  if (!future.isReady()) {
promise.fail(...);
terminate(self());
return;
  }
  
  killTasksIteration();
}

virtual void finalize()
{
  discard(statuses);
  
  promise.discard();
}
```


- Jie Yu


On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 28, 2015, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Aug. 28, 2015, 3:48 p.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086


Repository: mesos


Description (updated)
---

Added Non-Freezeer Task Killer.


Diffs
-

  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
> Hi jie, thanks for chiming in. This should be ready after the comments 
> are addressed. I'll try to merge this as well when it's done. I'll ping Joerg 
> about this.
> 
> Joerg Schad wrote:
> Hi Jie,
> I am not sure whether I should move the entire logic further into 
> cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how 
> to destroy the cgroups, which in my understanding makes sense to have in 
> seperate TasksKiller entities. Secondly cgroups::kill currently is little 
> more than trying to kill the cgroup, when moving the logic in here 
> cgroups::kill would have to call cgroups::freeze or call the iterative logic. 
> Secondly, in my understanding the interface here is the destroy() call. I 
> actually would like to refactor this piece of code in general, but this is 
> outside the scope of this patch. If we want to discuss this further -or 
> misunderstand something here- I would be happy to discuss it further!

Hi Joerg,

I think you misunderstood what I propose here. I am not asking you to merge the 
logics of TasksKiller and FreezerTasksKiller. I think it's great that you 
separate these two Processes. What I am sugguesting here is that you pull the 
code from 1777-1790 to a separate function (like the following). We may want to 
reuse that function in Mesos containerizer launcher in the future (i.e., not 
rely on freezer). It also makes the logics in Destroyer cleaner. Let me know 
your thoughts! Thank you for taking on this!

```
virtual void initialize()
{
  // Stop when no one cares.
  promise.future().onDiscard(lambda::bind(
  static_cast(terminate), self(), true));

  // Kill tasks in the given cgroups in parallel. Use collect mechanism to
  // wait until all kill processes finish.
  foreach (const string& cgroup, cgroups) {
killers.push_back(cgroups:kill(hierarchy, cgroup));
  }

  collect(killers)
.onAny(defer(self(), &Destroyer::killed, lambda::_1));
}


// Kill all processes in the given cgroup.
Future cgroups::kill(
const string& hierarchy,
const string& cgroup)
{
  Future future;
  
  // Use the freezer subsystem if available.
  Option freezerCheckError =
verify(hierarchy, cgroup, "freezer.state");
  
  if (freezerCheckError.isNone()) {
internal::FreezerTasksKiller* killer =
  new internal::FreezerTasksKiller(hierarchy, cgroup);
 
future = killer->future();

spawn(killer, true);
  } else {
internal::TasksKiller* killer =
  new internal::TasksKiller(hierarchy, cgroup);
  
future = killer->future();

spawn(killer, true);
  }
  
  return future;
}
```


- Jie


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


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

> On Aug. 24, 2015, 3:05 p.m., Alexander Rojas wrote:
> > src/linux/cgroups.cpp, line 1749
> > 
> >
> > Do they need to be actual attributes? For example `kill()` could return 
> > a `Future>>>`, and `reap()` could take a 
> > `list>>` as parameter, then line 1686 will look:
> > 
> > ```
> > chain = kill(processes).then(defer(self(), &Self::reap, lambda::_1));
> > ```
> > 
> > and you don't need statuses to be an attribute nor do you need line 
> > 1672.

see below and discussion offline.


- Joerg


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


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
> Hi jie, thanks for chiming in. This should be ready after the comments 
> are addressed. I'll try to merge this as well when it's done. I'll ping Joerg 
> about this.

Hi Jie,
I am not sure whether I should move the entire logic further into 
cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how to 
destroy the cgroups, which in my understanding makes sense to have in seperate 
TasksKiller entities. Secondly cgroups::kill currently is little more than 
trying to kill the cgroup, when moving the logic in here cgroups::kill would 
have to call cgroups::freeze or call the iterative logic. Secondly, in my 
understanding the interface here is the destroy() call. I actually would like 
to refactor this piece of code in general, but this is outside the scope of 
this patch. If we want to discuss this further -or misunderstand something 
here- I would be happy to discuss it further!


- Joerg


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


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Timothy Chen]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.

Hi jie, thanks for chiming in. This should be ready after the comments are 
addressed. I'll try to merge this as well when it's done. I'll ping Joerg about 
this.


- Timothy


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


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

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


What's the status on this review? Is it still WIP?

Would love to get this patch in given the perf ROOT tests are always broken on 
newer kernels.


src/linux/cgroups.cpp (lines 1776 - 1791)


Could you please introduce a new function under cgroups namespace and put 
this logic there:

```
// Kill all processes in the given cgroup.
Future cgroups::kill(
const string& hierarchy,
const string& cgroup)
{
  ...
  if (freezerCheckError.isNone()) {
  } else {
  }
  
  return ...;
}
```

You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
signal)` to `cgroups::signal`.


- Jie Yu


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

> On Aug. 24, 2015, 3:05 p.m., Alexander Rojas wrote:
> > src/linux/cgroups.cpp, line 1750
> > 
> >
> > Not sure if `chain` needs to be an attribute. The only reason I see is 
> > the discard call in `finalize()`. But if it needs to be, I feel the name of 
> > the attribute and the documentation are a little bit unfortunate since 
> > chain is not a list of reaped processes (their needed of type process, nor 
> > pid_t) but it is actually the exit statuses of those processes. Note that 
> > you cannot even assign which status came from which process.

As discussed we need to be able to access the chain in the finalize call (if 
interrupted from by external timeout).


- Joerg


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


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Till Toenshoff]

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


Given the rather complicated setup of this ( destroy -> Destroyer -...-> 
TasksKiller/s ), I think we really need tests here and also specifically tests 
that cover your addition (freezer-less cgroup destruction).


src/linux/cgroups.cpp (lines 1626 - 1632)


Why would the freezer component not be available -- can you please mention 
that e.g. a root owned cgroup wont have freezer.state available? Not sure if 
there are other expected reasons for a missing freezer though.



src/linux/cgroups.cpp (lines 1696 - 1697)


Kill one line plz.



src/linux/cgroups.cpp (lines 1735 - 1737)


Wouldn't this be a bit better in terms of preventing jaggedness?

```
  promise.fail(
"Failed to kill all processes in cgroup: " +
(processes.isError() ? processes.error() : "processes remain"));

```



src/linux/cgroups.cpp (line 1778)


s/freezerCheckError/verify/



src/linux/cgroups.cpp (line 1854)


s/move/Move/


- Till Toenshoff


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Alexander Rojas]

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



src/linux/cgroups.cpp (lines 1486 - 1487)


How about something along the lines of:

> The process uses the cgroups freezer component, which works
> similar to send a STOP signal to the task but without notifying
> the task about the signal, to stop execution of tasks before
> atomically kill the tasks in the group.
> See https://www.kernel.org/doc/Documentation/cgroups/freezer-subsystem.txt



src/linux/cgroups.cpp (line 1488)


Not sure about the name of this class, doesn't offer any relevant 
information: Does it kill freezer tasks, i.e. tasks of type freezer? Doest it 
kill frozen tasks, i.e. unresponsive tasks? 

After reading the documentation from the cgroups freezer on realizes that 
it uses a freezing mechanism to stop the tasks before killinh them, but this is 
not clear neither from the documentation nor from the name of the class.

How about: `CgroupsFreezerTaskKiller`



src/linux/cgroups.cpp (line 1642)


weird mix of active and passive voice here. How about:

> … Failure occurs if any process in the cgroup cannot be killed.



src/linux/cgroups.cpp (line 1691)


This is what I meant when I wrote the comment which appears next, it just 
makes little sense to me to discard a future which already failed, since 
discard purpose is to signal we are not interested in the result, but if 
`onFailed` is called, we have the future in a final state already.



src/linux/cgroups.cpp (line 1698)


Since there's no control of `processes` being an error (which shouldn't), 
it makes more sense for the signature of the method to be `Future 
kill(set&)`. And let the called to `kill(processes.get())`. 

It just prevent's someone in the future from making an error and passing a 
failed `Try` to `kill()`.



src/linux/cgroups.cpp (lines 1700 - 1701)


How about:

> Collect futures to the exit statuses of the processes before sending
> the kill signal ensures we actually caught all exit statuses.



src/linux/cgroups.cpp (line 1708)


This may be a pattern we use, but I think setting a `Future` to `Failure` 
to send status about the content of the `Future` is wrong. Another common 
pattern is to return a `Try` or `Option`, so the failed future 
indicates an error with the future mechanics (e.g. the promise when out of 
scope before setting its value) and the `Try` will pass information about the 
operation that it performs.



src/linux/cgroups.cpp (line 1749)


Do they need to be actual attributes? For example `kill()` could return a 
`Future>>>`, and `reap()` could take a 
`list>>` as parameter, then line 1686 will look:

```
chain = kill(processes).then(defer(self(), &Self::reap, lambda::_1));
```

and you don't need statuses to be an attribute nor do you need line 1672.



src/linux/cgroups.cpp (line 1750)


Not sure if `chain` needs to be an attribute. The only reason I see is the 
discard call in `finalize()`. But if it needs to be, I feel the name of the 
attribute and the documentation are a little bit unfortunate since chain is not 
a list of reaped processes (their needed of type process, nor pid_t) but it is 
actually the exit statuses of those processes. Note that you cannot even assign 
which status came from which process.



src/linux/cgroups.cpp (line 1778)


Again the name, without looking anywhere else, if feels as if we where 
checking for frozen tasks and that were an error.


- Alexander Rojas


On Aug. 24, 2015, 11:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 11:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Aug. 20, 2015, 11:42 a.m.)


Review request for mesos, Benjamin Hindman and Timothy Chen.


Summary (updated)
-

Added Non-Freezeer Task Killer.


Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086


Repository: mesos


Description
---

WIP Added Non-Freezeer Task Killer.


Diffs
-

  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad