mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 51278: Refactored LinuxLauncher to be nested container aware.
Date Tue, 23 Aug 2016 20:34:11 GMT

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




src/slave/containerizer/mesos/linux_launcher.hpp (lines 20 - 22)
<https://reviews.apache.org/r/51278/#comment213036>

    Swap these two.



src/slave/containerizer/mesos/linux_launcher.hpp (lines 64 - 85)
<https://reviews.apache.org/r/51278/#comment213038>

    Swap these two definitions. According to google style, type definitions should be listed
first.



src/slave/containerizer/mesos/linux_launcher.hpp (lines 97 - 99)
<https://reviews.apache.org/r/51278/#comment213039>

    Move this definition up below `recover` helper.



src/slave/containerizer/mesos/linux_launcher.cpp (lines 131 - 145)
<https://reviews.apache.org/r/51278/#comment213044>

    I suggest we move this to a helper. We already have a helper to get cgroup from containerId,
this will be the reverse operation.
    
    ```
    string getCgroup(const ContainerID& containerId);
    Try<ContainerID> getContainerId(const string& cgroup);
    ```
    
    We can add necessary validation in the helper.



src/slave/containerizer/mesos/linux_launcher.cpp (lines 149 - 151)
<https://reviews.apache.org/r/51278/#comment213040>

    As we discussed yesterday, we probably should checkpoint the pid in runtime_dir before
creating the cgroup in the freezer hierarchy.



src/slave/containerizer/mesos/linux_launcher.cpp (lines 186 - 193)
<https://reviews.apache.org/r/51278/#comment213052>

    The accumulation logic here is quite unintuitive. I'd suggest we make `cgroups::traverse`
simple in the sense that it only applies a _map_ function.
    
    ```
    template <typename T>
    Try<vector<T>> cgroups::traverse(
      const string& hierarchy,
      const string& cgroup,
      const lambda::function<T(const string&)>& f,
      const Traversal& traversal = PRE_ORDER);
    ```
    
    And we perform the accumulation in LinuxLauncher. In that way, `recover` helper can be
simplified to just return a `Try<Container>`.



src/slave/containerizer/mesos/linux_launcher.cpp (lines 201 - 206)
<https://reviews.apache.org/r/51278/#comment213054>

    I am wondering if should traverse the runtime_dir. Since we checkpoint the pid before
we create the cgroup (i.e., we remove cgroup before we remove the checkpointed pid), we might
run into the case where  the checkpointed pid exists but the freezer cgroup does not exist.
    
    However, for legacy containers, we still have to traverse freezer hierarchy.



src/slave/containerizer/mesos/linux_launcher.cpp (line 209)
<https://reviews.apache.org/r/51278/#comment213071>

    s/our process/agent



src/slave/containerizer/mesos/linux_launcher.cpp (line 223)
<https://reviews.apache.org/r/51278/#comment213074>

    Why do you need this if check?



src/slave/containerizer/mesos/linux_launcher.cpp (line 229)
<https://reviews.apache.org/r/51278/#comment213073>

    s/our process/agent



src/slave/containerizer/mesos/linux_launcher.cpp (lines 402 - 409)
<https://reviews.apache.org/r/51278/#comment213075>

    When do we remove `Container` from `containers`? We cannot keep them forever as it'll
cause a memory leak. I think we should remove it when the top level container is being destroyed,
and we should do it _after_ the `cgroups::destory` is successful.



src/slave/containerizer/mesos/linux_launcher.cpp (lines 408 - 409)
<https://reviews.apache.org/r/51278/#comment213057>

    Can you be more specific here? Who will be responsible for cleaning up sub-directories
under runtime_dir when the top level container terminates? I feel that Launcher should be
responsible for doing that because it was the one that checkpoints the data.
    
    The caveat here is that we cannot remove checkpoint dir for nested containers until the
top level container terminates. This is because the executor might issue `WAIT` call to the
agent to get the exit status of the nested container.
    
    For the top level container, if we move the entire checkpointed dir during destroy, and
if slave crashes before it checkpoints the sentinel file. During recovery, the containerizer
will still call `launcher->wait` on the containerId that launcher does not know about.
But since the agent does not need the exit code for the executor, this is fine for now. In
the future, if we want to add executor status update, we need to adjust the logics here so
that we only remove the top level checkpoint dir for the top level container after agent checkpoint
the sentinel file. We probably need an explicit `launcher->cleanup`.



src/slave/containerizer/mesos/linux_launcher.cpp (lines 414 - 419)
<https://reviews.apache.org/r/51278/#comment213055>

    This sounds like an uncessary optimization that buy us nothing. This also creates an explicit
dependency from launcher to pid namespace isolator. I'd suggest we simply just remove this
code here (i.e., always use freezer).


- Jie Yu


On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51278/
> -----------------------------------------------------------
> 
> (Updated Aug. 22, 2016, 4:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Note, this doesn't yet include the use of 'ns::enter' for creating
> nested containers.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/linux_launcher.hpp 8fbe1e9742df85b0fc6de46ac81a0c064c845a63

>   src/slave/containerizer/mesos/linux_launcher.cpp 7377316776646e3d660086da3c0d5b494ce8ace4

> 
> Diff: https://reviews.apache.org/r/51278/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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