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