Re: Review Request 43306: Migrated linux launcher systemd executor logic into subprocess hook.

[Joris Van Remoortere]

> On Feb. 9, 2016, 10:22 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos/linux_launcher.cpp, line 310
> > 
> >
> > What about something like:
> > 
> > ```c++
> > // Capture the freezer cgroup for use in the subprocess hook below.
> > string freezerCgroup = cgroup(containerId);
> > 
> > // Set up subprocess hooks to be executed by the parent before exec'ing 
> > the child.
> > std::vector hooks = {
> >   [freezerHierarchy, freezerCgroup, containerId](pid_t child) {
> > // Move the child into the freezer cgroup. Any grandchildren will
> > // also be contained in the cgroup.
> > // TODO(jieyu): Move this logic to the subprocess (i.e.,
> > // mesos-containerizer launch).
> > Try assign = cgroups::assign(
> > freezerHierarchy,
> > freezerCgroup,
> > child);
> > 
> > if (assign.isError()) {
> >   LOG(ERROR) << "Failed to assign process " << child
> >   << " of container '" << containerId << "'"
> >   << " to its freezer cgroup: " << assign.error();
> >   ::kill(child, SIGKILL);
> >   return Error("Failed to contain process");
> > }
> > 
> > return Nothing();
> >   }
> > };
> > 
> > // If we are on systemd, then extend the life of the child. As with the
> > // freezer, any grandchildren will also be contained in the slice.
> > if (systemdHierarchy.isSome()) {
> >   hooks.emplace_back(Subprocess::Hook(::extendLifetime));
> > }
> > ```
> > 
> > And now we don't need to use any pipes at all here IIUC?

Will do this in a subsequent review.


- Joris


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


On Feb. 10, 2016, 4:51 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43306/
> ---
> 
> (Updated Feb. 10, 2016, 4:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Bugs: MESOS-4636
> https://issues.apache.org/jira/browse/MESOS-4636
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrated linux launcher systemd executor logic into subprocess hook.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
> 
> Diff: https://reviews.apache.org/r/43306/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 43306: Migrated linux launcher systemd executor logic into subprocess hook.

[Joris Van Remoortere]

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

(Updated Feb. 10, 2016, 4:51 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


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


Repository: mesos


Description
---

Migrated linux launcher systemd executor logic into subprocess hook.


Diffs (updated)
-

  src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
  src/slave/containerizer/mesos/linux_launcher.cpp 
c2e252ec6ed0d6d4c47e244f700315bd340cee5f 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 43306: Migrated linux launcher systemd executor logic into subprocess hook.

[Joris Van Remoortere]

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

(Updated Feb. 9, 2016, 7:14 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Repository: mesos


Description (updated)
---

Migrated linux launcher systemd executor logic into subprocess hook.


Diffs (updated)
-

  src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
  src/slave/containerizer/mesos/linux_launcher.cpp 
c2e252ec6ed0d6d4c47e244f700315bd340cee5f 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 43306: Migrated linux launcher systemd executor logic into subprocess hook.

[Benjamin Hindman]

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




src/linux/systemd.cpp (line 76)


Should you need to do this? Isn't the expectation if a hook fails then 
`Subprocess` will take care of cleaning up the child?


- Benjamin Hindman


On Feb. 9, 2016, 7:14 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43306/
> ---
> 
> (Updated Feb. 9, 2016, 7:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrated linux launcher systemd executor logic into subprocess hook.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
> 
> Diff: https://reviews.apache.org/r/43306/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 43306: Migrated linux launcher systemd executor logic into subprocess hook.

[Benjamin Hindman]

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


Fix it, then Ship it!





src/linux/systemd.cpp (line 66)


I'd say this is an assert which breaks the local expectation, i.e., 
non-local code outside of this systemd "library" might call this which fails 
the CHECK. Local reasoning IMHO here would just be scoped to within the 
systemd.cpp file, not all of Mesos. I don't see any reason why we wouldn't want 
to just return an `Error` here rather than abort the process?



src/slave/containerizer/mesos/linux_launcher.cpp (line 310)


What about something like:

```c++
// Capture the freezer cgroup for use in the subprocess hook below.
string freezerCgroup = cgroup(containerId);

// Set up subprocess hooks to be executed by the parent before exec'ing the 
child.
std::vector hooks = {
  [freezerHierarchy, freezerCgroup, containerId](pid_t child) {
// Move the child into the freezer cgroup. Any grandchildren will
// also be contained in the cgroup.
// TODO(jieyu): Move this logic to the subprocess (i.e.,
// mesos-containerizer launch).
Try assign = cgroups::assign(
freezerHierarchy,
freezerCgroup,
child);

if (assign.isError()) {
  LOG(ERROR) << "Failed to assign process " << child
  << " of container '" << containerId << "'"
  << " to its freezer cgroup: " << assign.error();
  ::kill(child, SIGKILL);
  return Error("Failed to contain process");
}

return Nothing();
  }
};

// If we are on systemd, then extend the life of the child. As with the
// freezer, any grandchildren will also be contained in the slice.
if (systemdHierarchy.isSome()) {
  hooks.emplace_back(Subprocess::Hook(::extendLifetime));
}
```

And now we don't need to use any pipes at all here IIUC?



src/slave/containerizer/mesos/linux_launcher.cpp (line 312)


Can we do `::extendLifetime` instead? MPark and I ran into a bug 
previous with one of the compilers where we didn't use `&` before the global 
function and I don't see any reason not to use it always.


- Benjamin Hindman


On Feb. 9, 2016, 7:14 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43306/
> ---
> 
> (Updated Feb. 9, 2016, 7:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrated linux launcher systemd executor logic into subprocess hook.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
> 
> Diff: https://reviews.apache.org/r/43306/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 43306: Migrated linux launcher systemd executor logic into subprocess hook.

[Joris Van Remoortere]

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

Review request for mesos, Benjamin Hindman and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/linux/systemd.cpp 5034308cb4d1bb0b66c097daf5ec53a880cf510a 
  src/slave/containerizer/mesos/linux_launcher.cpp 
c2e252ec6ed0d6d4c47e244f700315bd340cee5f 
  src/slave/main.cpp 22b833044dc3f900e3b5ea509e6e545148649f48 

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


Testing
---


Thanks,

Joris Van Remoortere