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 69893: Clean up persistent volumes on SLRP disks.
Date Wed, 06 Feb 2019 04:54:36 GMT


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3137 (patched)
> > <https://reviews.apache.org/r/69893/diff/1/?file=2123916#file2123916line3139>
> >
> >     This is some pretty strong coupling to `master::validate` checking `resource::validatePersistentVolume`.
That's probably okay, but would could also just re-validate here instead.

This is similar to what we do between the master and the agent: the master validates and the
agent checks.


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3125-3126 (original), 3138-3139 (patched)
> > <https://reviews.apache.org/r/69893/diff/1/?file=2123916#file2123916line3140>
> >
> >     Do we actually validate this somewhere?

https://github.com/apache/mesos/blob/ae7e7330d7fc7bbe2ce923025025577e087f8348/src/master/validation.cpp#L2570-L2573


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3230 (patched)
> > <https://reviews.apache.org/r/69893/diff/1/?file=2123916#file2123916line3232>
> >
> >     Is this validated somewhere?

https://github.com/apache/mesos/blob/ae7e7330d7fc7bbe2ce923025025577e087f8348/src/master/validation.cpp#L2570-L2573
combined with L3208--L3213 in this patch.


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3234 (patched)
> > <https://reviews.apache.org/r/69893/diff/1/?file=2123916#file2123916line3236>
> >
> >     Is this validated?

All MOUNT disk resources managed by SLRP will have an entry in `volumes`, and the master only
allows an operation to be applied if its consumed resources are contained in the resource
pool, hence this is validated implicitly by the master.


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3244 (patched)
> > <https://reviews.apache.org/r/69893/diff/1/?file=2123916#file2123916line3246>
> >
> >     Does this break seemless upgrades? Probably okay, but still something worth
documenting in the upgrades guide.

This depends on what we'll do for https://reviews.apache.org/r/69892/#comment298376. In the
current proposal where there are separated boot IDs, this won't introduce any upgrade issue
since it will be empty anyway. During SLRP recovery, it would treat an existing published
volume as `VOL_READY` but not `PUBLISHED` and will call `NodePublishVolume` again. But this
is okay since `NodePublishVolume` is idempotent.


- Chun-Hung


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


On Feb. 5, 2019, 7:42 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69893/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2019, 7:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
>     https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch limits SLRP to only support persistent volumes on MOUNT
> disks, and makes it clean up data in persistent volumes when processing
> `DESTROY` operations.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/storage/provider.cpp d6e20a549ede189c757ae3ae922ab7cb86d2be2c

>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69893/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> More testing done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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