Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated April 24, 2018, 1:06 a.m.)


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


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.


Diffs (updated)
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
8ca2d3a98858940e6e027becefb53c2f00b4ae43 


Diff: https://reviews.apache.org/r/65594/diff/9/

Changes: https://reviews.apache.org/r/65594/diff/8-9/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3276 (patched)
> > 
> >
> > Should we capture this?
> > 
> > ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];
> > 
> > No strong opinion, it seems hard to make interacting with proto maps 
> > nice.
> 
> Chun-Hung Hsiao wrote:
> I'll do `ProfileInfo& profileInfo_ = ...`.
> 
> Chun-Hung Hsiao wrote:
> BTW the deference operator (`*`) applies on `mutable_profiles()` not `[]`.

Typo in the above comment: `s/mutable_profiles/mutable_capability/`.


- Chun-Hung


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


On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated April 12, 2018, 3:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3276 (patched)
> > 
> >
> > Should we capture this?
> > 
> > ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];
> > 
> > No strong opinion, it seems hard to make interacting with proto maps 
> > nice.
> 
> Chun-Hung Hsiao wrote:
> I'll do `ProfileInfo& profileInfo_ = ...`.

BTW the deference operator (`*`) applies on `mutable_profiles()` not `[]`.


- Chun-Hung


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


On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated April 12, 2018, 3:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 1003 (patched)
> > 
> >
> > Even thought an `auto` might make sense in such a place _in general_, 
> > due to the interface of `MapInfo` it leads to pretty dense code and maybe 
> > repeating a type here might make this easier to read. How about
> > 
> > // NOTE: this form requires a `using` decl above.
> > using Profile =
> >   MapPair;
> >   
> > foreach (const Profile& profile, storage.profiles()) {
> >   profileInfos.put(
> >   profile.first,
> >   {profile.second.capability(), profile.second.parameters()});
> > }

Adopted your suggestion, but I use `ProfileEntry` instead of `Profile` and keep 
the variable name `entry`. I'd like to make it consistent across the code that 
`profile` indicates a name, `profileInfo` indicates the CSI data, and `entry` 
indicates a map pair.


> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1010 (original), 1011 (patched)
> > 
> >
> > Nit: _pending operations_ should be specific enough as this list is 
> > neither exhaustive today nor will remain in the future.
> 
> Chun-Hung Hsiao wrote:
> I'm specifically talking about `CREATE_VOLUME` and `CREATE_BLOCK` here. 
> `DESTROY_VOLUME` and `DESTROY_BLOCK` don't need the profile info.

Removed them from the comments as suggested as the explicitness here doesn't 
bring much more clarity.


> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3276 (patched)
> > 
> >
> > Should we capture this?
> > 
> > ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];
> > 
> > No strong opinion, it seems hard to make interacting with proto maps 
> > nice.

I'll do `ProfileInfo& profileInfo_ = ...`.


- Chun-Hung


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


On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated April 12, 2018, 3:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1010 (original), 1011 (patched)
> > 
> >
> > Nit: _pending operations_ should be specific enough as this list is 
> > neither exhaustive today nor will remain in the future.

I'm specifically talking about `CREATE_VOLUME` and `CREATE_BLOCK` here. 
`DESTROY_VOLUME` and `DESTROY_BLOCK` don't need the profile info.


> On April 20, 2018, 12:57 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1021 (original), 1014 (patched)
> > 
> >
> > Both `disk` and `profile` are `optional` fields. We should check 
> > explicitly whether they are set (otherwise we might at some point e.g., hit 
> > a failure where the profile `` (empty string) was not found.

The implicit invariant here is that each resource either has it's id or profile 
set. I'll add a check to make it more explicit.


- Chun-Hung


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


On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated April 12, 2018, 3:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Benjamin Bannier]

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




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


Even thought an `auto` might make sense in such a place _in general_, due 
to the interface of `MapInfo` it leads to pretty dense code and maybe repeating 
a type here might make this easier to read. How about

// NOTE: this form requires a `using` decl above.
using Profile =
  MapPair;
  
foreach (const Profile& profile, storage.profiles()) {
  profileInfos.put(
  profile.first,
  {profile.second.capability(), profile.second.parameters()});
}



src/resource_provider/storage/provider.cpp
Line 1010 (original), 1011 (patched)


Nit: _pending operations_ should be specific enough as this list is neither 
exhaustive today nor will remain in the future.



src/resource_provider/storage/provider.cpp
Line 1020 (original), 1013 (patched)


Let's add a comment pointing out that this matches what is done and 
documented in some more detail in `checkpointResourceProviderState`.



src/resource_provider/storage/provider.cpp
Line 1021 (original), 1014 (patched)


Both `disk` and `profile` are `optional` fields. We should check explicitly 
whether they are set (otherwise we might at some point e.g., hit a failure 
where the profile `` (empty string) was not found.



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


This invariant seems to be pretty non-local :(



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


Should we capture this?

ProfileInfo& profile_ = *(storage->mutable_profiles())[profile];

No strong opinion, it seems hard to make interacting with proto maps nice.


- Benjamin Bannier


On April 12, 2018, 5:35 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated April 12, 2018, 5:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 12, 2018, 3:35 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated April 12, 2018, 3:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> a07620d1c4bf618f669575b3e183bf598da905a6 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated April 12, 2018, 3:35 a.m.)


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


Changes
---

Rebased and adopted CSI v0.2.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.


Diffs (updated)
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
a07620d1c4bf618f669575b3e183bf598da905a6 


Diff: https://reviews.apache.org/r/65594/diff/8/

Changes: https://reviews.apache.org/r/65594/diff/7-8/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated March 20, 2018, 3:13 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.


Diffs (updated)
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
bb19ed4b6b1b8f5f6b327461a737517497c8c38e 


Diff: https://reviews.apache.org/r/65594/diff/7/

Changes: https://reviews.apache.org/r/65594/diff/6-7/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated March 9, 2018, 1:38 a.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.


Diffs (updated)
-

  3rdparty/Makefile.am f4cb731198e271b967b920235ed2785e9296f9e1 
  src/Makefile.am 84753794eca822ab251200cccb907b328c849fb7 
  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
bb19ed4b6b1b8f5f6b327461a737517497c8c38e 


Diff: https://reviews.apache.org/r/65594/diff/6/

Changes: https://reviews.apache.org/r/65594/diff/5-6/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On Feb. 19, 2018, 3:39 p.m., Benjamin Bannier wrote:
> > src/resource_provider/state.proto
> > Lines 19 (patched)
> > 
> >
> > This file is not present for e.g., cmake builds or more generally when 
> > not building with CSI support.

This is fixed for autotools builds. I'll file a follow-up patch for cmake 
builds.


- Chun-Hung


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


On March 9, 2018, 1:29 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated March 9, 2018, 1:29 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am f4cb731198e271b967b920235ed2785e9296f9e1 
>   src/Makefile.am 84753794eca822ab251200cccb907b328c849fb7 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> 63dde512fd8cc9f68f5f48a96869eb09b23b6f4a 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated March 9, 2018, 1:29 a.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


Changes
---

Fixed a compilation problem when gRPC is disabled.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.


Diffs (updated)
-

  3rdparty/Makefile.am f4cb731198e271b967b920235ed2785e9296f9e1 
  src/Makefile.am 84753794eca822ab251200cccb907b328c849fb7 
  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
63dde512fd8cc9f68f5f48a96869eb09b23b6f4a 


Diff: https://reviews.apache.org/r/65594/diff/5/

Changes: https://reviews.apache.org/r/65594/diff/4-5/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1304 (original), 1329 (patched)
> > 
> >
> > This flag is a bit confusing. When we will set this flag to `true`? Do 
> > we need to set it to true when `reconcileStoragePools` is called 
> > (currently, it's not)?
> > 
> > What if `GetCapacity` reports shrinked capacity? Should we drop 
> > `CreateVolume` in that case?

Refactored in a followup patch: https://reviews.apache.org/r/65975/

It is fine for `GetCapacity` to shrink the resources since will wait for all 
pending `CreateVolume` to complete before calling `GetCapacity`.


> On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2968-2969 (patched)
> > 
> >
> > Do you need a dispatch here?

Removed in https://reviews.apache.org/r/65975/.


> On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2970-2971 (patched)
> > 
> >
> > This is a bit confusing to me initially, and a bit hard to follow.
> > 
> > I think the goal here is trying to prevent concurrent reconciliation. 
> > However, currently in the code, we used two ways to enforce that:
> > 1) the loop for detecting profile changes (to prevent two 
> > reconcileProfiles run concurrently
> > 2) operationSequence to prevent reconcileProfiles and 
> > reconcileStoragePool run concurrently
> > 
> > Maybe we should use a unified way.

Refactored in https://reviews.apache.org/r/65975/.


> On Feb. 24, 2018, 12:11 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3087 (patched)
> > 
> >
> > Do you want to add some CHECK here to check resource `has_disk`, 
> > `has_source` and `has_profile`?

I think `CHECK(resource.disk().source().has_profile())` is sufficient and 
concise since if either `disk` or `source` is missing, we will get an empty 
`Resource::DiskInfo::Source`.


- Chun-Hung


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


On March 8, 2018, 5:04 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated March 8, 2018, 5:04 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and does
> not depend on the `DiskProfileAdaptor` module to return the set of
> previously-known profiles during recovery.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> 63dde512fd8cc9f68f5f48a96869eb09b23b6f4a 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated March 8, 2018, 5:04 a.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


Changes
---

Refactored and addressed Jie's comments. Will address Ben's comments tomorrow.


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


Repository: mesos


Description (updated)
---

SLRP now checkpoints profiles associated with storage pools, and does
not depend on the `DiskProfileAdaptor` module to return the set of
previously-known profiles during recovery.


Diffs (updated)
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
63dde512fd8cc9f68f5f48a96869eb09b23b6f4a 


Diff: https://reviews.apache.org/r/65594/diff/4/

Changes: https://reviews.apache.org/r/65594/diff/3-4/


Testing (updated)
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Jie Yu]

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




