Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

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

(Updated Aug. 20, 2019, 11:26 a.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
---

Address comments from Chun


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


Repository: mesos


Description
---

Performed periodic storage local provider reconciliations.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 
69b59d48ceefebbb7accefe411c54ac5cecff1c3 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





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


Shouldn't `alwaysUpdate` be false here?



src/resource_provider/storage/provider.cpp
Line 753 (original), 820 (patched)


`resourceVersion` needs to be updated every time `UPDATE_STATE` is sent to 
avoid races. In the original implementation, since it is initialized to a 
random UUID in the constructor, we avoided assigning a new random value for the 
first `UPDATE_STATE` call.

In this new codepath, it's possible that we update the random UUID for the 
first `UPDATE_STATE` call, which is okay, just that the initial random UUID is 
wasted. If we don't care about this small overhead, I'd suggest that we move 
this line into the `sendResourceProviderStateUpdate` function.



src/resource_provider/storage/provider.cpp
Lines 760-775 (original), 827-842 (patched)


These should be be removed. L741-753 already handled the transition.



src/resource_provider/storage/provider.cpp
Line 779 (original), 846 (patched)


This should be removed. L739 resumed the SUM.


- Chun-Hung Hsiao


On Aug. 19, 2019, 11:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 19, 2019, 11:58 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Chun-Hung Hsiao]

> On Aug. 9, 2019, 7:41 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 752 (original), 811 (patched)
> > 
> >
> > As commented in the previous patch, since we're having two functions, 
> > `reconcileStoragePools` and `reconcileResources`, it's alright to have a 
> > little bit of repeated code and don't introduce an extra function without a 
> > very focused purpose.
> > 
> > In `reconcileResources`, we could have the logic that checks for 
> > `alwaysUpdate`. In `reconcileStoragePools`, since it's only called when 
> > profiles are updated or volumes of a stale profile are deleted, there's no 
> > need for the extra `alwaysUpdate` logic there.
> 
> Benjamin Bannier wrote:
> I updated the code in question. I am still not sure I understood what 
> exactly you were after, could you have a look?

Since you went for merging `reconfileStoragePools` into `reconcileResources`, 
please ignore it :)


- Chun-Hung


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


On Aug. 19, 2019, 11:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 19, 2019, 11:58 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 19, 2019, 4:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 19, 2019, 4:58 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

> On Aug. 9, 2019, 9:41 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 752 (original), 811 (patched)
> > 
> >
> > As commented in the previous patch, since we're having two functions, 
> > `reconcileStoragePools` and `reconcileResources`, it's alright to have a 
> > little bit of repeated code and don't introduce an extra function without a 
> > very focused purpose.
> > 
> > In `reconcileResources`, we could have the logic that checks for 
> > `alwaysUpdate`. In `reconcileStoragePools`, since it's only called when 
> > profiles are updated or volumes of a stale profile are deleted, there's no 
> > need for the extra `alwaysUpdate` logic there.

I updated the code in question. I am still not sure I understood what exactly 
you were after, could you have a look?


- Benjamin


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


On Aug. 19, 2019, 1:58 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 19, 2019, 1:58 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

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

(Updated Aug. 19, 2019, 1:58 p.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
---

Address issue raised by Chun


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


Repository: mesos


Description
---

Performed periodic storage local provider reconciliations.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 
69b59d48ceefebbb7accefe411c54ac5cecff1c3 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 15, 2019, 1:31 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 15, 2019, 1:31 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

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

(Updated Aug. 15, 2019, 3:31 p.m.)


Review request for mesos and Chun-Hung Hsiao.


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


Repository: mesos


Description
---

Performed periodic storage local provider reconciliations.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 
69b59d48ceefebbb7accefe411c54ac5cecff1c3 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Line 735 (original), 796 (patched)


Let's check that the `reconciled` future is pending at the very beginning 
of this function, similar to what we do in `reconcileStoragePools`, because 
`getExistingVolumes` and `getStoragePools` must be called after a 
reconciliation starts.



src/resource_provider/storage/provider.cpp
Line 752 (original), 811 (patched)


As commented in the previous patch, since we're having two functions, 
`reconcileStoragePools` and `reconcileResources`, it's alright to have a little 
bit of repeated code and don't introduce an extra function without a very 
focused purpose.

In `reconcileResources`, we could have the logic that checks for 
`alwaysUpdate`. In `reconcileStoragePools`, since it's only called when 
profiles are updated or volumes of a stale profile are deleted, there's no need 
for the extra `alwaysUpdate` logic there.



src/tests/storage_local_resource_provider_tests.cpp
Lines 6743 (patched)


These are kinda ugly. Probably should find some time to refactor them in 
the future :)



