mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 72806: Added tests for 'volume/csi' isolator recovery.
Date Thu, 03 Sep 2020 15:47:11 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/#review221796
-----------------------------------------------------------




src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 1122-1123 (patched)
<https://reviews.apache.org/r/72806/#comment310877>

    How do we verify the volume is successfully unpublished? Use `TASK_FINISHED` status update
as the signal? Do we need to check if the target path is indeed unmounted by the unpublish
operation?



src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 1153-1154 (patched)
<https://reviews.apache.org/r/72806/#comment310874>

    Do we really need to explicitly create containerizer here? I think usually we need to
create containerizer in a test because we need to call its method in the test, like: `containerizer->wait()`,
`containerizer->containers()`. But in this test, I do not see we call its methods.
    
    Maybe we should call `StartSlave()` without specifying containerizer which will be implicitly
created in `Slave::create()`, this may simiply the code of this test, and we may need to do
`slave->reset();` after `agent.get()->terminate();` like: https://github.com/apache/mesos/blob/1.10.0/src/tests/containerizer/cni_isolator_tests.cpp#L2882:L2883
    
    WDYT?



src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 1530-1531 (patched)
<https://reviews.apache.org/r/72806/#comment310876>

    I think this is not the definition of orphan container. Actually orphan containers are
the containers launched by the framework without checkpoint enabled. So if you do `frameworkInfo.set_checkpoint(false);`
at line 1401, then the framework will launch an orphan container. But I guess what you want
to verify in this test is not orphan container, instead it should be the container finishes
when agent is down, right? Maybe you can just update the name and the comments of this test
by not mentioning "orphan container"?


- Qian Zhang


On Sept. 3, 2020, 3:01 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72806/
> -----------------------------------------------------------
> 
> (Updated Sept. 3, 2020, 3:01 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for 'volume/csi' isolator recovery.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72806/diff/5/
> 
> 
> Testing
> -------
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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