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

[Zhitao Li]

> On April 17, 2018, 3: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.

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.


> On April 17, 2018, 3: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.

We need to obtain offer at least twice.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Zhitao Li]

> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 455-459 (patched)
> > 
> >
> > Or alternatively, we can do the following:
> > ```
> > class PersistentVolumeResizeTest : public PersistentVolumeTest {};
> > 
> > INSTANTIATE_TEST_CASE_P(
> > DiskResource, /* Not sure if the same name can be used twice */
> > PersistentVolumeResizeTest,
> > ::testing::Values(
> > PersistentVolumeSourceType::NONE,
> > PersistentVolumeSourceType::PATH));
> > 
> > TEST_P(PersistentVolumeResizeTest, GrowAndShrink)
> > {
> >   ...
> > }
> > ```
> > And remove the conditional return.

I feel keeping current form is easier to reason, and the condition upon `MOUNT` 
is reduced to only one test case.


> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 650-655 (patched)
> > 
> >
> > Since the test checks if the subsequent offer contains the shrunk 
> > volume, there is no need to check the internal `ApplyOperationMessage`.

See comments above: this is used to eliminate race condition between master and 
allocator.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Zhitao Li]

> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 453 (patched)
> > 
> >
> > If I'm not mistaken, this test does the following steps:
> > 
> > 1. Create a persistent volume
> > 2. Launch a task to write a file in the volume
> > 3. Grow the volume
> > 4. Check if the file exists in the volume
> > 5. Shrink the volume
> > 6. Check if the file exists in the volume
> > 
> > The majority of this test is to implement Step 1 and 2, which are 
> > already testsed in `AccessPersistentVolume` and not really relevent to the 
> > growing and shrinking functions. How about splitting this test into two 
> > tests, `GrowPersistentVolume` and `ShrinkPersistentVolume`, where the first 
> > one only does Step 1, 3, 4 and the second only does Step 1, 5, 6? The 
> > creation of the file can be done with a simple `os::write()` to the volume 
> > path instead of using a task.
> > 
> > Also, I'm not sure if the file check is really necessary. If not, we 
> > can just check the existence of the volume path in Step 4 and 6. 
> > Furthermore, we could even choose not to check the existence of the volume 
> > path. In this case, we could make the persistent volume in 
> > `slaveFlags.resources` from the very beginning, and completely get rid of 
> > Step 4 and 6, and thus the whole test would be much easier to read.

The file check is definitely necessary IMO, because we are guaranteed that 
content of volume is not affected during grow/shrink, but I'm okay to use a 
file operation directly rather than a task to perform the check.

+1 for splitting the test.

w.r.t to directly creating volume from the very beginning, isn't persistent 
volume controlled by checkpointed info rather than `slaveFlags.resources`?


> On April 15, 2018, 5:39 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 519-570 (patched)
> > 
> >
> > I feel the whole task launch doesn't need to be tested in this unit 
> > test. See my comments above.

Will convert to file operation directly.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test 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.

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.


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

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.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Gaston Kleiman]

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




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.



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.


- Gaston Kleiman


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test 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?

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.


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 492 (patched)
> > 
> >
> > Is this necessary? Since the clock is paused, is it really possible 
> > that we'll get more offers than expected?

This is very common in our tests (there are more than 500 line matches with 
this comment through `git grep`).

My guess is that `clock::advance` can guarantee at least one offer being 
triggered but there might be some race condition cases which would make the 
test flaky if we do not guard against this?


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 549 (patched)
> > 
> >
> > Is this necessary?

See above comment.


> On April 13, 2018, 9:43 a.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 553-554 (patched)
> > 
> >
> > Is this necessary? Awaiting on the task status updates may be 
> > sufficient?

See above comment.


- Zhitao


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


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

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

[Greg Mann]

