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 37881: Implemented AppcProvisioner.
Date Sun, 30 Aug 2015 07:36:32 GMT

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


1st pass. Haven't checked the recovery logic yet.


src/slave/containerizer/provisioners/appc.cpp (line 80)
<https://reviews.apache.org/r/37881/#comment152716>

    Maybe s/provisionerDir/root/ ?
    
    The 'provisioner' part is redundant because the class name already contains "Privisioner".



src/slave/containerizer/provisioners/appc.cpp (line 83)
<https://reviews.apache.org/r/37881/#comment152715>

    Why not const as well?



src/slave/containerizer/provisioners/appc.cpp (line 106)
<https://reviews.apache.org/r/37881/#comment152718>

    "... provisioner root directory"



src/slave/containerizer/provisioners/appc.cpp (line 114)
<https://reviews.apache.org/r/37881/#comment152719>

    Ditto.



src/slave/containerizer/provisioners/appc.cpp (line 118)
<https://reviews.apache.org/r/37881/#comment152717>

    CHECK_SOME



src/slave/containerizer/provisioners/appc.cpp (lines 131 - 132)
<https://reviews.apache.org/r/37881/#comment152720>

    Let's try to make the indentation in this file consistent (don't mix).
    
    Either
    ```
    return Error(xxx,
                 yyy);
    ```
    
    Or
    ```
    return Error(
        xxx,
        yyy);
    ```



src/slave/containerizer/provisioners/appc.cpp (lines 174 - 181)
<https://reviews.apache.org/r/37881/#comment152721>

    Let's put all the logics in AppcProvisionerProcess (which is the convention we used consistently
in the code base).



src/slave/containerizer/provisioners/appc.cpp (lines 328 - 332)
<https://reviews.apache.org/r/37881/#comment152729>

    We try to avoid race as much as possible, even if 'backends' here is read only.
    
    Can you pull this code into `_provision`? I found it more readable then using a lambda
here.
    
    Lambda is useful if it does not rely on 'this' (i.e., a static method).
    
    ```
    return store->get(image)
      .then(defer(self(), &Self::_provision, containerId, rootfs, lambda::_1));
      
    Future<string> _provision(...)
    {
      ...
      return backends[..]->provision(layers, rootfs)
        .then([rootfs]() { return rootfs; });
    }
    ```



src/slave/containerizer/provisioners/appc.cpp (lines 334 - 339)
<https://reviews.apache.org/r/37881/#comment152723>

    I think the containerizer will still call containerizer->destroy if provisioning fails.
That means the filesystem isolator cleanup will be invoked, meaning that provisioner->destroy
will be called as well.
    
    So, I think we should rely on 'destroy' here to cleanup those partially constructed rootfses.
    
    In other words, no need for the onFailed callback here.
    
    I would suggest you take a look at the port mapping isolator.



src/slave/containerizer/provisioners/appc.cpp (lines 365 - 367)
<https://reviews.apache.org/r/37881/#comment152732>

    Let's try not to abuse lambda here.
    
    This code is buggy because 'info.erase' can be executed in other threads! Race condition!
    
    Also, if you take a look at the port mapping isolator. We remove Info from infos first
and then start to do cleaning. The cleaning can fail of course. In that case, we rely on the
'recovery' logic to properly clean them up later.



src/slave/containerizer/provisioners/appc.cpp (lines 372 - 381)
<https://reviews.apache.org/r/37881/#comment152734>

    Hum, that makes me wonder if we need to return bool for backends. Looks like that's not
necessary. Also, you need to consider the case where provision fails partially.


- Jie Yu


On Aug. 30, 2015, 4:28 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2015, 4:28 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
>     https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829

>   src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7

>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020

> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> -------
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


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