mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 70213: Added the `VolumeManager` interface to manage CSI volumes.
Date Wed, 20 Mar 2019 06:54:25 GMT


> On March 19, 2019, 2:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.hpp
> > Lines 74-75 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132465#file2132465line74>
> >
> >     Do we have a chance here to translate from CSI semantics to our own semantics
instead of leaking the CSI protocol? I think not using a magic value of `0` would be nice.
> >     
> >     Seems like we could either return a `Failure` if `GET_CAPACITY` is not supported
or some `Option<Bytes>::none()`. This would likely require capability knowledge.

I'm actually debating if we should expose CSI capabilities. Given that the capabilities might
change, do you think it's a good idea to expose that?


> On March 19, 2019, 2:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.hpp
> > Lines 77 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132465#file2132465line77>
> >
> >     Shouldn't this be version-agnostic?

Addressed in r/70247 and r/70248. These patches are put later in chain because I'm considering
if the first few refactoring patches should be backported for backporting bug fixes in the
future. But as you suggested, this is too big of a change so could be a terrible idea. Instead
maybe we should just do extra backporting efforts in the future, and only for critical bugs.

If this is the plan, I could reorder the patches.

Dropping the following "ditto" issues but leave this one open for discussion.


> On March 19, 2019, 2:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.hpp
> > Lines 115-117 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132465#file2132465line115>
> >
> >     Am I reading correctly that the return boolean is not dynamic and just maps
onto whether the controller has `CREATE_DELETE_VOLUME` or not?

Yes you're right lol. This roughly matches the semantics defined here: https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L2102

Again, not sure if we should expose the CSI capabilities instead. WDYT?


> On March 19, 2019, 2:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.cpp
> > Lines 28-35 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132466#file2132466line28>
> >
> >     How do you plan to dispatch here once we need to support both `v0` and `v1`?
Can we infer what needs to be done from these arguments alone?

Nope. This will return `Future<Owned<VolumeManager>>` in the future, where a `ServiceManager`
is created first to detect CSI versions then the proper `VolumeManager` is created accordingly.


> On March 19, 2019, 2:42 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.cpp
> > Lines 44 (patched)
> > <https://reviews.apache.org/r/70213/diff/2/?file=2132466#file2132466line44>
> >
> >     `#include <stout/errorbase.hpp>`

I intentionally don't want to use `errorbase.hpp` as `error.hpp` seems to be a more proper
public header for inclusion. Dropping. Feel free to reopen.


- Chun-Hung


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


On March 15, 2019, 5:14 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70213/
> -----------------------------------------------------------
> 
> (Updated March 15, 2019, 5:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9622
>     https://issues.apache.org/jira/browse/MESOS-9622
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `VolumeManager` is a wrapper for SLRP to use v0 and v1 CSI plugins
> polymorphically. It will be managing volume lifecycles and checkpoints
> and making the actual CSI calls for SLRP and SERP in the future.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/csi/volume_manager.hpp PRE-CREATION 
>   src/csi/volume_manager.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70213/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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