Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-12 Thread Benjamin Bannier 

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


Ship it!




Ship It!

- Benjamin Bannier


On Feb. 12, 2019, 9:42 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 12, 2019, 9:42 p.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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> 09a710d668a5a7460b6c4e4fa32d3829dca7ac55 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-12 Thread Chun-Hung Hsiao 

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

(Updated Feb. 12, 2019, 8:42 p.m.)


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


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

If a CSI volume has been node-published before a reboot, SLRP will now
try to bring it back to node-published again. This is important to
perform synchronous persistent volume cleanup for `DESTROY`.

To achieve this, in addition to keeping track of the boot ID when a CSI
volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
`VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
the volume is node-published. This helps SLRP to better determine if a
volume has been published before reboot.


Diffs (updated)
-

  src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
  src/resource_provider/storage/provider.cpp 
09a710d668a5a7460b6c4e4fa32d3829dca7ac55 


Diff: https://reviews.apache.org/r/69892/diff/3/

Changes: https://reviews.apache.org/r/69892/diff/2-3/


Testing
---

`make check`

Testing for publish failures will be done later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-12 Thread Chun-Hung Hsiao 


> On Feb. 12, 2019, 1:24 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 945-950 (patched)
> > 
> >
> > Hmm, executing this only on the `!node_publish_required` path seems 
> > asymmetric. Could we install this unconditionally?
> 
> Chun-Hung Hsiao wrote:
> Good suggestion. Sure let me add logging for the other case as well.

However, the `recover` callback here is to intentionally skip the failure for 
the `!node_publish_required` case. This makes the behavior more consistent with 
other scenarios, e.g., when the SLRP fails to publish or delete a volume, it 
returns the error but continue to run, so it should do so even if there's a 
recovery in between.


- Chun-Hung


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


On Feb. 12, 2019, 5:05 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 12, 2019, 5:05 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> 09a710d668a5a7460b6c4e4fa32d3829dca7ac55 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-12 Thread Chun-Hung Hsiao 


> On Feb. 12, 2019, 1:24 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 797 (patched)
> > 
> >
> > Since this is `defer`'ed, any chance this could race with 
> > `deleteVolume` and the volume state at `volumeId` not being there anymore?

No for the following reasons:
1. The volumes are recovered first, which happen before registration and 
replaying pending operations. 
2. As a safety guard and for consistency, all calls operating on the same 
volume will be in a sequence, see line 958 in this patch and 
https://github.com/apache/mesos/blob/c8e3553022f5949bf8f5f6984e283a4861f9d74f/src/resource_provider/storage/provider.cpp#L3051.

Dropping.


> On Feb. 12, 2019, 1:24 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 897 (patched)
> > 
> >
> > We could pull the lambda into its own variable to remove a level of 
> > nesting.

It seems it's not much more readable so I'll probably keep it as, unless you 
feel this code is not readable at all ;)


> On Feb. 12, 2019, 1:24 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 898 (patched)
> > 
> >
> > Similar race possible here?

No. See above.


> On Feb. 12, 2019, 1:24 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 945-950 (patched)
> > 
> >
> > Hmm, executing this only on the `!node_publish_required` path seems 
> > asymmetric. Could we install this unconditionally?

Good suggestion. Sure let me add logging for the other case as well.


- Chun-Hung


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


On Feb. 12, 2019, 5:05 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 12, 2019, 5:05 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> 09a710d668a5a7460b6c4e4fa32d3829dca7ac55 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-12 Thread Benjamin Bannier 

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




src/resource_provider/storage/provider.cpp
Lines 797 (patched)


Since this is `defer`'ed, any chance this could race with `deleteVolume` 
and the volume state at `volumeId` not being there anymore?



src/resource_provider/storage/provider.cpp
Lines 897 (patched)


We could pull the lambda into its own variable to remove a level of nesting.



src/resource_provider/storage/provider.cpp
Lines 898 (patched)


Similar race possible here?



src/resource_provider/storage/provider.cpp
Lines 903-920 (patched)


Took me a while to understand this, but a pretty nifty async jump table 
impl ;).



src/resource_provider/storage/provider.cpp
Lines 945-950 (patched)


Hmm, executing this only on the `!node_publish_required` path seems 
asymmetric. Could we install this unconditionally?


- Benjamin Bannier


On Feb. 12, 2019, 6:05 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 12, 2019, 6:05 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> 09a710d668a5a7460b6c4e4fa32d3829dca7ac55 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-11 Thread Chun-Hung Hsiao 

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

(Updated Feb. 12, 2019, 5:05 a.m.)


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


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

If a CSI volume has been node-published before a reboot, SLRP will now
try to bring it back to node-published again. This is important to
perform synchronous persistent volume cleanup for `DESTROY`.

To achieve this, in addition to keeping track of the boot ID when a CSI
volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
`VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
the volume is node-published. This helps SLRP to better determine if a
volume has been published before reboot.


Diffs (updated)
-

  src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
  src/resource_provider/storage/provider.cpp 
09a710d668a5a7460b6c4e4fa32d3829dca7ac55 


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

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


Testing
---

`make check`

