mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 72716: Added implementation of the CSI server.
Date Wed, 29 Jul 2020 15:27:13 GMT

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




src/Makefile.am
Lines 1300-1301 (patched)
<https://reviews.apache.org/r/72716/#comment310346>

    I think we should put these two lines at L1211 (right after `slave/container_logger.cpp`).



src/slave/csi_server.cpp
Lines 83-84 (patched)
<https://reviews.apache.org/r/72716/#comment310348>

    A newline between.



src/slave/csi_server.cpp
Lines 89-90 (patched)
<https://reviews.apache.org/r/72716/#comment310347>

    Ditto.



src/slave/csi_server.cpp
Lines 104 (patched)
<https://reviews.apache.org/r/72716/#comment310349>

    Can we add a comment for this hashmap? Like what is the key of this hashmap.



src/slave/csi_server.cpp
Lines 118 (patched)
<https://reviews.apache.org/r/72716/#comment310350>

    I think we do not need `csi` prefix here since there is already `csi` in the name of this
class.



src/slave/csi_server.cpp
Lines 121 (patched)
<https://reviews.apache.org/r/72716/#comment310351>

    Ditto. I think `pluginConfigs` is good enough just like the field `hashmap<string,
CSIPlugin> plugins;`.



src/slave/csi_server.cpp
Lines 123 (patched)
<https://reviews.apache.org/r/72716/#comment310355>

    So we use a single runtime for all the plugins managed by CSI server, will that cause
any problems?



src/slave/csi_server.cpp
Lines 125 (patched)
<https://reviews.apache.org/r/72716/#comment310353>

    I think each plugin should have its own metrics, so maybe we should put this field into
`struct csiPlugin`?



src/slave/csi_server.cpp
Lines 159 (patched)
<https://reviews.apache.org/r/72716/#comment310352>

    Kill this line.



src/slave/csi_server.cpp
Lines 194 (patched)
<https://reviews.apache.org/r/72716/#comment310354>

    I think we should set this based on the CSI services in the related `CSIPluginInfo` rather
than hard code here, e.g. in future we may support both node and controller services for Portworx.
    
    Or maybe we should not have this parameter at all? I mean `ServiceManager` should be able
to figure out the support services of the plugin based on the related `CSIPluginInfo`, right?



src/slave/csi_server.cpp
Lines 233-235 (patched)
<https://reviews.apache.org/r/72716/#comment310356>

    I am just curious what would happen if any of the initialization logic fail, how will
the failure be propogated back?



src/slave/csi_server.cpp
Lines 244-245 (patched)
<https://reviews.apache.org/r/72716/#comment310368>

    Do we have to use `started` and `initializationCallbacks`? Can we do the similar with
https://github.com/apache/mesos/blob/1.10.0/src/csi/v1_volume_manager.cpp#L1336 ?



src/slave/csi_server.cpp
Lines 266-270 (patched)
<https://reviews.apache.org/r/72716/#comment310357>

    I think we should only return the mount target path until `volumeManager->publishVolume`
is done, right? So maybe we need a `then` here?



src/slave/csi_server.cpp
Lines 273-274 (patched)
<https://reviews.apache.org/r/72716/#comment310358>

    I think the second line should be `plugins[volume.plugin_name()].info.name()` which is
`default` by default, right?



src/slave/csi_server.cpp
Lines 442 (patched)
<https://reviews.apache.org/r/72716/#comment310369>

    Instead of `CREATED`, I think we need to set the state to `VOL_READY` or `NODE_READY`?
And we also need to set `pre_provisioned` to true.



src/slave/csi_server.cpp
Lines 442 (patched)
<https://reviews.apache.org/r/72716/#comment310370>

    Instead of `CREATED`, I think we need to set the state to `VOL_READY` or `NODE_READY`?
And we also need to set `pre_provisioned` to true.


- Qian Zhang


On 七月 29, 2020, 5:50 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72716/
> -----------------------------------------------------------
> 
> (Updated 七月 29, 2020, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Qian Zhang.
> 
> 
> Bugs: MESOS-10163
>     https://issues.apache.org/jira/browse/MESOS-10163
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation of the CSI server.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt bcb5128f0e61af0d541502e4ed833da0487b7792 
>   src/Makefile.am a89919dd7d5ccbc4c5fa79d9a83616608f84ef49 
>   src/slave/csi_server.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72716/diff/2/
> 
> 
> Testing
> -------
> 
> Currently work-in-progress. Unit tests are forthcoming.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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