You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/11/04 01:06:23 UTC

Re: Review Request 63021: Added functions to launch CSI plugin in storage local resource provider.

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

(Updated Nov. 4, 2017, 1:06 a.m.)


Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
-------

Updated using the new `storage::paths` helper functions.


Bugs: MESOS-8032
    https://issues.apache.org/jira/browse/MESOS-8032


Repository: mesos


Description (updated)
-------

During initialization, the storage local resource provider first tries
to recover its ID of the last session through reading the actual path
linked by `<work_dir>/meta/resource_providers/<type>/<name>/latest`,
then subscribe to the agent's resource provider manager. If this is a
new subscription, it will set up a new dir for CSI plugins to put their
socket files.

Once the CSI socket dir is set up, the storage local resource provider
can connect to a CSI plugin through the following procedure:
1. It first tries to connect to the existing socket file if there is
   one. On success, just return the connection.
2. On failure, kill the running plugin and remove the socket file.
3. Launch a container to run the plugin, and w for the socket file to
   appear, then connect to the socket file.


Diffs (updated)
-----

  src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f 


Diff: https://reviews.apache.org/r/63021/diff/7/

Changes: https://reviews.apache.org/r/63021/diff/6-7/


Testing
-------


Thanks,

Chun-Hung Hsiao

Re: Review Request 63021: Added `getService()` function to launch CSI plugins.

Posted by Joseph Wu <jo...@mesosphere.io>.

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




src/resource_provider/storage/provider.cpp
Lines 124 (patched)
<https://reviews.apache.org/r/63021/#comment268755>

    Usually a bad idea to use Path helpers with URIs.  
    
    Mostly because... Windows...  I doubt SLRP items will reach Windows before we have an actual URI class with helpers, so it should be alright to leave a TODO here.



src/resource_provider/storage/provider.cpp
Lines 130-141 (patched)
<https://reviews.apache.org/r/63021/#comment268754>

    Hm... Can't you use this?
    http://www.cplusplus.com/reference/algorithm/find/



src/resource_provider/storage/provider.cpp
Lines 272-275 (patched)
<https://reviews.apache.org/r/63021/#comment268756>

    Does this mean we need to manually update this version each time we bump the CSI 3rdparty bundle?



src/resource_provider/storage/provider.cpp
Lines 394 (patched)
<https://reviews.apache.org/r/63021/#comment268757>

    Whoops, double period..


- Joseph Wu


On Nov. 14, 2017, 6:40 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63021/
> -----------------------------------------------------------
> 
> (Updated Nov. 14, 2017, 6:40 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8032
>     https://issues.apache.org/jira/browse/MESOS-8032
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `getService()` method first checks if there is already a container
> daemon for the specified plugin, and creates a new one if not. The
> post-start hook for the container daemon will call `connect()` to
> wait for the endpoint socket file to appear and connect to it, then
> set up the corresponding promise of CSI client. The post-stop hook will
> remove the socket file and create a new promise for the next call to the
> post-start hook to set it up.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/provider.cpp 46224997430ac0c568904d80014166a6f059907f 
> 
> 
> Diff: https://reviews.apache.org/r/63021/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63021: Added `getService()` function to launch CSI plugins.

Posted by Jie Yu <yu...@gmail.com>.

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




src/resource_provider/storage/provider.cpp
Lines 188 (patched)
<https://reviews.apache.org/r/63021/#comment270822>

    Can you add a TODO here? This is a bit hacky way to get the v1 operator API endpoint.


- Jie Yu


On Dec. 1, 2017, 1:36 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63021/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2017, 1:36 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8032
>     https://issues.apache.org/jira/browse/MESOS-8032
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `getService()` method first checks if there is already a container
> daemon for the specified plugin component, and creates a new one if not.
> The post-start hook for the container daemon will call `connect()` to
> wait for the endpoint socket file to appear and connect to it, then
> set up the corresponding promise of CSI client. The post-stop hook will
> remove the socket file and create a new promise for the next call to the
> post-start hook to set it up.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp a348c7df083324602d25f40069ad49e29f7918a5 
>   src/common/type_utils.cpp 3657d55ab2a052b5c659022e3fc264e748429d54 
>   src/resource_provider/storage/provider.cpp f586afc256fbcb2f2bf2451ffb7e7ba1d59cfa78 
> 
> 
> Diff: https://reviews.apache.org/r/63021/diff/14/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63021: Added `getService()` function to launch CSI plugins.

Posted by Jie Yu <yu...@gmail.com>.

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


Fix it, then Ship it!





src/common/type_utils.cpp
Lines 746 (patched)
<https://reviews.apache.org/r/63021/#comment270703>

    Please udpate v1



src/resource_provider/storage/provider.cpp
Lines 154 (patched)
<https://reviews.apache.org/r/63021/#comment270710>

    Can we change `getContainerIdPrefix` to not have the tailing `-`?
    
    So here, you basically construct three things:
    1) prefix
    2) csi plugin type/name
    3) services
    
    and join them by `--`



src/resource_provider/storage/provider.cpp
Lines 508-511 (patched)
<https://reviews.apache.org/r/63021/#comment270726>

    Instead of nesting, i'd prefer flat them out:
    
    ```
    return client
      .then(defer(..., [=](const csi::Client& client) {
        return client.GetSupportedVersion(...);
      })
      .then(defer(..., [=](const ...) {
        ...
      });
    ```
    
    Any reason not using `const ref` for the parameter?



src/resource_provider/storage/provider.cpp
Lines 591 (patched)
<https://reviews.apache.org/r/63021/#comment270735>

    You need to return after calling `fatal`


- Jie Yu


On Dec. 1, 2017, 1:36 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63021/
> -----------------------------------------------------------
> 
> (Updated Dec. 1, 2017, 1:36 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8032
>     https://issues.apache.org/jira/browse/MESOS-8032
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `getService()` method first checks if there is already a container
> daemon for the specified plugin component, and creates a new one if not.
> The post-start hook for the container daemon will call `connect()` to
> wait for the endpoint socket file to appear and connect to it, then
> set up the corresponding promise of CSI client. The post-stop hook will
> remove the socket file and create a new promise for the next call to the
> post-start hook to set it up.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp a348c7df083324602d25f40069ad49e29f7918a5 
>   src/common/type_utils.cpp 3657d55ab2a052b5c659022e3fc264e748429d54 
>   src/resource_provider/storage/provider.cpp f586afc256fbcb2f2bf2451ffb7e7ba1d59cfa78 
> 
> 
> Diff: https://reviews.apache.org/r/63021/diff/14/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>