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 69970: Made SLRP clean up mount directories for destroyed MOUNT disks.
Date Wed, 27 Feb 2019 18:46:31 GMT


> On Feb. 27, 2019, 1:53 p.m., Benjamin Bannier wrote:
> > src/csi/paths.cpp
> > Lines 285-289 (patched)
> > <https://reviews.apache.org/r/69970/diff/1/?file=2125062#file2125062line285>
> >
> >     I wonder whether it makes more sense to just inline this definition and get
rid of the function (for now).

It's more about putting all path laout logics into this file so we got one place to look at.
WDYT?


> On Feb. 27, 2019, 1:53 p.m., Benjamin Bannier wrote:
> > src/csi/paths.cpp
> > Lines 316-321 (patched)
> > <https://reviews.apache.org/r/69970/diff/1/?file=2125062#file2125062line316>
> >
> >     If this function would accept a `const Path& dir` we wouldn't need to create
the temporary paths (there's a `Path::string` function to get a string representation).
> >     
> >     That being said, `Path` is very underappreciated in our code base, so feel free
to drop.

This is for consistency. Also this is supposed to be consuming the output from `getMountPaths`.
Dropping.


> On Feb. 27, 2019, 1:53 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 975-980 (patched)
> > <https://reviews.apache.org/r/69970/diff/1/?file=2125063#file2125063line975>
> >
> >     This almost seems like a hard error to me (but would introduce coupling). Maybe
just inline the function here and possibly assert.

This definitely means the csidir is somehow "damaged." But the agent can operate without it,
and the failure would fail the recovery and terminate the SLRP in the `recover()` function.


- Chun-Hung


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


On Feb. 13, 2019, 5:54 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69970/
> -----------------------------------------------------------
> 
> (Updated Feb. 13, 2019, 5:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
> 
> 
> Bugs: MESOS-9568
>     https://issues.apache.org/jira/browse/MESOS-9568
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When the SLRP stage/publish a CSI volume, it creates the staging/target
> paths, but only the target path is cleaned up during unpublish.
> Moreover, even if the CSI plugin does not support node-staging, the
> current cleanup is still not enough, as the parent directory of the
> target path is not removed. This patch fixes this problem.
> 
> 
> Diffs
> -----
> 
>   src/csi/paths.hpp 7a4e9e0e85ca33c8be0a16ff4531c765f40e0adc 
>   src/csi/paths.cpp 772d960d96d481949cf25bc464e39e1ec15cd27d 
>   src/resource_provider/storage/provider.cpp 09a710d668a5a7460b6c4e4fa32d3829dca7ac55

>   src/resource_provider/storage/provider_process.hpp 36187fb4a25c49653530ee286fa9c1663177fbc6

> 
> 
> Diff: https://reviews.apache.org/r/69970/diff/1/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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