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/12/01 01:36:22 UTC
Re: Review Request 63021: Added `getService()` function to launch CSI
plugins.
-----------------------------------------------------------
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.
Changes
-------
Rebased.
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/63021/diff/13-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/#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/#review192538
-----------------------------------------------------------
src/resource_provider/storage/provider.cpp
Lines 173 (patched)
<https://reviews.apache.org/r/63021/#comment270749>
I'd rename this to `getCSIPluginContainerInfo`
- 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
>
>