----------------------------------------------------------- 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 > >
