-----------------------------------------------------------
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)
<https://reviews.apache.org/r/43306/#comment179714>

    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)
<https://reviews.apache.org/r/43306/#comment179707>

    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<Subprocess::Hook> 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<Nothing> 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(&systemd::extendLifetime));
    }
    ```
    
    And now we don't need to use any pipes at all here IIUC?



src/slave/containerizer/mesos/linux_launcher.cpp (line 312)
<https://reviews.apache.org/r/43306/#comment179706>

    Can we do `&systemd::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
> 
>

Reply via email to