src/tests/storage_local_resource_provider_tests.cpp
Lines 6871 (patched)


Let's do a `Clock::settle()` before `Clock::advance()` to ensure that the 
timer has been set up.



src/tests/storage_local_resource_provider_tests.cpp
Lines 6881 (patched)


Ditto.


- Chun-Hung Hsiao


On Aug. 6, 2019, 2:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 6, 2019, 2:27 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 6, 2019, 2:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 6, 2019, 2:27 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

> On Aug. 2, 2019, 9:59 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 749-768 (patched)
> > 
> >
> > ```
> > loop(
> >   self(),
> >   std::bind(::after, reconciliationInterval),
> >   [this](const Nothing&) {
> > // Poll resource provider state ...
> > reconciled = sequence.add(defer(self(), ::reconcileResources));
> > 
> > return reconciled.then([](const Nothing&) -> ControlFlow {
> >   return Continue();
> > });
> >   });
> > ```

This moves the initial backoff into the function, fixed the call sites for that.


> On Aug. 2, 2019, 9:59 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 756-760 (original), 801-821 (patched)
> > 
> >
> > How about introduce a `bool alwaysUpdate` parameter in this function:
> > ```
> > bool shouldUpdate = alwaysUpdate;
> > 
> > if (result != totalResources) {
> >   LOG(INFO) << "Removing"... ;
> >   
> >   // Update the resource version...
> >   totalResources = result;
> >   resourceVersion = id::UUID::random();
> >   checkpointResourceProviderState();
> >   
> >   shouldUpdate = true;
> > }
> > 
> > if (shouldUpdate) {
> >   sendResourceProviderStateUpdate();
> > }
> > ```
> > 
> > Then, in `reconcileResourceProviderState()`:
> > ```
> >   return reconcileOperationStatuses()
> > .then(defer(self(), ::reconcileResources, true))
> > .then(defer(self(), [this] {
> >   statusUpdateManager.resume();
> >   
> >   LOG(INFO)
> > << "Resource provider " << info.id() << " is in READY state";
> > 
> >   state = READY;
> > 
> >   return Nothing();
> > }));
> > ```
> > So we don't put the state transition logic inside this function?

Great suggestion.


> On Aug. 2, 2019, 9:59 a.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 1869-1886 (original), 1933-1950 (patched)
> > 
> >
> > With periodic reconciliation, this won't be necessary. Removing this 
> > would also reduce a source of reconciliation, so the reconciliation can 
> > only happen in either `watchProfiles` or `watchResources`.

While I agree that having less points where we reconcile is better, I feel we 
shouldn't leave reconciliations like this one to user-specified configs. What 
if a user sets a `reconciliation_interval_seconds` of `0`? -- we'd still want 
to reconcile in that case.

Dropping for now, but removing mentions of MESOS-9254 from the code base. 
Please reopen if you feel strongly about this.


- Benjamin


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


On Aug. 6, 2019, 4:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 6, 2019, 4:27 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

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