Testing for publish failures will be done later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-11 Thread Chun-Hung Hsiao 


> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.
> 
> Chun-Hung Hsiao wrote:
> Consider the following scenario:
> `CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
> `VOL_READY` -> reboot
> If we share the same `boot_id` for all transitions, we won't be able to 
> tell that this volume has been published before.
> If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that 
> there has been a reboot after the last `VOL_READY` so we need to call 
> `NodeStageVolume` again.
> 
> Chun-Hung Hsiao wrote:
> After an offline discussion, we decided to simplify the state machine, 
> and SLRP will try to bring a volume to `PUBLISHED` during recovery as long as 
> it has ever been in `VOL_READY` before. This would mean that a 
> misconfiguration that makes a plugin succeed on `NodeStageVolume` but fail on 
> `NodePublishVolume` will make the SLRP unable destroy the persisten volume, 
> even if no data have ever been written to it.
> 
> James DeFelice wrote:
> How would an operator recover from such a sitution?
> 
> Chun-Hung Hsiao wrote:
> The operator needs to be somehow notified, then investigate the problem, 
> then restart the SLRP (through a config update or agent restart).
> 
> It seems to me that the scenario where the plugin is able to do 
> `NodeStageVolume` but not `NodePublishVolume` should be rare.
> 
> However, it is more likely that the plugin is not 
> `STAGE_UNSTAGE_VOLUME`-capable and misconfigured in a way that it cannot do 
> `NodePublishVolume`.
> In this case SLRP would use a no-op to transition a volume from 
> `NODE_READY` to `VOL_READY`, and set up the boot ID. However, because it 
> cannot use the plugin to bring the volume to `PUBLISHED`, it won't be able to 
> destroy the PV or recover.
> 
> The originaly proposal, where we keep track of the boot IDs of 
> `NodeStageVolume` and `NodePublishVolume` separatedly, is more resilient to 
> bad plugins: PV destroy and SLRP recovery is blocked only when necessary. 
> WDYT?

Had another chat with Benjamin. Let's keep a single `boot_id` for reboot 
detection. I'll introduce a new `node_publish_required` boolean field to keep 
track of the need for republish the volume after reboot.


- Chun-Hung


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-05 Thread Chun-Hung Hsiao 


> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.
> 
> Chun-Hung Hsiao wrote:
> Consider the following scenario:
> `CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
> `VOL_READY` -> reboot
> If we share the same `boot_id` for all transitions, we won't be able to 
> tell that this volume has been published before.
> If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that 
> there has been a reboot after the last `VOL_READY` so we need to call 
> `NodeStageVolume` again.
> 
> Chun-Hung Hsiao wrote:
> After an offline discussion, we decided to simplify the state machine, 
> and SLRP will try to bring a volume to `PUBLISHED` during recovery as long as 
> it has ever been in `VOL_READY` before. This would mean that a 
> misconfiguration that makes a plugin succeed on `NodeStageVolume` but fail on 
> `NodePublishVolume` will make the SLRP unable destroy the persisten volume, 
> even if no data have ever been written to it.
> 
> James DeFelice wrote:
> How would an operator recover from such a sitution?

The operator needs to be somehow notified, then investigate the problem, then 
restart the SLRP (through a config update or agent restart).

It seems to me that the scenario where the plugin is able to do 
`NodeStageVolume` but not `NodePublishVolume` should be rare.

However, it is more likely that the plugin is not 
`STAGE_UNSTAGE_VOLUME`-capable and misconfigured in a way that it cannot do 
`NodePublishVolume`.
In this case SLRP would use a no-op to transition a volume from `NODE_READY` to 
`VOL_READY`, and set up the boot ID. However, because it cannot use the plugin 
to bring the volume to `PUBLISHED`, it won't be able to destroy the PV or 
recover.

The originaly proposal, where we keep track of the boot IDs of 
`NodeStageVolume` and `NodePublishVolume` separatedly, is more resilient to bad 
plugins: PV destroy and SLRP recovery is blocked only when necessary. WDYT?


- Chun-Hung


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-05 Thread James DeFelice 


> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.
> 
> Chun-Hung Hsiao wrote:
> Consider the following scenario:
> `CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
> `VOL_READY` -> reboot
> If we share the same `boot_id` for all transitions, we won't be able to 
> tell that this volume has been published before.
> If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that 
> there has been a reboot after the last `VOL_READY` so we need to call 
> `NodeStageVolume` again.
> 
> Chun-Hung Hsiao wrote:
> After an offline discussion, we decided to simplify the state machine, 
> and SLRP will try to bring a volume to `PUBLISHED` during recovery as long as 
> it has ever been in `VOL_READY` before. This would mean that a 
> misconfiguration that makes a plugin succeed on `NodeStageVolume` but fail on 
> `NodePublishVolume` will make the SLRP unable destroy the persisten volume, 
> even if no data have ever been written to it.

How would an operator recover from such a sitution?


- James


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-05 Thread Chun-Hung Hsiao 


> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.
> 
> Chun-Hung Hsiao wrote:
> Consider the following scenario:
> `CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
> `VOL_READY` -> reboot
> If we share the same `boot_id` for all transitions, we won't be able to 
> tell that this volume has been published before.
> If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that 
> there has been a reboot after the last `VOL_READY` so we need to call 
> `NodeStageVolume` again.

