mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Hindman <b...@berkeley.edu>
Subject Re: Review Request 43306: Migrated linux launcher systemd executor logic into subprocess hook.
Date Tue, 09 Feb 2016 22:22:43 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message