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