After an offline discussion, we decided to simplify the state machine, and SLRP 
will try to bring a volume to `PUBLISHED` during recovery as long as it has 
ever been in `VOL_READY` before. This would mean that a misconfiguration that 
makes a plugin succeed on `NodeStageVolume` but fail on `NodePublishVolume` 
will make the SLRP unable destroy the persisten volume, even if no data have 
ever been written to it.


- Chun-Hung


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-05 Thread Chun-Hung Hsiao 


> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.

Consider the following scenario:
`CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
`VOL_READY` -> reboot
If we share the same `boot_id` for all transitions, we won't be able to tell 
that this volume has been published before.
If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that there 
has been a reboot after the last `VOL_READY` so we need to call 
`NodeStageVolume` again.


> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 817 (original), 827 (patched)
> > 
> >
> > Is us not using a sequence anymore related to what is being done here?
> > 
> > Here and below.

No. The sequence is still used. Note that the `recovered` future is wrapped by 
a `recoverVolume` lambda, and we put the lambda in the sequence so all steps 
will be executed as a whole without being interleved with other calls. Dropping.


- Chun-Hung


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-05 Thread Benjamin Bannier 

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




src/csi/state.proto
Lines 62-67 (original), 62-77 (patched)


Any reason we cannot use a single field containing the `bootId` of the last 
transition? A single field would cut down on the number of possible message 
permutations, and also allow simpler handling (branching a changed `boot_id`, 
triggering `state`-dependent handling). We could set such a `boot_id` whenever 
there is a state transition.



src/resource_provider/storage/provider.cpp
Line 817 (original), 827 (patched)


Is us not using a sequence anymore related to what is being done here?

Here and below.


- Benjamin Bannier


On Feb. 5, 2019, 8:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 8:40 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-05 Thread Mesos Reviewbot Windows 

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['69858', '69866', '69892']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2850/mesos-review-69892

Relevant logs:

- 
[mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2850/mesos-review-69892/logs/mesos-tests.log):

```
I0205 08:43:13.151952 32844 master.cpp:11317] Removing task 
83482d1a-9498-4eb7-af15-05073d895a62 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 9bfba4c6-997f-4c2e-bff6-5867256f8ba7- on 
agent 9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0205 08:43:13.153949 33920 master.cpp:1269] Agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0205 08:43:13.154947 33920 master.cpp:3272] Disconnecting agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0205 08:43:13.154947 33920 master.cpp:3291] Deactivating agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0205 08:43:13.154947 25360 hierarchical.cpp:358] Removed framework 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-
I0205 08:43:13.154947 25360 hierarchical.cpp:793] Agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 deactivated
I0205 08:43:13.155947 34176 containerizer.cpp:2477] Destroying container 
7995f0b2-2fba-430c-98a1-abd316d3485a in RUNNING state
I0205 08:43:13.155947 34176 containerizer.cpp:3144] Transitioning the state of 
container 7995f0b2-2fba-430c-98a1-abd316d3485a from RUNNING to DESTROYING
I0205 08:43:13.156953 3417[   OK ] 
IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (687 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (704 ms total)

[--] Global test environment tear-down
[==] 1095 tests from 104 test cases ran. (506130 ms total)
[  PASSED  ] 1094 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 1 FAILED TEST
  YOU HAVE 231 DISABLED TESTS

6 launcher.cpp:161] Asked to destroy container 
7995f0b2-2fba-430c-98a1-abd316d3485a
W0205 08:43:13.157941 30808 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=3456 to peer '192.10.1.6:61876': IO failed with error 
code: The specified network name is no longer available.

W0205 08:43:13.157941 30808 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=3512 to peer '192.10.1.6:61877': IO failed with error 
code: The specified network name is no longer available.

I0205 08:43:13.255445 33920 containerizer.cpp:2983] Container 
7995f0b2-2fba-430c-98a1-abd316d3485a has exited
I0205 08:43:13.283437 34472 master.cpp:1109] Master terminating
I0205 08:43:13.285444 30556 hierarchical.cpp:644] Removed agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0
I0205 08:43:13.722429 30808 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 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
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/

Review Request 69892: Made SLRP recover node-published volumes after reboot.

2019-02-04 Thread Chun-Hung Hsiao 

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

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
---

If a CSI volume has been node-published before a reboot, SLRP will now
try to bring it back to node-published again. This is important to
perform synchronous persistent volume cleanup for `DESTROY`.

To achieve this, in addition to keeping track of the boot ID when a CSI
volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
`VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
the volume is node-published. This helps SLRP to better determine if a
volume has been published before reboot.


Diffs
-

  src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
  src/resource_provider/storage/provider.cpp 
d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
  src/tests/storage_local_resource_provider_tests.cpp 
e8ed20f818ed7f1a3ce15758ea3c366520443377 


Diff: https://reviews.apache.org/r/69892/diff/1/


Testing
---

`make check`

Testing for publish failures will be done later in chain.


Thanks,

Chun-Hung Hsiao