> On April 16, 2018, 5:57 p.m., Zhitao Li wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 541-542 (patched)
> > 
> >
> > Let's capture this message and test its content to make sure proper 
> > operation is forwarded.

I think it's OK to rely on the successful task launch to infer that the volume 
was created successfully. I would recommend removing this future.


> On April 16, 2018, 5:57 p.m., Zhitao Li wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 597-603 (patched)
> > 
> >
> > This was necessary if the operation is implemented as non-speculative, 
> > because master/allocator only sends out `converted` resources after 
> > receiving the feedback from agent. If we don't ensure this is processed, 
> > the offer could be subject to race condition (I think).
> > 
> > Now that we changed the course, this may not be necessary in 1.6, but 
> > we'd need them again once we get to MESOS-8791.
> > 
> > Please let me know if you think keeping it is still a bad idea.

I think if the framework accepts the offer and then you immediately call 
`Clock::settle()`, you can still be sure that the next offer you get will 
contain the volume. I would recommend getting rid of the `growVolumeMessage` 
and `shrinkVolumeMessage` futures.


- Greg


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


On April 11, 2018, 9:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 9:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test 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/#review201235
---



Please consider unreplied comments as "will do".


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


Is the goal of splitting the test to have more focused tests? Otherwise, 
just taking out the task launching part from the code seems resulting in 
minimum amount of code.

The file check actually verifies the fix for r/66218, which I think is 
something pretty important (not losing data during a grow/shrink). Only 
checking path exists for the volume would not capture that behavior.



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


I think we discussed about not allowing this but I forgot add this to 
validation. I'll update parent patch to include that validation.

If we will drop any `GROW`/`SHRINK` operation, then we can verify that 
`GROW` does not trigger any `ApplyOperationMessage` and framework will be 
offered original volume as is and keep this well tested.



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


I do not see a possibility. Let me try to drop it and see what happens.



src/tests/persistent_volume_tests.cpp
Lines 541-542 (patched)


Let's capture this message and test its content to make sure proper 
operation is forwarded.



src/tests/persistent_volume_tests.cpp
Lines 597-603 (patched)


This was necessary if the operation is implemented as non-speculative, 
because master/allocator only sends out `converted` resources after receiving 
the feedback from agent. If we don't ensure this is processed, the offer could 
be subject to race condition (I think).

Now that we changed the course, this may not be necessary in 1.6, but we'd 
need them again once we get to MESOS-8791.

Please let me know if you think keeping it is still a bad idea.



src/tests/persistent_volume_tests.cpp
Lines 677-678 (patched)


Resources::contains only matches a volume if their size perfectly matches, 
so I think it is sufficient.


- Zhitao Li


On April 11, 2018, 2:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 2:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test 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/#review201180
---




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


If I'm not mistaken, this test does the following steps:

1. Create a persistent volume
2. Launch a task to write a file in the volume
3. Grow the volume
4. Check if the file exists in the volume
5. Shrink the volume
6. Check if the file exists in the volume

The majority of this test is to implement Step 1 and 2, which are already 
testsed in `AccessPersistentVolume` and not really relevent to the growing and 
shrinking functions. How about splitting this test into two tests, 
`GrowPersistentVolume` and `ShrinkPersistentVolume`, where the first one only 
does Step 1, 3, 4 and the second only does Step 1, 5, 6? The creation of the 
file can be done with a simple `os::write()` to the volume path instead of 
using a task.

Also, I'm not sure if the file check is really necessary. If not, we can 
just check the existence of the volume path in Step 4 and 6. Furthermore, we 
could even choose not to check the existence of the volume path. In this case, 
we could make the persistent volume in `slaveFlags.resources` from the very 
beginning, and completely get rid of Step 4 and 6, and thus the whole test 
would be much easier to read.



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


Or alternatively, we can do the following:
```
class PersistentVolumeResizeTest : public PersistentVolumeTest {};

INSTANTIATE_TEST_CASE_P(
DiskResource, /* Not sure if the same name can be used twice */
PersistentVolumeResizeTest,
::testing::Values(
PersistentVolumeSourceType::NONE,
PersistentVolumeSourceType::PATH));

TEST_P(PersistentVolumeResizeTest, GrowAndShrink)
{
  ...
}
```
And remove the conditional return.



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


The `roles` master flag is deprecated and there's no need to set it.



src/tests/persistent_volume_tests.cpp
Lines 519-570 (patched)


I feel the whole task launch doesn't need to be tested in this unit test. 
See my comments above.



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


Let's use all remaining free disk in the offer, so we can check that the 
consumed resource is indeed used and does not show up in the subsequent offer.

If you use only 1G here, then there will be 1G free disk remaining and this 
would make the above check hard.



src/tests/persistent_volume_tests.cpp
Lines 597-603 (patched)


I agree with Greg on this. Let's not check the internal 
`ApplyOperationMessage`, but check the public behavior: make the framework wait 
for a subsequent offer and then check if the offer has the grown volume. Note 
that `Resources(offer.resources()).persistentVolumes().contains(grownVolume)` 
is not enough. We should check if the size meets the expectation.

Also, let's check that the new offer doesn't contain the disk resource set 
up in `addition`.



src/tests/persistent_volume_tests.cpp
Lines 650-655 (patched)


Since the test checks if the subsequent offer contains the shrunk volume, 
there is no need to check the internal `ApplyOperationMessage`.



src/tests/persistent_volume_tests.cpp
Lines 677-678 (patched)


This check is not enough. We should check if the size of the persistent 
volume in the offer meets the expectation.

Also, let's check that the freed disk resource shows up in the offer.


- Chun-Hung Hsiao


On April 11, 2018, 9:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 9:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test 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/#review201109
---




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?



src/tests/persistent_volume_tests.cpp
Lines 471-479 (patched)


Nit: could you group all of the agent dependencies together?
i.e.
```
  Future updateSlaveMessage =
  FUTURE_PROTOBUF(UpdateSlaveMessage(), _, _);

  slave::Flags slaveFlags = CreateSlaveFlags();
  slaveFlags.resources = getSlaveResources();

  Owned detector = master.get()->createDetector();

  Try slave = StartSlave(detector.get(), slaveFlags);
  ASSERT_SOME(slave);
```



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


Is this necessary? Since the clock is paused, is it really possible that 
we'll get more offers than expected?



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


Could you move this to the top of the test to make it clearer that we will 
have the clock paused for the whole test?



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?



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


Indented too far.



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


Is this necessary?



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


Is this necessary? Awaiting on the task status updates may be sufficient?



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


Suggestion: you could use the `TaskStatusTaskIdEq` matcher in the 
`EXPECT_CALL(sched, statusUpdate(, _))` call to avoid these expectations 
for the task ID.



src/tests/persistent_volume_tests.cpp
Lines 597-603 (patched)


I'm not sure if these expectations are necessary - perhaps it's sufficient 
to confirm that the subsequent offer contains a volume of the expected size?

Ditto for the `shrinkVolumeMessage` below.



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


Is this comment correct? Won't this offer contain the grown volume?



src/tests/persistent_volume_tests.cpp
Lines 634-636 (patched)


I think these should be indented two more spaces.



src/tests/persistent_volume_tests.cpp
Lines 657-658 (patched)


Is this necessary?



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


Is this necessary?



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


Is this necessary?


- Greg Mann


On April 11, 2018, 9:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66220/
> ---
> 
> (Updated April 11, 2018, 9:19 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 test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66220: Added test 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 11, 2018, 2:19 p.m.)


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


Changes
---

Use proper test macros.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added test 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 March 28, 2018, 11:29 a.m.)


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


Changes
---

Rebase and verify content on volume remains.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
4edf781711d9efdb994114aeb6289b6af750b87a 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66220: Added test 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/#review199788
---



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

