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 Sun, 25 Sep 2016 22:32:02 GMT

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




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

    = None() is not necessary.



src/slave/containerizer/mesos/linux_launcher.cpp (lines 108 - 110)
<https://reviews.apache.org/r/51278/#comment218327>

    Do you still need this function?



src/slave/containerizer/mesos/linux_launcher.cpp (lines 257 - 259)
<https://reviews.apache.org/r/51278/#comment218328>

    checkpointed pid? Please update the comment here



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

    `return Failure("Failed to list cgroups under <cgroups_root>: " + cgroups.error());`



src/slave/containerizer/mesos/linux_launcher.cpp (lines 276 - 289)
<https://reviews.apache.org/r/51278/#comment218330>

    I suggest we factor out into a helper:
    ```
    ContainerID containerId = getContainerId(cgroup);
    ```



src/slave/containerizer/mesos/linux_launcher.cpp (lines 308 - 319)
<https://reviews.apache.org/r/51278/#comment218331>

    launcher destroy will be called for those orphans that launcher do not know about as well
(e.g., launcher destroy succeeds, but isolator cleanup is not done yet, agent crashes).
    
    Therefore, I'd suggest we adjust the destroy interface to return a Future<bool>.
'false' means the container is unknown to the launcher. Let the caller decide what to do.
In Mesos containerizer, if the launcher does not know about a container in the destroy path,
we probably should just continue the chain.
    
    In that way, you don't have to do the reconciliation here.



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

    count returns 'size_t'. So please use xxx.count() != 0



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

    space after `:`



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

    Ditto. Return a false here



src/slave/containerizer/mesos/linux_launcher.cpp (lines 520 - 526)
<https://reviews.apache.org/r/51278/#comment218335>

    I won't worry about this case because the containerizer will make sure cleanup/destroy
is only called once for launcher/isolator/provisioner.
    
    But I like the fact we remove the container from `containers` map so that other calls
to launcher will be properly handled (e.g., status).



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

    using executor_pid here is weird. I think we should rethink how we should `ContainerStatus`
when we have nested containers. Should we rename it to `pid`? Sounds like 'executor_pid' is
not very precise.
    
    Another to consider is: in the future, user might want to get the pid of it's actual process,
not the 'init' pid we forked. What should we call that?


- Jie Yu


On Sept. 25, 2016, 4:08 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51278/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored LinuxLauncher to be nested container aware.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.hpp 859990cb85e9e8c06400397256cfc512f0811800 
>   src/slave/containerizer/mesos/linux_launcher.hpp 2cd1e4d4b6d9e656c2447a9d3ab8e1f49ba5fffa

>   src/slave/containerizer/mesos/linux_launcher.cpp f27757032cc28be0f0067bed045536431dde4d6c

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


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