src/resource_provider/storage/provider.cpp
Line 1304 (original), 1329 (patched)


This flag is a bit confusing. When we will set this flag to `true`? Do we 
need to set it to true when `reconcileStoragePools` is called (currently, it's 
not)?

What if `GetCapacity` reports shrinked capacity? Should we drop 
`CreateVolume` in that case?



src/resource_provider/storage/provider.cpp
Lines 2968-2969 (patched)


Do you need a dispatch here?



src/resource_provider/storage/provider.cpp
Lines 2970-2971 (patched)


This is a bit confusing to me initially, and a bit hard to follow.

I think the goal here is trying to prevent concurrent reconciliation. 
However, currently in the code, we used two ways to enforce that:
1) the loop for detecting profile changes (to prevent two reconcileProfiles 
run concurrently
2) operationSequence to prevent reconcileProfiles and reconcileStoragePool 
run concurrently

Maybe we should use a unified way.



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


Do you want to add some CHECK here to check resource `has_disk`, 
`has_source` and `has_profile`?


- Jie Yu


On Feb. 13, 2018, 10:18 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated Feb. 13, 2018, 10:18 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and no
> longer assumes that profiles won't be removed by the disk profile
> adaptor. It will reject volume/block creation requests that use
> missing profiles, but existing volumes/blocks will continue to work.
> When a volume/block with a missing profiles is destroyed, it will be
> converted to an empty resource, and SLRP will update the sizes of
> storage pools for currently known profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> 33abc0e05a804969ae14da9cb9c58698ba1aaea5 
>   src/resource_provider/storage/uri_disk_profile.cpp 
> 665798fdb085ea34f93bd287fe6f9ab29a265cbf 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> NOTE: This patch does not change the URI disk profile daptor to allow missing 
> profiles from upstream.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Benjamin Bannier]

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




src/resource_provider/state.proto
Lines 19 (patched)


This file is not present for e.g., cmake builds or more generally when not 
building with CSI support.


- Benjamin Bannier


On Feb. 13, 2018, 11:18 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated Feb. 13, 2018, 11:18 p.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and no
> longer assumes that profiles won't be removed by the disk profile
> adaptor. It will reject volume/block creation requests that use
> missing profiles, but existing volumes/blocks will continue to work.
> When a volume/block with a missing profiles is destroyed, it will be
> converted to an empty resource, and SLRP will update the sizes of
> storage pools for currently known profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> 33abc0e05a804969ae14da9cb9c58698ba1aaea5 
>   src/resource_provider/storage/uri_disk_profile.cpp 
> 665798fdb085ea34f93bd287fe6f9ab29a265cbf 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> NOTE: This patch does not change the URI disk profile daptor to allow missing 
> profiles from upstream.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated Feb. 13, 2018, 10:18 p.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


Changes
---

Reverted to revision 1 to remove the best-effort non-decreasing watch logic.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and no
longer assumes that profiles won't be removed by the disk profile
adaptor. It will reject volume/block creation requests that use
missing profiles, but existing volumes/blocks will continue to work.
When a volume/block with a missing profiles is destroyed, it will be
converted to an empty resource, and SLRP will update the sizes of
storage pools for currently known profiles.