(Updated Aug. 6, 2019, 4:27 p.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
---

Address comments from Chun


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


Repository: mesos


Description
---

Performed periodic storage local provider reconciliations.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 
69b59d48ceefebbb7accefe411c54ac5cecff1c3 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 5, 2019, 8:15 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated Aug. 5, 2019, 8:15 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

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

(Updated Aug. 5, 2019, 10:15 p.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
---

Rebase


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


Repository: mesos


Description
---

Performed periodic storage local provider reconciliations.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 
69b59d48ceefebbb7accefe411c54ac5cecff1c3 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Chun-Hung Hsiao]

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




src/resource_provider/storage/provider.cpp
Lines 749-768 (patched)


```
loop(
  self(),
  std::bind(::after, reconciliationInterval),
  [this](const Nothing&) {
// Poll resource provider state ...
reconciled = sequence.add(defer(self(), ::reconcileResources));

return reconciled.then([](const Nothing&) -> ControlFlow {
  return Continue();
});
  });
```



src/resource_provider/storage/provider.cpp
Lines 756-760 (original), 801-821 (patched)


How about introduce a `bool alwaysUpdate` parameter in this function:
```
bool shouldUpdate = alwaysUpdate;

if (result != totalResources) {
  LOG(INFO) << "Removing"... ;
  
  // Update the resource version...
  totalResources = result;
  resourceVersion = id::UUID::random();
  checkpointResourceProviderState();
  
  shouldUpdate = true;
}

if (shouldUpdate) {
  sendResourceProviderStateUpdate();
}
```

Then, in `reconcileResourceProviderState()`:
```
  return reconcileOperationStatuses()
.then(defer(self(), ::reconcileResources, true))
.then(defer(self(), [this] {
  statusUpdateManager.resume();
  
  LOG(INFO)
<< "Resource provider " << info.id() << " is in READY state";

  state = READY;

  return Nothing();
}));
```
So we don't put the state transition logic inside this function?



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


The delay won't guarantee the happens-before order between the first 
reconciliation and the ones in `watchResources`. How about:

```
reconciled = reconcileResourceProviderState()
  .onReady(defer(self(), ::watchProfiles))
  .onReady(defer(self(), ::watchResources))
  .onFailed(...)
  .onDiscarded(...);



src/resource_provider/storage/provider.cpp
Lines 1869-1886 (original), 1933-1950 (patched)


With periodic reconciliation, this won't be necessary. Removing this would 
also reduce a source of reconciliation, so the reconciliation can only happen 
in either `watchProfiles` or `watchResources`.


- Chun-Hung Hsiao


On July 29, 2019, 8:56 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated July 29, 2019, 8:56 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On July 29, 2019, 8:56 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated July 29, 2019, 8:56 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On July 29, 2019, 8:56 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71151/
> ---
> 
> (Updated July 29, 2019, 8:56 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9254
> https://issues.apache.org/jira/browse/MESOS-9254
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed periodic storage local provider reconciliations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71151/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Benjamin Bannier]

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

(Updated July 29, 2019, 10:56 a.m.)


Review request for mesos and Chun-Hung Hsiao.


Changes
---

Interpret a zero duration as disabled reconciliations


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


Repository: mesos


Description
---

Performed periodic storage local provider reconciliations.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
6d632606f411d3ca99d3573a57c9f68b02ba8072 
  src/tests/storage_local_resource_provider_tests.cpp 
69b59d48ceefebbb7accefe411c54ac5cecff1c3 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_71151"]

Error:
..
HAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../src   -Werror -DLIBDIR=\"/mesos/mesos-1.9.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/mesos/mesos-1.9.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/mesos/mesos-1.9.0/_inst/share/mesos\" 
-DPKGMODULEDIR=\"/mesos/mesos-1.9.0/_inst/lib/mesos/modules\" -I../../include 
-I../include -I../include/mesos -D__STDC_FORMAT_MACROS 
-I../3rdparty/boost-1.65.0 -I../3rdparty/concurrentqueue-7b69a8f 
-I../3rdparty/elfio-3.2 -I../3rdparty/glog-0.4.0/src 
-I../3rdparty/grpc-1.10.0/include -I../3rdparty/leveldb-1.19/include 
-I../3rdparty/libarchive-3.3.2/libarchive/ -I../../3rdparty/libprocess/include  
-I../3rdparty/nvml-352.79 -I../3rdparty/picojson-1.3.0 
-I../3rdparty/protobuf-3.5.0/src -I../3rdparty/rapidjson-1.1.0/include 
-I../../3rdparty/stout/include -I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated -I/usr/include/subversion-1 
-I/usr/include/apr-1 -I/usr/include/apr-1.0 -pthread -Wall -Wsig
 n-compare -Wformat-security -fstack-protector -fPIC -fPIE -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c -o 
slave/containerizer/mesos/isolators/cgroups/subsystems/libmesos_no_3rdparty_la-net_cls.lo
 `test -f 'slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp' 
|| echo 
'../../src/'`slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.9.0\" "-DPACKAGE_STRING=\"mesos 1.9.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.9.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DHAVE_LIBSASL2=1 
-DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 
-DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" 
-DMESOS_HAS_PYTHON=1 -I. -I../../src -Werror 
-DLIBDIR=\"/mesos/mesos-1.9.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/mesos/mesos-1.9.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/mesos/mesos-1.9.0/_inst/share/mesos\" -DPKGMODULED
 IR=\"/mesos/mesos-1.9.0/_inst/lib/mesos/modules\" -I../../include -I../include 
-I../include/mesos -D__STDC_FORMAT_MACROS -I../3rdparty/boost-1.65.0 
-I../3rdparty/concurrentqueue-7b69a8f -I../3rdparty/elfio-3.2 
-I../3rdparty/glog-0.4.0/src -I../3rdparty/grpc-1.10.0/include 
-I../3rdparty/leveldb-1.19/include -I../3rdparty/libarchive-3.3.2/libarchive/ 
-I../../3rdparty/libprocess/include -I../3rdparty/nvml-352.79 
-I../3rdparty/picojson-1.3.0 -I../3rdparty/protobuf-3.5.0/src 
-I../3rdparty/rapidjson-1.1.0/include -I../../3rdparty/stout/include 
-I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated -I/usr/include/subversion-1 
-I/usr/include/apr-1 -I/usr/include/apr-1.0 -pthread -Wall -Wsign-compare 
-Wformat-security -fstack-protector -fPIC -g1 -O0 -Wno-unused-local-typedefs 
-std=c++11 -c 
../../src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp  
-fPIC -DPIC -o 
slave/containerizer/mesos/isolators/cgroups/subsystems/.libs/libmesos_no_3rdp
 arty_la-net_cls.o
/bin/bash ../libtool  --tag=CXX   --mode=compile g++ -DPACKAGE_NAME=\"mesos\" 
-DPACKAGE_TARNAME=\"mesos\" -DPACKAGE_VERSION=\"1.9.0\" 
-DPACKAGE_STRING=\"mesos\ 1.9.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" 
-DPACKAGE=\"mesos\" -DVERSION=\"1.9.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 
-DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 
-DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 
-DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 
-DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 
-DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 -DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 
-DMESOS_HAS_JAVA=1 -DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 
-DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 
-DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../src   -Werror -DLIBDIR=\"/mesos/mesos-1.9.0/_inst/lib\" 

Re: Review Request 71151: Performed periodic storage local provider reconciliations.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71143, 71144, 71145, 71146, 71147, 71148, 71149, 71150, 71151]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_71151"]

Error:
..
hino-java.
Preparing to unpack .../librhino-java_1.7R4-2_all.deb ...
Unpacking librhino-java (1.7R4-2) ...
Selecting previously unselected package libsaxon-java.
Preparing to unpack .../libsaxon-java_1%3a6.5.5-10_all.deb ...
Unpacking libsaxon-java (1:6.5.5-10) ...
Selecting previously unselected package libtinfo-dev:amd64.
Preparing to unpack .../libtinfo-dev_5.9+20140118-1ubuntu1_amd64.deb ...
Unpacking libtinfo-dev:amd64 (5.9+20140118-1ubuntu1) ...
Selecting previously unselected package libtool.
Preparing to unpack .../libtool_2.4.2-1.7ubuntu1_amd64.deb ...
Unpacking libtool (2.4.2-1.7ubuntu1) ...
Selecting previously unselected package libwagon2-java.
Preparing to unpack .../libwagon2-java_2.5-1_all.deb ...
Unpacking libwagon2-java (2.5-1) ...
Selecting previously unselected package libxom-java.
Preparing to unpack .../libxom-java_1.2.10-1_all.deb ...
Unpacking libxom-java (1.2.10-1) ...
Selecting previously unselected package lksctp-tools.
Preparing to unpack .../lksctp-tools_1.0.15+dfsg-1_amd64.deb ...
Unpacking lksctp-tools (1.0.15+dfsg-1) ...
Selecting previously unselected package llvm-3.5-runtime.
Preparing to unpack .../llvm-3.5-runtime_1%3a3.5-4ubuntu2~trusty2_amd64.deb ...
Unpacking llvm-3.5-runtime (1:3.5-4ubuntu2~trusty2) ...
Selecting previously unselected package llvm-3.5.
Preparing to unpack .../llvm-3.5_1%3a3.5-4ubuntu2~trusty2_amd64.deb ...
Unpacking llvm-3.5 (1:3.5-4ubuntu2~trusty2) ...
Selecting previously unselected package libffi-dev:amd64.
Preparing to unpack .../libffi-dev_3.1~rc1+r3.0.13-12ubuntu0.2_amd64.deb ...
Unpacking libffi-dev:amd64 (3.1~rc1+r3.0.13-12ubuntu0.2) ...
Selecting previously unselected package llvm-3.5-dev.
Preparing to unpack .../llvm-3.5-dev_1%3a3.5-4ubuntu2~trusty2_amd64.deb ...
Unpacking llvm-3.5-dev (1:3.5-4ubuntu2~trusty2) ...
Selecting previously unselected package manpages-dev.
Preparing to unpack .../manpages-dev_3.54-1ubuntu1_all.deb ...
Unpacking manpages-dev (3.54-1ubuntu1) ...
Selecting previously unselected package maven.
Preparing to unpack .../archives/maven_3.0.5-1_all.deb ...
Unpacking maven (3.0.5-1) ...
Selecting previously unselected package python3-pycurl.
Preparing to unpack .../python3-pycurl_7.19.3-0ubuntu3_amd64.deb ...
Unpacking python3-pycurl (7.19.3-0ubuntu3) ...
Selecting previously unselected package unattended-upgrades.
Preparing to unpack .../unattended-upgrades_0.82.1ubuntu2.5_all.deb ...
Unpacking unattended-upgrades (0.82.1ubuntu2.5) ...
Selecting previously unselected package python3-software-properties.
Preparing to unpack .../python3-software-properties_0.92.37.8_all.deb ...
Unpacking python3-software-properties (0.92.37.8) ...
Selecting previously unselected package rhino.
Preparing to unpack .../archives/rhino_1.7R4-2_all.deb ...
Unpacking rhino (1.7R4-2) ...
Selecting previously unselected package software-properties-common.
Preparing to unpack .../software-properties-common_0.92.37.8_all.deb ...
Unpacking software-properties-common (0.92.37.8) ...
Selecting previously unselected package tcpd.
Preparing to unpack .../tcpd_7.6.q-25_amd64.deb ...
Unpacking tcpd (7.6.q-25) ...
Selecting previously unselected package testng.
Preparing to unpack .../testng_6.8.7-2_all.deb ...
Unpacking testng (6.8.7-2) ...
Processing triggers for ureadahead (0.100.0-16) ...
Processing triggers for mime-support (3.54ubuntu1.1) ...
Setting up libroken18-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libasn1-8-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libkrb5support0:amd64 (1.12+dfsg-2ubuntu5.4) ...
Setting up libk5crypto3:amd64 (1.12+dfsg-2ubuntu5.4) ...
Setting up libkeyutils1:amd64 (1.5.6-1) ...
Setting up libkrb5-3:amd64 (1.12+dfsg-2ubuntu5.4) ...
Setting up libgssapi-krb5-2:amd64 (1.12+dfsg-2ubuntu5.4) ...
Setting up libidn11:amd64 (1.28-1ubuntu2.2) ...
Setting up libhcrypto4-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libheimbase1-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libwind0-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libhx509-5-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libkrb5-26-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libheimntlm0-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up libgssapi3-heimdal:amd64 (1.6~git20131207+dfsg-1ubuntu1.2) ...
Setting up