Reviews applied: `['66049', '66050', '66052', '66051', '66218', '66219', 
'66220']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (109 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (1062 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (37 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (72 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (809 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (833 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (836 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (864 ms total)

[--] Global test environment tear-down
[==] 943 tests from 94 test cases ran. (494702 ms total)
[  PASSED  ] 942 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 215 DISABLED TESTS

```

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

```
I0322 17:48:28.511474  7556 master.cpp:10374] Updating the state of task 
8cd82f36-e752-4b85-8825-9b9b5781d64c of framework 
b9f57e0f-bc01-4782-af35-d9de66556fe9- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0322 17:48:28.511474  8468I0322 17:48:28.319489  2160 exec.cpp:162] Version: 
1.6.0
I0322 17:48:28.346469  8900 exec.cpp:236] Executor registered on agent 
b9f57e0f-bc01-4782-af35-d9de66556fe9-S0
I0322 17:48:28.350457  7236 executor.cpp:176] Received SUBSCRIBED event
I0322 17:48:28.355482  7236 executor.cpp:180] Subscribed executor on 
winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0322 17:48:28.356497  7236 executor.cpp:176] Received LAUNCH event
I0322 17:48:28.361474  7236 executor.cpp:648] Starting task 
8cd82f36-e752-4b85-8825-9b9b5781d64c
I0322 17:48:28.451474  7236 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0322 17:48:28.486456  7236 executor.cpp:661] Forked command at 10628
I0322 17:48:28.514452  7796 exec.cpp:445] Executor asked to shutdown
I0322 17:48:28.515460  6880 executor.cpp:176] Received SHUTDOWN event
I0322 17:48:28.515460  6880 executor.cpp:758] Shutting down
I0322 17:48:28.515460  6880 executor.cpp:868] Sending SIGTERM to process tree 
at pid  slave.cpp:3873] Shutting down framework 
b9f57e0f-bc01-4782-af35-d9de66556fe9-
I0322 17:48:28.512471  8468 slave.cpp:6617] Shutting down executor 
'8cd82f36-e752-4b85-8825-9b9b5781d64c' of framework 
b9f57e0f-bc01-4782-af35-d9de66556fe9- at executor(1)@10.3.1.8:52549
I0322 17:48:28.514452  8468 slave.cpp:919] Agent terminating
W0322 17:48:28.514452  8468 slave.cpp:3869] Ignoring shutdown framework 
b9f57e0f-bc01-4782-af35-d9de66556fe9- because it is terminating
I0322 17:48:28.515460  7556 master.cpp:10473] Removing task 
8cd82f36-e752-4b85-8825-9b9b5781d64c with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework b9f57e0f-bc01-4782-af35-d9de66556fe9- on 
agent b9f57e0f-bc01-4782-af35-d9de66556fe9-S0 at slave(412)@10.3.1.8:52528 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0322 17:48:28.517442  8468 containerizer.cpp:2338] Destroying container 
ffba61a9-0761-4f64-b03d-95468a286ebc in RUNNING state
I0322 17:48:28.518457  8468 containerizer.cpp:2952] Transitioning the state of 
container ffba61a9-0761-4f64-b03d-95468a286ebc from RUNNING to DESTROYING
I0322 17:48:28.518457  7556 master.cpp:1295] Agent 
b9f57e0f-bc01-4782-af35-d9de66556fe9-S0 at slave(412)@10.3.1.8:52528 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0322 17:48:28.518457  7556 master.cpp:3283] Disconnecting agent 
b9f57e0f-bc01-4782-af35-d9de66556fe9-S0 at 

Review Request 66220: Added test 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/
---

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


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


Repository: mesos


Description
---

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


Diffs
-

  src/tests/persistent_volume_tests.cpp 
924d8458e54e34a49c99593482b5908c5f7c7a48 


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


Testing
---


Thanks,

Zhitao Li