Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

> On May 2, 2018, 4:41 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 580 (patched)
> > 
> >
> > How about doing these checks in a task so we could validate that a task 
> > could actually use the grown volume on the agent? This can be added in a 
> > follow-up patch.

Resolved here: https://reviews.apache.org/r/66920/


- Chun-Hung


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


On May 1, 2018, 10:44 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated May 1, 2018, 10:44 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/11/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

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



Just leaving the things we discussed here for a record. The issues can be 
addressed in a follow-up patch after 1.6 RC if you don't have time.


src/tests/persistent_volume_tests.cpp
Lines 580 (patched)


How about doing these checks in a task so we could validate that a task 
could actually use the grown volume on the agent? This can be added in a 
follow-up patch.



src/tests/persistent_volume_tests.cpp
Lines 721 (patched)


Ditto.


- Chun-Hung Hsiao


On May 1, 2018, 10:44 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated May 1, 2018, 10:44 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On May 1, 2018, 10:44 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated May 1, 2018, 10:44 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

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

(Updated May 1, 2018, 3:44 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Several more review comments.


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


Repository: mesos


Description
---

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


Diff: https://reviews.apache.org/r/66220/diff/10/

Changes: https://reviews.apache.org/r/66220/diff/9-10/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

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

(Updated May 1, 2018, 3:37 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 30, 2018, 5:20 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 459 (patched)
> > 
> >
> > Suggestion for the TODO: Make `MOUNT` a meaningful parameter value for 
> > this test, or create a new fixture to avoid testing against it.
> > 
> > BTW, would it be hard to do what you did for `MOUNT` in the 
> > `ShrinkVolume` test?

Yes it is harder here because there is not conceivable way for a framework to 
receive both a `MOUNT` volume and free disk resource on the same `MOUNT`, so 
framework cannot even construct such operation from offered resources.


> On April 30, 2018, 5:20 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 837 (patched)
> > 
> >
> > To make this test more succinct, how about doing
> > `{CREATE(volume), GROW_VOLUME(volume, addition), LAUNCH(task)}` so we 
> > don't need the subsequent `acceptOffers`?

I like this suggestion. will try it out.


- Zhitao


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


On April 27, 2018, 1:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 27, 2018, 1:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

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




src/tests/persistent_volume_tests.cpp
Lines 823 (patched)


If you decide to merge two `acceptOffers` calls, I suggest 
s/`offersBeforeGrow`/`offersAfterOperations`/. Otherwise the name is 
appropriate.



src/tests/persistent_volume_tests.cpp
Lines 960 (patched)


If you decide to merge two `acceptOffers` calls, I suggest 
s/`offersBeforeShrink`/`offersAfterOperations`/. Otherwise the name is 
appropriate.


- Chun-Hung Hsiao


On April 27, 2018, 8:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 27, 2018, 8:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

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




src/tests/persistent_volume_tests.cpp
Lines 788 (patched)


If you decide to merge two `acceptOffers` calls, I suggest 
s/`offerBeforeCreate`/`offersBeforeOperations`/. Otherwise, the name is 
appropriate.



src/tests/persistent_volume_tests.cpp
Lines 898 (patched)


Suggestion: How about `NonSpeculativeShrinkAndLaunch`?



src/tests/persistent_volume_tests.cpp
Lines 930 (patched)


If you decide to merge two `acceptOffers` calls, the name is appropriate. 
Otherwise I suggest s/`offersBeforeOperations`/`offersBeforeCreate`/ for 
consistency.


- Chun-Hung Hsiao


On April 27, 2018, 8:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 27, 2018, 8:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!




Overall LGTM. Thanks for your hard work!


src/tests/persistent_volume_tests.cpp
Lines 451 (patched)


`that a framework can ... and receive the grown volume`



src/tests/persistent_volume_tests.cpp
Lines 459 (patched)


Suggestion for the TODO: Make `MOUNT` a meaningful parameter value for this 
test, or create a new fixture to avoid testing against it.

BTW, would it be hard to do what you did for `MOUNT` in the `ShrinkVolume` 
test?



src/tests/persistent_volume_tests.cpp
Lines 500 (patched)


`The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 505 (patched)


`which does not`



src/tests/persistent_volume_tests.cpp
Lines 524 (patched)


`containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 539 (patched)


Could you leave the following comment here?
```
// Ensure that the allocator has updated its resources before the next 
allocation.
```



src/tests/persistent_volume_tests.cpp
Lines 555 (patched)


`Make sure the volume exists`



src/tests/persistent_volume_tests.cpp
Lines 565 (patched)


`containing the grown volume`



src/tests/persistent_volume_tests.cpp
Lines 569 (patched)


```
// Grow the volume.
```



src/tests/persistent_volume_tests.cpp
Lines 569 (patched)


`Grow the volume.`



src/tests/persistent_volume_tests.cpp
Lines 575 (patched)


Could you leave the following comment here?
```
// Ensure that the allocator has updated its resources before the next 
allocation.
```



src/tests/persistent_volume_tests.cpp
Lines 575 (patched)


Could you leave the following comment here?
```
// Ensure that the allocator has updated its resources before the next 
allocation.
```



src/tests/persistent_volume_tests.cpp
Lines 601 (patched)


`that a framework can ... see the shrunk volume`



src/tests/persistent_volume_tests.cpp
Lines 642 (patched)


`The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 666 (patched)


`containing the original volume`



src/tests/persistent_volume_tests.cpp
Lines 681 (patched)


Could you leave the following comment here?
```
// Ensure that the allocator has updated its resources before the next 
allocation.
```



src/tests/persistent_volume_tests.cpp
Lines 697 (patched)


`Make sure the volume exists`



src/tests/persistent_volume_tests.cpp
Lines 707 (patched)


`containing the shrunk volume`



src/tests/persistent_volume_tests.cpp
Lines 716 (patched)


Could you leave the following comment here?
```
// Ensure that the allocator has updated its resources before the next 
allocation.
```



src/tests/persistent_volume_tests.cpp
Lines 752 (patched)


Remove "by".
Suggestion: that a subsequent `LAUNCH` depending on a grown volume will be 
dropped



src/tests/persistent_volume_tests.cpp
Lines 754 (patched)


Suggestion: How about `NonSpeculativeGrowAndLaunch`?



src/tests/persistent_volume_tests.cpp
Lines 760 (patched)


Suggestion for the TODO: Make `MOUNT` a meaningful parameter value for this 
test, or create a new fixture to avoid testing against it.



src/tests/persistent_volume_tests.cpp
Lines 801 (patched)


`The disk spaces ... if the fixture parameter`



src/tests/persistent_volume_tests.cpp
Lines 806 (patched)


`which does not`



src/tests/persistent_volume_tests.cpp
Lines 825 (patched)

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 25, 2018, 9:09 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 451 (patched)
> > 
> >
> > It's not 100% clear to me if these tests actually verify that the 
> > agent's resource checkpointing has occurred? IIUC, settling the clock after 
> > awaiting on the `ApplyOperationMessage` which applies the GROW or SHRINK 
> > may be sufficient to verify this?

They don't. I've rewrittent the comment.


- Zhitao


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


On April 27, 2018, 1:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 27, 2018, 1:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 25, 2018, 4:59 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 501-505 (patched)
> > 
> >
> > ```
> > // Disk spaces will be merged if fixture parameter is `NONE`.
> > Bytes totalBytes = GetParam() == NONE ? MegaBytes(4096) : 
> > MegaBytes(2048);
> > ```
> > 
> > TBH I'm not a fan of conditioning on the fixture parameters. If a new 
> > fixture is created, this can be resolved by having only one disk in 
> > `getSlaveResources()` instead. But given the fact that we also do similar 
> > conditional branches in other tests, I can live with this.

I'll leave this as a TODO.


> On April 25, 2018, 4:59 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 823 (patched)
> > 
> >
> > This is not required.

New test has offers so this will be necessary.


> On April 25, 2018, 4:59 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 853-855 (patched)
> > 
> >
> > This is essentially testing the same property as the last 
> > `EXPECT_FALSE` so we don't need it.

New test dropped this.


- Zhitao


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


On April 27, 2018, 1:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 27, 2018, 1:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 455-459 (patched)
> > 
> >
> > Is this enforced somewhere in validation code? Can we check for 
> > expected behavior when a GROW/SHRINK operation is submitted for a MOUNT 
> > volume, rather than simply returning?
> 
> Zhitao Li wrote:
> I added validation in r/66050 so we drop shrink volume operation on 
> MOUNT. There is no logical path for triggering `GROW` on `MOUNT` disk type so 
> I'm not writing validation.
> 
> For testing, we can either keep the current structure and check that 
> `GROW`/`SHRINK` do not work on `MOUNT` (operation will be dropped), or take 
> Chun's suggestion to not test them for `MOUNT`. Please let me know.
> 
> Zhitao Li wrote:
> I think this will be the only test case which needs to skip `MOUNT`. It 
> also makes some sense because there is no logical starting point for a 
> framework to even construct a `GROW_VOLUME` message for `MOUNT`.
> 
> I still feel that the handling here could be better. we can discuss on 
> next patch revision.
> 
> Chun-Hung Hsiao wrote:
> I'm sorry for missing this reply. If we want to just skip `MOUNT`, I 
> prefer having a new fixture as a subclass of the original and make it only 
> parameterized against `NONE` and `PATH`; otherwise, how about doing proper 
> checks in `offersAfterGrow` based on the parameter:
> ```
> if (GetParam() == MOUNT) {
>   EXPECT_EQ(
>   allocatedResources(Resources(volume), frameworkInfo.roles(0)),
>   Resources(offer.resources()).persistentVolumes());
> } else {
>   EXPECT_TRUE(os::exists(volumePath));
>   EXPECT_SOME_EQ("abc", os::read(filePath));
> 
>   EXPECT_EQ(
>   allocatedResources(Resources(grownVolume), frameworkInfo.roles(0)),
>   Resources(offer.resources()).persistentVolumes());
> 
>   EXPECT_FALSE(
>   Resources(offer.resources()).contains(
>   allocatedResources(addition, frameworkInfo.roles(0;
> }
> ```
> My reason is that when seeing 
> `ROOT_MountDiskResource/PersistentVolumeTest.GrowVolume/0` passes really 
> means something is tested.

I'm going to leave this as a TODO for future improvement.


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 541-542 (patched)
> > 
> >
> > Is this `Future` necessary? Since the task consumes the volume, it may 
> > be sufficient to await on the task status updates?
> 
> Zhitao Li wrote:
> Yes this is necessary if we do not launch task in this test. we need to 
> reliably know that `Allocator::updateAllocation` is called from master, and 
> this message happens before that, so this `Future` ensures allocator has 
> properly processed all operation conversions and next offer will contain the 
> host and updated resources.
> 
> Chun-Hung Hsiao wrote:
> I see. Here you're relying on the fact that when the 
> `ApplyOperationMessage` is processed on the slave, the master has already 
> speculatively applied the operation called `updatedAllocation`. However 
> `updateAllocation` is asynchronous so there is still a potential race between 
> `ApplyOperationMessage` and the actual allocation update.
> 
> I suggest instead of waiting for the internal message, let's do a 
> `Clock::settle();` before `Clock::advance(...)`. I just tested it locally.
> 
> Ditto for all the following awaits for `ApplyOperationMessage`.

`Clock::settle()` is not sufficient. What we need here is not confirming on 
agent processing, but confirming master has sent this to agent, because we 
reliably know that master has also called `allocatior::updateAllocation()` 
(which happens before this call). Otherwise allocator may not have correct view 
of resources.

Unfornately master -> allocator communication cannot be intecepted by 
`FUTURE...` so this is the closest thing I can find.


- Zhitao


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


On April 27, 2018, 1:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 27, 2018, 1:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/t

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

> On April 25, 2018, 9:09 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 585-586 (patched)
> > 
> >
> > IIUC, the `Clock::settle()` here is ensuring that this test actually 
> > verifies that the agent's resources have been checkpointed?
> > 
> > If so, I would recommend:
> > 1) Add a comment explaining the reason for the settle.
> > 2) Place the settle after the await. I think the two orderings are 
> > equivalent, but I think it's more readable if the settle is after the await?
> 
> Chun-Hung Hsiao wrote:
> The await can be removed. See my comments above.

This is necessary to make sure that master -> allocator communication for 
`allocator::updateAllocation()` has happened.


- Zhitao


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


On April 27, 2018, 1:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 27, 2018, 1:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

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

(Updated April 27, 2018, 1:04 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


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


Repository: mesos


Description
---

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

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




src/tests/persistent_volume_tests.cpp
Lines 501-505 (patched)


```
// Disk spaces will be merged if fixture parameter is `NONE`.
Bytes totalBytes = GetParam() == NONE ? MegaBytes(4096) : MegaBytes(2048);
```

TBH I'm not a fan of conditioning on the fixture parameters. If a new 
fixture is created, this can be resolved by having only one disk in 
`getSlaveResources()` instead. But given the fact that we also do similar 
conditional branches in other tests, I can live with this.



src/tests/persistent_volume_tests.cpp
Lines 525-528 (patched)


Move this just before `acceptOffers` to make it close to its use.



src/tests/persistent_volume_tests.cpp
Lines 525-528 (patched)


Move this just before `acceptOffers` to make it close to its use.



src/tests/persistent_volume_tests.cpp
Lines 554-560 (patched)


How about merging these two into
```
EXPECT_TRUE(Resources(offer.resources()).contains(
allocatedResources(Resources(volume) + addition, 
frameworkInfo.roles(0;
```

Nit for indentation: We indent for 4 more spaces when we open a parenthesis.



src/tests/persistent_volume_tests.cpp
Lines 554-560 (patched)


How about merging these two into the following, so the intention of this 
check is clear: verifying that the offer contains the resources we are going to 
use in the next ACCEPT call?
```
EXPECT_TRUE(Resources(offer.resources()).contains(
allocatedResources(Resources(volume) + addition, 
frameworkInfo.roles(0;
```

Nit for indentation: We indent for 4 more spaces when we open a parenthesis.



src/tests/persistent_volume_tests.cpp
Lines 654-660 (patched)


```
// Disk spaces will be merged if fixture parameter is `NONE`.
Bytes totalBytes = GetParam() == NONE ? Megabytes(4096) : Megabytes(2048);
```



src/tests/persistent_volume_tests.cpp
Lines 725-733 (patched)


We don't need this if we just use `Clock::settle()` later for 
synchronization, as I suggested.



src/tests/persistent_volume_tests.cpp
Lines 745-748 (patched)


We could replace this with just `Clock::settle()`.



src/tests/persistent_volume_tests.cpp
Lines 823 (patched)


This is not required.



src/tests/persistent_volume_tests.cpp
Lines 853-855 (patched)


This is essentially testing the same property as the last `EXPECT_FALSE` so 
we don't need it.



src/tests/persistent_volume_tests.cpp
Lines 865 (patched)


```
// ... a `GROW_VOLUME` and ...
```



src/tests/persistent_volume_tests.cpp
Lines 888 (patched)


```
// Expect that the offer does not contain ...
```

Please drop this if I'm wrong.



src/tests/persistent_volume_tests.cpp
Lines 889-890 (patched)


```
EXPECT_TRUE(Resources(offer.resources()).persistentVolumes().empty());
```



src/tests/persistent_volume_tests.cpp
Lines 966-968 (patched)


No need for this because of the last `EXPECT_FALSE`.



src/tests/persistent_volume_tests.cpp
Lines 978 (patched)


```
// a `SHRINK_VOLUME` and ...
```



src/tests/persistent_volume_tests.cpp
Lines 1001 (patched)


```
// Expect that the offer does not contain ...
```



src/tests/persistent_volume_tests.cpp
Lines 1002-1003 (patched)


```
EXPECT_TRUE(Resources(offer.resources()).persistentVolumes().empty());
```


- Chun-Hung Hsiao


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> D

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

> On April 25, 2018, 4:09 p.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 585-586 (patched)
> > 
> >
> > IIUC, the `Clock::settle()` here is ensuring that this test actually 
> > verifies that the agent's resources have been checkpointed?
> > 
> > If so, I would recommend:
> > 1) Add a comment explaining the reason for the settle.
> > 2) Place the settle after the await. I think the two orderings are 
> > equivalent, but I think it's more readable if the settle is after the await?

The await can be removed. See my comments above.


- Chun-Hung


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


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

> On April 13, 2018, 4:43 p.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 455-459 (patched)
> > 
> >
> > Is this enforced somewhere in validation code? Can we check for 
> > expected behavior when a GROW/SHRINK operation is submitted for a MOUNT 
> > volume, rather than simply returning?
> 
> Zhitao Li wrote:
> I added validation in r/66050 so we drop shrink volume operation on 
> MOUNT. There is no logical path for triggering `GROW` on `MOUNT` disk type so 
> I'm not writing validation.
> 
> For testing, we can either keep the current structure and check that 
> `GROW`/`SHRINK` do not work on `MOUNT` (operation will be dropped), or take 
> Chun's suggestion to not test them for `MOUNT`. Please let me know.
> 
> Zhitao Li wrote:
> I think this will be the only test case which needs to skip `MOUNT`. It 
> also makes some sense because there is no logical starting point for a 
> framework to even construct a `GROW_VOLUME` message for `MOUNT`.
> 
> I still feel that the handling here could be better. we can discuss on 
> next patch revision.

I'm sorry for missing this reply. If we want to just skip `MOUNT`, I prefer 
having a new fixture as a subclass of the original and make it only 
parameterized against `NONE` and `PATH`; otherwise, how about doing proper 
checks in `offersAfterGrow` based on the parameter:
```
if (GetParam() == MOUNT) {
  EXPECT_EQ(
  allocatedResources(Resources(volume), frameworkInfo.roles(0)),
  Resources(offer.resources()).persistentVolumes());
} else {
  EXPECT_TRUE(os::exists(volumePath));
  EXPECT_SOME_EQ("abc", os::read(filePath));

  EXPECT_EQ(
  allocatedResources(Resources(grownVolume), frameworkInfo.roles(0)),
  Resources(offer.resources()).persistentVolumes());

  EXPECT_FALSE(
  Resources(offer.resources()).contains(
  allocatedResources(addition, frameworkInfo.roles(0;
}
```
My reason is that when seeing 
`ROOT_MountDiskResource/PersistentVolumeTest.GrowVolume/0` passes really means 
something is tested.


> On April 13, 2018, 4:43 p.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 541-542 (patched)
> > 
> >
> > Is this `Future` necessary? Since the task consumes the volume, it may 
> > be sufficient to await on the task status updates?
> 
> Zhitao Li wrote:
> Yes this is necessary if we do not launch task in this test. we need to 
> reliably know that `Allocator::updateAllocation` is called from master, and 
> this message happens before that, so this `Future` ensures allocator has 
> properly processed all operation conversions and next offer will contain the 
> host and updated resources.

I see. Here you're relying on the fact that when the `ApplyOperationMessage` is 
processed on the slave, the master has already speculatively applied the 
operation called `updatedAllocation`. However `updateAllocation` is 
asynchronous so there is still a potential race between `ApplyOperationMessage` 
and the actual allocation update.

I suggest instead of waiting for the internal message, let's do a 
`Clock::settle();` before `Clock::advance(...)`. I just tested it locally.

Ditto for all the following awaits for `ApplyOperationMessage`.


- Chun-Hung


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


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

> On April 17, 2018, 10:52 p.m., Gaston Kleiman wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 502 (patched)
> > 
> >
> > Nit: `Offer offer = offersBeforeCreate->at(0);`
> > 
> > If this test is split into smaller ones, then you can also probable 
> > make `offer` a const ref.
> 
> Zhitao Li wrote:
> We need to obtain offer at least twice.

s/`.get()[0]`/`->at(0)`/ here and below. Reopen this since this is missed.


- Chun-Hung


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


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Greg Mann]

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




src/tests/persistent_volume_tests.cpp
Lines 451 (patched)


It's not 100% clear to me if these tests actually verify that the agent's 
resource checkpointing has occurred? IIUC, settling the clock after awaiting on 
the `ApplyOperationMessage` which applies the GROW or SHRINK may be sufficient 
to verify this?



src/tests/persistent_volume_tests.cpp
Lines 530-531 (patched)


Indented too far.



src/tests/persistent_volume_tests.cpp
Lines 539 (patched)


This accept call doesn't launch a task.



src/tests/persistent_volume_tests.cpp
Lines 552 (patched)


You could do `offersBeforeGrow->at(0)`, which I prefer. WDYT?

Here and below.



src/tests/persistent_volume_tests.cpp
Lines 560 (patched)


Indent four more spaces.



src/tests/persistent_volume_tests.cpp
Lines 562 (patched)


s/leaves/leave/



src/tests/persistent_volume_tests.cpp
Lines 571 (patched)


Indented too far.



src/tests/persistent_volume_tests.cpp
Lines 575 (patched)


This offer won't contain the original volume, right?



src/tests/persistent_volume_tests.cpp
Lines 585-586 (patched)


IIUC, the `Clock::settle()` here is ensuring that this test actually 
verifies that the agent's resources have been checkpointed?

If so, I would recommend:
1) Add a comment explaining the reason for the settle.
2) Place the settle after the await. I think the two orderings are 
equivalent, but I think it's more readable if the settle is after the await?



src/tests/persistent_volume_tests.cpp
Lines 744-747 (patched)


Do we need a settle after this to ensure that the agent has checkpointed 
its resources?


- Greg Mann


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Chun-Hung Hsiao]

> On April 17, 2018, 10:52 p.m., Gaston Kleiman wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 474-475 (patched)
> > 
> >
> > I see that Benjamin recently added this line and the corresponding 
> > `AWAIT_READY` to many tests.
> > 
> > However I don't think it is necessary here; I'll check with Benjamin to 
> > see if we can also remove it from other tests in which it is not necessary.
> 
> Zhitao Li wrote:
> I'd like to keep this (see reasons above) because I've seen race 
> condition on offer otherwise. We can remove with other `UpdateSlaveMessage` 
> in this test all together.

IMO we should be careful and only add necessary synchronizations so the test 
can help us to capture unexpecetd races.


- Chun-Hung


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


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 66049.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66049`

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

Relevant logs:

- 
[apply-review-66049-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66220/logs/apply-review-66049-stdout.log):

```
error: patch failed: include/mesos/mesos.proto:1917
error: include/mesos/mesos.proto: patch does not apply
error: patch failed: include/mesos/v1/mesos.proto:1909
error: include/mesos/v1/mesos.proto: patch does not apply
```

- Mesos Reviewbot Windows


On April 19, 2018, 4:39 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 19, 2018, 4:39 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

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

(Updated April 19, 2018, 9:39 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Remove unnecessary `Ignore subsequent offers.`


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


Repository: mesos


Description
---

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.

[Zhitao Li]

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

(Updated April 19, 2018, 9:30 a.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Review comments.


Summary (updated)
-

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


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


Repository: mesos


Description (updated)
---

Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


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

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


Testing
---


Thanks,

Zhitao Li