mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66220: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operations.
Date Mon, 16 Apr 2018 00:39:38 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/66220/#comment282230>

    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)
<https://reviews.apache.org/r/66220/#comment282229>

    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)
<https://reviews.apache.org/r/66220/#comment282228>

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



src/tests/persistent_volume_tests.cpp
Lines 519-570 (patched)
<https://reviews.apache.org/r/66220/#comment282231>

    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)
<https://reviews.apache.org/r/66220/#comment282234>

    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)
<https://reviews.apache.org/r/66220/#comment282232>

    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)
<https://reviews.apache.org/r/66220/#comment282233>

    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)
<https://reviews.apache.org/r/66220/#comment282235>

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


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