You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2020/09/02 05:52:24 UTC
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/
-----------------------------------------------------------
(Updated Sept. 2, 2020, 5:52 a.m.)
Review request for mesos and Qian Zhang.
Summary (updated)
-----------------
Added tests for 'volume/csi' isolator recovery.
Repository: mesos
Description (updated)
-------
Added tests for 'volume/csi' isolator recovery.
Diffs (updated)
-----
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72806/diff/3/
Changes: https://reviews.apache.org/r/72806/diff/2-3/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*UnpublishAfterAgentFailover*"`
Thanks,
Greg Mann
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
> 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
>
>
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/#review221803
-----------------------------------------------------------
Ship it!
Ship It!
- Qian Zhang
On Sept. 4, 2020, 2:39 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72806/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2020, 2:39 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/8/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/
-----------------------------------------------------------
(Updated Sept. 4, 2020, 6:39 a.m.)
Review request for mesos and Qian Zhang.
Repository: mesos
Description
-------
Added tests for 'volume/csi' isolator recovery.
Diffs (updated)
-----
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72806/diff/8/
Changes: https://reviews.apache.org/r/72806/diff/7-8/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Sept. 4, 2020, 3:04 a.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 1676-1689 (patched)
> > <https://reviews.apache.org/r/72806/diff/7/?file=2239262#file2239262line1676>
> >
> > Could you please elaborate a bit on why we need to publish the volume here? I think during recovery volume manager will publish the volume internally, right? And why does `targetPath` not exist when publishing volume succeeds?
I ended up removing this test since I wasn't able to synchronize it correctly.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/#review221801
-----------------------------------------------------------
On Sept. 4, 2020, 6:39 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, 6:39 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/8/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/#review221801
-----------------------------------------------------------
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 1462 (patched)
<https://reviews.apache.org/r/72806/#comment310883>
Do we need to do this for the other 2 tests?
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 1567-1568 (patched)
<https://reviews.apache.org/r/72806/#comment310884>
This comments seems not correct.
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 1676-1689 (patched)
<https://reviews.apache.org/r/72806/#comment310886>
Could you please elaborate a bit on why we need to publish the volume here? I think during recovery volume manager will publish the volume internally, right? And why does `targetPath` not exist when publishing volume succeeds?
- Qian Zhang
On Sept. 4, 2020, 9: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, 9: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
>
>
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-----
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72806/diff/7/
Changes: https://reviews.apache.org/r/72806/diff/6-7/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/
-----------------------------------------------------------
(Updated Sept. 4, 2020, 1:17 a.m.)
Review request for mesos and Qian Zhang.
Repository: mesos
Description
-------
Added tests for 'volume/csi' isolator recovery.
Diffs (updated)
-----
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72806/diff/6/
Changes: https://reviews.apache.org/r/72806/diff/5-6/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/
-----------------------------------------------------------
(Updated Sept. 3, 2020, 7:01 a.m.)
Review request for mesos and Qian Zhang.
Repository: mesos
Description
-------
Added tests for 'volume/csi' isolator recovery.
Diffs (updated)
-----
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72806/diff/5/
Changes: https://reviews.apache.org/r/72806/diff/4-5/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/
-----------------------------------------------------------
(Updated Sept. 2, 2020, 7:37 p.m.)
Review request for mesos and Qian Zhang.
Repository: mesos
Description
-------
Added tests for 'volume/csi' isolator recovery.
Diffs (updated)
-----
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72806/diff/4/
Changes: https://reviews.apache.org/r/72806/diff/3-4/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann
Re: Review Request 72806: Added tests for 'volume/csi' isolator
recovery.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72806/
-----------------------------------------------------------
(Updated Sept. 2, 2020, 5:53 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/3/
Testing (updated)
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann