mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 70168: Added `ServiceManager` to manage CSI plugin container lifecycles.
Date Wed, 20 Mar 2019 13:21:02 GMT


> On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote:
> > src/csi/service_manager.cpp
> > Lines 184-185 (patched)
> > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line184>
> >
> >     In Mesos we typically expand types unless they are already mentioned somewhere
in the statement or are nested types (e.g., iterators).
> >     ````
> >     foreach (const CSIPluginContainerInfo::Service& service, services) {
> >       foreach (const CSIPluginContainerInfo& container, info.containers()) {
> >         ...
> >       }
> >     }
> >     ````
> 
> Chun-Hung Hsiao wrote:
>     This is actually the debate around `auto` ;) But I don't have a strong opinion on
this and the types seems less likely to change, I'll take your suggestion.

I do have a strong opinion, but I understand that consistency trumps everything ;)


> On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote:
> > src/csi/service_manager.cpp
> > Lines 316 (patched)
> > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line316>
> >
> >     Let's just use the `static` function like we had previously,
> >     ```
> >     static ContainerID getContainerId(
> >         const CSIPluginInfo& info,
> >         const CSIPluginContainerInfo& container);
> >     ```
> 
> Chun-Hung Hsiao wrote:
>     We'll have to pass in `containerPrefix` as well. Do you have a reason that makes
you prefer a static function, given that this is already in the implementation file so won't
be leaked?

I primarily suggested to use a free-standing function as before to reduce number of things
we do in this patch.

IMO free-standing functions have better defined state and coupling; it is much easier to accidentally
completely entangle a member function with other, unrelated instance state than it is to do
the same to a non-member function.


> On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote:
> > src/csi/service_manager.cpp
> > Lines 332-338 (patched)
> > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line332>
> >
> >     Maybe the following instead?
> >     ````
> >     auto it = std::find_if(
> >         info.containers().begin(),
> >         info.containers().end(),
> >         [this, containerId](const CSIPluginContainerInfo& info) {
> >           return getContainerId(info) == containerId;
> >         });
> >         
> >     return it != info.containers().end()
> >              ? *it
> >              : Option<CSIPluginContainerInfo>::none();
> >     ````
> 
> Chun-Hung Hsiao wrote:
>     I actually thought about this before. But I'm not sure if this is more readable than
the current one. WDYT?

My argument would be that the replacment would be idiomatic C++ and have a higher level of
abstraction. That should in general not only reduce the likelihood of bugs, but also lead
to more maintainable code. See e.g., https://sean-parent.stlab.cc/presentations/2013-09-11-cpp-seasoning/cpp-seasoning.pdf.
IMO raising the abstraction level in Mesos for such issues would be helpful.

We currently don't seem to have a strong preference in the project one way or another though,
so it's a matter of taste which I'll leave up to you.


> On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote:
> > src/csi/service_manager.cpp
> > Lines 519-520 (patched)
> > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line519>
> >
> >     How about avoiding the hard assertion here and instead returning a `Failure`?
> >     
> >     Since this was in the existing code, so maybe let's do any cleanup in a separate
patch.
> 
> Chun-Hung Hsiao wrote:
>     This should not happen. The public `getServiceEndpoint` function already returns
a failure. Dropping.

Sounds good.


> On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote:
> > src/csi/service_manager.cpp
> > Lines 614 (patched)
> > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line614>
> >
> >     Should we have a metric for the start action as well?
> 
> Chun-Hung Hsiao wrote:
>     It seems not adding much value to me. The start action count itself doesn't provide
much value unless comparing it with the termination count, which could be used to deduce the
number of running containers. If that's the purpose, then maybe it's more direct to just add
a metrics for running containers.
>     
>     However, the number of running containers is tightly coupled with whether the RP
would launch a single plugin container (unified CSI deployment) or two containers (split-component
deployment). And that means potentially the monitoring configuration using this metric would
be couple with the RP configs, which seems not good. The termination count itself seems sufficient
to detect plugin failures.
>     
>     We can discuss if this is necessary in another ticket.

Make sense.


- Benjamin


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


On March 20, 2019, 6:36 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70168/
> -----------------------------------------------------------
> 
> (Updated March 20, 2019, 6:36 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9632
>     https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `ServiceManager` will be used to refactor SLRP to move container
> management out from SLRP. It is agnostic to CSI versions, so can be used
> to manage plugin containers for both CSI v0 and v1 plugins. Most of its
> logic are adapted from the SLRP code.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/csi/service_manager.hpp PRE-CREATION 
>   src/csi/service_manager.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70168/diff/7/
> 
> 
> Testing
> -------
> 
> Testing done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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