Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[Chun-Hung Hsiao]

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

(Updated Feb. 6, 2019, 4:59 a.m.)


Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.


Changes
---

Addressed Benjamin's comment.


Bugs: MESOS-9544
https://issues.apache.org/jira/browse/MESOS-9544


Repository: mesos


Description
---

`DESTROY_DISK` would bypass persistent volume cleanup and directly ask
the CSI plugin to delete the backed volume. Since the CSI spec does not
require the plugin to do data cleanup, to avoid data leakage, we require
that if there is persistent volume on the CSI volume, it should be
destroyed first.


Diffs (updated)
-

  src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
  src/tests/master_validation_tests.cpp 
c00e8bb315c28bdf438da2089dd81f5e348982e5 


Diff: https://reviews.apache.org/r/69894/diff/2/

Changes: https://reviews.apache.org/r/69894/diff/1-2/


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 5:15 p.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2577-2579 (patched)
> > 
> >
> > Maybe make this more in line with what we already have for `UNRESERVE` 
> > of persistent volumes?
> > ```
> > A disk resource containing a persistent volume {source} cannot be 
> > destroyed directly. Please destroy the persistent volume first then destroy 
> > the disk resource.
> > ```
> > Also, spaces either at start or end of line, not mixed. At the start 
> > seems popular.

The line breaking is done by clang-format ;) But I'll follow your suggestion.


- Chun-Hung


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


On Feb. 5, 2019, 7:43 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69894/
> ---
> 
> (Updated Feb. 5, 2019, 7:43 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
> ---
> 
> `DESTROY_DISK` would bypass persistent volume cleanup and directly ask
> the CSI plugin to delete the backed volume. Since the CSI spec does not
> require the plugin to do data cleanup, to avoid data leakage, we require
> that if there is persistent volume on the CSI volume, it should be
> destroyed first.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/tests/master_validation_tests.cpp 
> c00e8bb315c28bdf438da2089dd81f5e348982e5 
> 
> 
> Diff: https://reviews.apache.org/r/69894/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/master/validation.cpp
Lines 2577-2579 (patched)


Maybe make this more in line with what we already have for `UNRESERVE` of 
persistent volumes?
```
A disk resource containing a persistent volume {source} cannot be destroyed 
directly. Please destroy the persistent volume first then destroy the disk 
resource.
```
Also, spaces either at start or end of line, not mixed. At the start seems 
popular.


- Benjamin Bannier


On Feb. 5, 2019, 8:43 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69894/
> ---
> 
> (Updated Feb. 5, 2019, 8:43 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
> ---
> 
> `DESTROY_DISK` would bypass persistent volume cleanup and directly ask
> the CSI plugin to delete the backed volume. Since the CSI spec does not
> require the plugin to do data cleanup, to avoid data leakage, we require
> that if there is persistent volume on the CSI volume, it should be
> destroyed first.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/tests/master_validation_tests.cpp 
> c00e8bb315c28bdf438da2089dd81f5e348982e5 
> 
> 
> Diff: https://reviews.apache.org/r/69894/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[James DeFelice]

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


Ship it!




Ship It!

- James DeFelice


On Feb. 5, 2019, 7:43 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69894/
> ---
> 
> (Updated Feb. 5, 2019, 7:43 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
> ---
> 
> `DESTROY_DISK` would bypass persistent volume cleanup and directly ask
> the CSI plugin to delete the backed volume. Since the CSI spec does not
> require the plugin to do data cleanup, to avoid data leakage, we require
> that if there is persistent volume on the CSI volume, it should be
> destroyed first.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/tests/master_validation_tests.cpp 
> c00e8bb315c28bdf438da2089dd81f5e348982e5 
> 
> 
> Diff: https://reviews.apache.org/r/69894/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>