You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/05/01 04:05:19 UTC
Re: Review Request 66532: Added test for authorization actions for
`RESIZE_VOLUME`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66532/#review202170
-----------------------------------------------------------
src/tests/authorization_tests.cpp
Lines 1980 (patched)
<https://reviews.apache.org/r/66532/#comment283849>
`resizing volumes`
src/tests/persistent_volume_tests.cpp
Lines 1027 (patched)
<https://reviews.apache.org/r/66532/#comment283853>
Suggestion: I know this creates a little inconsistency within this test file, but it would be more consistent if we use the full operation name (as in the code) in the backticks, i.e., "`GROW_VOLUME` and `SHRINK_VOLUME` operations." Or, just say "grow and shrink operations."
src/tests/persistent_volume_tests.cpp
Lines 1029-1030 (patched)
<https://reviews.apache.org/r/66532/#comment283884>
We could combine the `CREATE` call with a `GROW_VOLUME` or `SHRINK_VOLUME` except for the `GrowVolume` and `ShrinkVolume` tests, then this is TODO seems not that necessary.
src/tests/persistent_volume_tests.cpp
Lines 1037 (patched)
<https://reviews.apache.org/r/66532/#comment283854>
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 1046-1051 (patched)
<https://reviews.apache.org/r/66532/#comment283858>
This is not required. Also there's no need to create a nested block.
src/tests/persistent_volume_tests.cpp
Lines 1093 (patched)
<https://reviews.apache.org/r/66532/#comment283861>
`Offer offer = offersBeforeCreate->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1095 (patched)
<https://reviews.apache.org/r/66532/#comment283862>
`The disk spaces ... if the fixture parameter`
src/tests/persistent_volume_tests.cpp
Lines 1100 (patched)
<https://reviews.apache.org/r/66532/#comment283863>
`which does not`
src/tests/persistent_volume_tests.cpp
Lines 1117-1120 (patched)
<https://reviews.apache.org/r/66532/#comment283896>
Let's move this close to its first use.
src/tests/persistent_volume_tests.cpp
Lines 1124 (patched)
<https://reviews.apache.org/r/66532/#comment283864>
`containing the original volume`
src/tests/persistent_volume_tests.cpp
Lines 1128 (patched)
<https://reviews.apache.org/r/66532/#comment283865>
s/`and launch task `//
src/tests/persistent_volume_tests.cpp
Lines 1131 (patched)
<https://reviews.apache.org/r/66532/#comment283866>
To make this test more succinct, how about doing
`{CREATE(volume), GROW_VOLUME(volume, addition)}` so we can merge this `acceptOffers` with the subsequent one?
src/tests/persistent_volume_tests.cpp
Lines 1140 (patched)
<https://reviews.apache.org/r/66532/#comment283885>
`offer = offersBeforeGrow->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1152 (patched)
<https://reviews.apache.org/r/66532/#comment283877>
`containing the grown volume`
src/tests/persistent_volume_tests.cpp
Lines 1156 (patched)
<https://reviews.apache.org/r/66532/#comment283887>
`Grow the volume.` Or merge this call with the above `CREATE` one.
src/tests/persistent_volume_tests.cpp
Lines 1167 (patched)
<https://reviews.apache.org/r/66532/#comment283879>
`offer = offersAfterGrow->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1177-1178 (patched)
<https://reviews.apache.org/r/66532/#comment283880>
This is not needed since its the same as `addition`. If you prefer not to use the name `addition` all the time, how about renaming it to `difference`?
src/tests/persistent_volume_tests.cpp
Lines 1182 (patched)
<https://reviews.apache.org/r/66532/#comment283883>
`containing the original volume`
src/tests/persistent_volume_tests.cpp
Lines 1196 (patched)
<https://reviews.apache.org/r/66532/#comment283886>
`offer = offersAfterShrink->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1213 (patched)
<https://reviews.apache.org/r/66532/#comment283888>
Suggestion: This test verifies that `GROW_VOLUME` and `SHRINK_VOLUME` operations (or "grow and shrink operations" if you prefer) get dropped if authorization fails and no principal is supplied.
src/tests/persistent_volume_tests.cpp
Lines 1215 (patched)
<https://reviews.apache.org/r/66532/#comment283891>
Suggestion: `BadACLDropGrowAndShrink` for consistency.
src/tests/persistent_volume_tests.cpp
Lines 1221 (patched)
<https://reviews.apache.org/r/66532/#comment283889>
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 1230-1235 (patched)
<https://reviews.apache.org/r/66532/#comment283890>
This is not required. Also there's no need to create a nested block.
src/tests/persistent_volume_tests.cpp
Lines 1256 (patched)
<https://reviews.apache.org/r/66532/#comment283898>
Suggestion: Start the first framework with `DEFAULT_CREDENTIAL`, to be consistent with your comment for the second framework.
src/tests/persistent_volume_tests.cpp
Lines 1277 (patched)
<https://reviews.apache.org/r/66532/#comment283892>
`Offer offer = offersBeforeCreate->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1279 (patched)
<https://reviews.apache.org/r/66532/#comment283893>
`The disk spaces ... if the fixture parameter`
src/tests/persistent_volume_tests.cpp
Lines 1284 (patched)
<https://reviews.apache.org/r/66532/#comment283894>
`which does not`
src/tests/persistent_volume_tests.cpp
Lines 1292-1293 (patched)
<https://reviews.apache.org/r/66532/#comment283895>
How about renaming `addition` to `difference` and removing `subtract`?
src/tests/persistent_volume_tests.cpp
Lines 1302-1305 (patched)
<https://reviews.apache.org/r/66532/#comment283897>
Let's move this close to its first use.
src/tests/persistent_volume_tests.cpp
Lines 1309 (patched)
<https://reviews.apache.org/r/66532/#comment283900>
`containing the original volume`
src/tests/persistent_volume_tests.cpp
Lines 1316 (patched)
<https://reviews.apache.org/r/66532/#comment283899>
To make this test more succinct, how about doing
`{CREATE(volume), GROW_VOLUME(volume, addition)}` so we can merge this `acceptOffers` with the subsequent one, and check that the next offer contains the original volume but not the grown one?
src/tests/persistent_volume_tests.cpp
Lines 1325 (patched)
<https://reviews.apache.org/r/66532/#comment283860>
`offer = offersBeforeGrow1->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1336 (patched)
<https://reviews.apache.org/r/66532/#comment283901>
This is not necessary if we examine the offers.
src/tests/persistent_volume_tests.cpp
Lines 1343 (patched)
<https://reviews.apache.org/r/66532/#comment283905>
`Grow the volume.` Or merge with the above `acceptOffers`.
src/tests/persistent_volume_tests.cpp
Lines 1348 (patched)
<https://reviews.apache.org/r/66532/#comment283914>
`Clock::settle();`
src/tests/persistent_volume_tests.cpp
Lines 1351 (patched)
<https://reviews.apache.org/r/66532/#comment283857>
This test fails here:
```
../src/tests/persistent_volume_tests.cpp:1351: Failure
Failed to wait 15secs for offersAfterGrow1
```
src/tests/persistent_volume_tests.cpp
Lines 1353 (patched)
<https://reviews.apache.org/r/66532/#comment283859>
`offer = offersAfterGrow1->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1355 (patched)
<https://reviews.apache.org/r/66532/#comment283906>
`Check that the offer still contains the original volume.`
src/tests/persistent_volume_tests.cpp
Lines 1362 (patched)
<https://reviews.apache.org/r/66532/#comment283907>
`containing the original volume`
src/tests/persistent_volume_tests.cpp
Lines 1370 (patched)
<https://reviews.apache.org/r/66532/#comment283915>
`Clock::settle();`
src/tests/persistent_volume_tests.cpp
Lines 1375 (patched)
<https://reviews.apache.org/r/66532/#comment283904>
`offer = offersAfterShrink1->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1381 (patched)
<https://reviews.apache.org/r/66532/#comment283902>
The subsequent `Clock::advance()` calls have no effect once the clock is resumed.
I'm not sure if you can stop and join a driver with a paused clock. If not, please pause the clock again afterward.
src/tests/persistent_volume_tests.cpp
Lines 1386 (patched)
<https://reviews.apache.org/r/66532/#comment283908>
Suggestion: s/`which has`/`with`/
src/tests/persistent_volume_tests.cpp
Lines 1408 (patched)
<https://reviews.apache.org/r/66532/#comment283909>
`offer = offersBeforeGrow2->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1423 (patched)
<https://reviews.apache.org/r/66532/#comment283910>
`Grow the volume`
src/tests/persistent_volume_tests.cpp
Lines 1428 (patched)
<https://reviews.apache.org/r/66532/#comment283916>
`Clock::settle();`
src/tests/persistent_volume_tests.cpp
Lines 1433 (patched)
<https://reviews.apache.org/r/66532/#comment283918>
`offer = offersAfterGrow2->at(0);`
src/tests/persistent_volume_tests.cpp
Lines 1435 (patched)
<https://reviews.apache.org/r/66532/#comment283911>
`Check that the offer still contains the original volume.`
src/tests/persistent_volume_tests.cpp
Lines 1442 (patched)
<https://reviews.apache.org/r/66532/#comment283912>
`containing the original volume`
src/tests/persistent_volume_tests.cpp
Lines 1450 (patched)
<https://reviews.apache.org/r/66532/#comment283917>
`Clock::settle();`
src/tests/persistent_volume_tests.cpp
Lines 1464 (patched)
<https://reviews.apache.org/r/66532/#comment283913>
No need to resume the clock here, as the fixture teardown will resume it. However, I'm not sure if you need to resume the clock before stopping and joining `driver2`.
- Chun-Hung Hsiao
On April 27, 2018, 8:08 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> -----------------------------------------------------------
>
> (Updated April 27, 2018, 8:08 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added test for authorization actions for `RESIZE_VOLUME`.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c
> src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a
>
>
> Diff: https://reviews.apache.org/r/66532/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66532: Added test for authorization actions for
`RESIZE_VOLUME`.
Posted by Zhitao Li <zh...@gmail.com>.
> On April 30, 2018, 9:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 1256 (patched)
> > <https://reviews.apache.org/r/66532/diff/6/?file=2014384#file2014384line1256>
> >
> > Suggestion: Start the first framework with `DEFAULT_CREDENTIAL`, to be consistent with your comment for the second framework.
This is implicit by using `DEFAULT_FRAMEWORK_INFO`. Are you suggesting to call set_principal() explicitly?
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66532/#review202170
-----------------------------------------------------------
On April 27, 2018, 1:08 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> -----------------------------------------------------------
>
> (Updated April 27, 2018, 1:08 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added test for authorization actions for `RESIZE_VOLUME`.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c
> src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a
>
>
> Diff: https://reviews.apache.org/r/66532/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66532: Added test for authorization actions for
`RESIZE_VOLUME`.
Posted by Zhitao Li <zh...@gmail.com>.
> On April 30, 2018, 9:05 p.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 1464 (patched)
> > <https://reviews.apache.org/r/66532/diff/6/?file=2014384#file2014384line1464>
> >
> > No need to resume the clock here, as the fixture teardown will resume it. However, I'm not sure if you need to resume the clock before stopping and joining `driver2`.
I think enough tests explicitly resume the clock, so I'd like to keep it unless you have issue with it.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66532/#review202170
-----------------------------------------------------------
On April 27, 2018, 1:08 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> -----------------------------------------------------------
>
> (Updated April 27, 2018, 1:08 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added test for authorization actions for `RESIZE_VOLUME`.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c
> src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a
>
>
> Diff: https://reviews.apache.org/r/66532/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66532: Added test for authorization actions for
`RESIZE_VOLUME`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On May 1, 2018, 4:05 a.m., Chun-Hung Hsiao wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 1464 (patched)
> > <https://reviews.apache.org/r/66532/diff/6/?file=2014384#file2014384line1464>
> >
> > No need to resume the clock here, as the fixture teardown will resume it. However, I'm not sure if you need to resume the clock before stopping and joining `driver2`.
>
> Zhitao Li wrote:
> I think enough tests explicitly resume the clock, so I'd like to keep it unless you have issue with it.
That's fine but I was wondering if we should move this before `driver2.stop()`, as we do in other tests.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66532/#review202170
-----------------------------------------------------------
On April 27, 2018, 8:08 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66532/
> -----------------------------------------------------------
>
> (Updated April 27, 2018, 8:08 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8748
> https://issues.apache.org/jira/browse/MESOS-8748
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added test for authorization actions for `RESIZE_VOLUME`.
>
>
> Diffs
> -----
>
> src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c
> src/tests/persistent_volume_tests.cpp 4edf781711d9efdb994114aeb6289b6af750b87a
>
>
> Diff: https://reviews.apache.org/r/66532/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>