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