mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 72716: Added implementation of the CSI server.
Date Fri, 31 Jul 2020 19:00:30 GMT


> On July 29, 2020, 3:27 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 123 (patched)
> > <https://reviews.apache.org/r/72716/diff/2/?file=2236468#file2236468line123>
> >
> >     So we use a single runtime for all the plugins managed by CSI server, will that
cause any problems?

Hmm I'm not completely sure; I switched to using a separate runtime for each plugin.


> On July 29, 2020, 3:27 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 194 (patched)
> > <https://reviews.apache.org/r/72716/diff/2/?file=2236468#file2236468line194>
> >
> >     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?

Good point. Yea we may want to remove the service information from the plugin info, I'm not
sure. I'll leave it there for now and just initialize the volume manager with the correct
services here.


> On July 29, 2020, 3:27 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 233-235 (patched)
> > <https://reviews.apache.org/r/72716/diff/2/?file=2236468#file2236468line233>
> >
> >     I am just curious what would happen if any of the initialization logic fail,
how will the failure be propogated back?

I updated the server so that now `start()` returns a future associated with the initialization.


> On July 29, 2020, 3:27 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 244-245 (patched)
> > <https://reviews.apache.org/r/72716/diff/2/?file=2236468#file2236468line244>
> >
> >     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 ?

The reason it's more complicated here is because we may add more "initialization logic" after
server construction if publish/unpublish calls are made before the server is started. So we
need an approach which will allow us to add more function calls which are executed during
startup. I explored another approach while coding but this is what I ended up settling on,
but I'm happy to explore other options if we can think of something better.


- Greg


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


On July 31, 2020, 7 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72716/
> -----------------------------------------------------------
> 
> (Updated July 31, 2020, 7 p.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/3/
> 
> 
> Testing
> -------
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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