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 41820: Pulled out provisioner from linux filesystem isolator.
Date Fri, 08 Jan 2016 01:05:06 GMT

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



src/slave/containerizer/mesos/containerizer.cpp (lines 482 - 483)
<https://reviews.apache.org/r/41820/#comment173953>

    We should recover the isolators first before recover provisioner (the reverse order of
launching a container because recover might do cleanup on unknown containers).
    
    YOu might want to add `recoverIsolators` and `recoverProvisioner`. Change the `_recover`
function to be:
    ```
    return recoverIsolators(recoverable, orphans)
      .then(defer(self(), &Self::recoverProvisioner, recoverable, orphans))
      .then(defer(self(), &Self::__recover, recoverable, orphans));
    ```



src/slave/containerizer/mesos/containerizer.cpp (lines 689 - 690)
<https://reviews.apache.org/r/41820/#comment173983>

    We mutate the executorInfo here and that mutated executorInfo here needs to be passed
to `prepare` and `_launch`. This is important for command executors.
    
    I guess what you needs to do here is: rename the existing `_launch` to `__launch` and
add a new `_launch`. In `launch`, do the following:
    
    ```
    return provisioner->provision(containerId, executorInfo)
      .then(defer(self(),
                  &Self::_launch,
                  containerId,
                  executorInfo,
                  directory,
                  user,
                  slaveId,
                  slavePid,
                  checkpoint,
                  lambda::_1));
    ```
    
    The new `_launch` should be similar to what you have here for `_provision`, but in the
end, if will call `__launch` with mutated executorInfo.



src/slave/containerizer/mesos/containerizer.cpp (line 711)
<https://reviews.apache.org/r/41820/#comment173985>

    I realized another issue with command executors. We don't have to resolve it in the patch,
but I just want to mention it here so that we don't forget. You probably can add a TODO comments
here.
    
    For command executors, we modify the executorInfo so that the user image will be mounted
in as a volume. However, we also need to figure out a way to support passing and handling
those runtime configurations in the image.
    
    cc @tnachen



src/slave/containerizer/mesos/containerizer.cpp (lines 1447 - 1451)
<https://reviews.apache.org/r/41820/#comment173984>

    Again, we should call provisioner->destroy after all isolators have been cleaned up.
Also, it does not make sense to put provisioner->destroy in cleanupIsolators.



src/slave/containerizer/mesos/isolators/filesystem/linux.hpp (lines 45 - 46)
<https://reviews.apache.org/r/41820/#comment173955>

    This fits in one line?



src/slave/containerizer/mesos/isolators/filesystem/linux.hpp (lines 77 - 78)
<https://reviews.apache.org/r/41820/#comment173956>

    Ditto.



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 62 - 63)
<https://reviews.apache.org/r/41820/#comment173957>

    Ditto.



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 173 - 174)
<https://reviews.apache.org/r/41820/#comment173958>

    Fits in one line?



src/tests/containerizer/filesystem_isolator_tests.cpp (lines 157 - 158)
<https://reviews.apache.org/r/41820/#comment173959>

    Fits in one line?



src/tests/containerizer/filesystem_isolator_tests.cpp (lines 1008 - 1009)
<https://reviews.apache.org/r/41820/#comment173960>

    FIts in one line?


- Jie Yu


On Jan. 6, 2016, 11:41 p.m., Gilbert Song wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
>     https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/containerizer.hpp 89d18624b08f2c953b9fa21663aeda694d4aac12

>   src/slave/containerizer/mesos/containerizer.cpp f3c370aeb331beb6202fd30cd0278877da0b42e0

>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp a29f9d184a0d1088577b3168a12bc8c06493a477

>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 8af2cecdca947e55d8d39e26d2fc3d42212f36c3

>   src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1

> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> -------
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>


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