Diffs (updated)
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
33abc0e05a804969ae14da9cb9c58698ba1aaea5 
  src/resource_provider/storage/uri_disk_profile.cpp 
665798fdb085ea34f93bd287fe6f9ab29a265cbf 


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

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


Testing
---

sudo make check

NOTE: This patch does not change the URI disk profile daptor to allow missing 
profiles from upstream. Will have another patch to address this.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated Feb. 12, 2018, 7:07 p.m.)


Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and no
longer assumes that profiles won't be removed by the disk profile
adaptor. It will reject volume/block creation requests that use
missing profiles, but existing volumes/blocks will continue to work.
When a volume/block with a missing profiles is destroyed, it will be
converted to an empty resource, and SLRP will update the sizes of
storage pools for currently known profiles.


Diffs (updated)
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
33abc0e05a804969ae14da9cb9c58698ba1aaea5 


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

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


Testing (updated)
---

sudo make check

NOTE: This patch does not change the URI disk profile daptor to allow missing 
profiles from upstream. Will have another patch to address this.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

> On Feb. 10, 2018, 2:22 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/uri_disk_profile.cpp
> > Line 248 (original), 242 (patched)
> > 
> >
> > This should be removed to support missing profiles from upstream.

Will address this in a follow-up patch.


- Chun-Hung


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


On Feb. 10, 2018, 12:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated Feb. 10, 2018, 12:47 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and no
> longer assumes that profiles won't be removed by the disk profile
> adaptor. It will reject volume/block creation requests that use
> missing profiles, but existing volumes/blocks will continue to work.
> When a volume/block with a missing profiles is destroyed, it will be
> converted to an empty resource, and SLRP will update the sizes of
> storage pools for currently known profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> 33abc0e05a804969ae14da9cb9c58698ba1aaea5 
>   src/resource_provider/storage/uri_disk_profile.cpp 
> 665798fdb085ea34f93bd287fe6f9ab29a265cbf 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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




src/resource_provider/storage/uri_disk_profile.cpp
Line 248 (original), 242 (patched)


This should be removed to support missing profiles from upstream.


- Chun-Hung Hsiao


On Feb. 10, 2018, 12:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65594/
> ---
> 
> (Updated Feb. 10, 2018, 12:47 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8492
> https://issues.apache.org/jira/browse/MESOS-8492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now checkpoints profiles associated with storage pools, and no
> longer assumes that profiles won't be removed by the disk profile
> adaptor. It will reject volume/block creation requests that use
> missing profiles, but existing volumes/blocks will continue to work.
> When a volume/block with a missing profiles is destroyed, it will be
> converted to an empty resource, and SLRP will update the sizes of
> storage pools for currently known profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
>   src/resource_provider/storage/provider.cpp 
> 33abc0e05a804969ae14da9cb9c58698ba1aaea5 
>   src/resource_provider/storage/uri_disk_profile.cpp 
> 665798fdb085ea34f93bd287fe6f9ab29a265cbf 
> 
> 
> Diff: https://reviews.apache.org/r/65594/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65594: Checkpointed profiles in storage local resource provider.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['65594']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65594

Relevant logs:

- 
[mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65594/logs/mesos-tests-cmake-stdout.log):

```
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(2601):
 warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3091):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3426):
 warning C4996: 'strerror': This function or variable may be unsafe. Consider 
using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 

Review Request 65594: Checkpointed profiles in storage local resource provider.

[Chun-Hung Hsiao]

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

Review request for mesos, James DeFelice, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

SLRP now checkpoints profiles associated with storage pools, and no
longer assumes that profiles won't be removed by the disk profile
adaptor. It will reject volume/block creation requests that use
missing profiles, but existing volumes/blocks will continue to work.
When a volume/block with a missing profiles is destroyed, it will be
converted to an empty resource, and SLRP will update the sizes of
storage pools for currently known profiles.


Diffs
-

  src/resource_provider/state.proto 8577b58b8cdb63b3daddf06ea5d12f80f9d42f2b 
  src/resource_provider/storage/provider.cpp 
33abc0e05a804969ae14da9cb9c58698ba1aaea5 
  src/resource_provider/storage/uri_disk_profile.cpp 
665798fdb085ea34f93bd287fe6f9ab29a265cbf 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao