> On April 13, 2018, 4:43 p.m., Greg Mann wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 455-459 (patched)
> > <https://reviews.apache.org/r/66220/diff/4/?file=1996762#file1996762line455>
> >
> > 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)
> > <https://reviews.apache.org/r/66220/diff/4/?file=1996762#file1996762line541>
> >
> > 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
>
>
