mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 72806: Added tests for 'volume/csi' isolator recovery.
Date Fri, 04 Sep 2020 01:26:26 GMT


> On Sept. 3, 2020, 3:47 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 1122-1123 (patched)
> > <https://reviews.apache.org/r/72806/diff/5/?file=2239184#file2239184line1122>
> >
> >     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?

Good call, I decided to use the existence of the mount target path to verify the unpublish
call, let me know what you think.


> On Sept. 3, 2020, 3:47 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 1153-1154 (patched)
> > <https://reviews.apache.org/r/72806/diff/5/?file=2239184#file2239184line1153>
> >
> >     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?

Good call, thank you!


> On Sept. 3, 2020, 3:47 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 1530-1531 (patched)
> > <https://reviews.apache.org/r/72806/diff/5/?file=2239184#file2239184line1530>
> >
> >     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"?

Excellent, thanks - I changed the name of this test and added a new one which tests the orphan
container case.


- Greg


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


On Sept. 4, 2020, 1:25 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72806/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2020, 1:25 a.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/7/
> 
> 
> Testing
> -------
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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