mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zhitao Li <zhitaoli...@gmail.com>
Subject Re: Review Request 66220: Added tests for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
Date Fri, 27 Apr 2018 20:04:32 GMT


> On April 13, 2018, 9:43 a.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.
> 
> 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)
> > <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.
> 
> 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/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a 
> 
> 
> Diff: https://reviews.apache.org/r/66220/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message