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:06 UTC
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/
-----------------------------------------------------------
(Updated Sept. 2, 2020, 5:52 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10163
https://issues.apache.org/jira/browse/MESOS-10163
Repository: mesos
Description
-------
Added tests for the 'volume/csi' isolator.
Diffs (updated)
-----
src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72728/diff/13/
Changes: https://reviews.apache.org/r/72728/diff/12-13/
Testing
-------
`sudo make check`
Thanks,
Greg Mann
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/#review221793
-----------------------------------------------------------
Ship it!
Ship It!
- Qian Zhang
On Sept. 3, 2020, 3:05 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72728/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2020, 3:05 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the 'volume/csi' isolator.
>
>
> Diffs
> -----
>
> src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
> src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
> src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
> src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72728/diff/14/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/
-----------------------------------------------------------
(Updated Sept. 2, 2020, 7:05 p.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10163
https://issues.apache.org/jira/browse/MESOS-10163
Repository: mesos
Description
-------
Added tests for the 'volume/csi' isolator.
Diffs (updated)
-----
src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72728/diff/14/
Changes: https://reviews.apache.org/r/72728/diff/13-14/
Testing
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Sept. 2, 2020, 1:35 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 238 (patched)
> > <https://reviews.apache.org/r/72728/diff/13/?file=2239017#file2239017line238>
> >
> > Do we really need this method? I see it is only called when we create CSI server in `ROOT_PluginConfigAddedAtRuntime`, can we just create the CSI server with NULL secret generator like what we do in `ROOT_InvalidPluginConfig`?
I'd rather leave it in, since it is also used by the subsequent tests in the next patch in this chain. In order to run these tests with authentication enabled, we need to create a secret generator. Since we have some nontrivial authentication code in these code paths, keeping authentication enabled seems like a good idea to me. What do you think?
> On Sept. 2, 2020, 1:35 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 794 (patched)
> > <https://reviews.apache.org/r/72728/diff/13/?file=2239017#file2239017line794>
> >
> > Usually we use `SUDO_USER` as the non-root user in our unit tests, see https://github.com/apache/mesos/blob/1.10.0/src/tests/containerizer/docker_volume_isolator_tests.cpp#L1335 . And the test name should have the `UNPRIVILEGED_USER_` prefix.
> >
> > Just realized in a couple of your tests here, we need to pull Docker image `alpine` from Docker Hub, right? Then I think we need to include the prefix `INTERNET_CURL_` in the test name.
Thanks!!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/#review221754
-----------------------------------------------------------
On Sept. 2, 2020, 5:57 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72728/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2020, 5:57 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the 'volume/csi' isolator.
>
>
> Diffs
> -----
>
> src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
> src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
> src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
> src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72728/diff/13/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Sept. 2, 2020, 1:35 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 794 (patched)
> > <https://reviews.apache.org/r/72728/diff/13/?file=2239017#file2239017line794>
> >
> > Usually we use `SUDO_USER` as the non-root user in our unit tests, see https://github.com/apache/mesos/blob/1.10.0/src/tests/containerizer/docker_volume_isolator_tests.cpp#L1335 . And the test name should have the `UNPRIVILEGED_USER_` prefix.
> >
> > Just realized in a couple of your tests here, we need to pull Docker image `alpine` from Docker Hub, right? Then I think we need to include the prefix `INTERNET_CURL_` in the test name.
>
> Greg Mann wrote:
> Thanks!!
I ended up switching the tests to use a mixture of tasks with a rootfs and tasks without - let me know what you think.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/#review221754
-----------------------------------------------------------
On Sept. 2, 2020, 7:05 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72728/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2020, 7:05 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the 'volume/csi' isolator.
>
>
> Diffs
> -----
>
> src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
> src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
> src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
> src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72728/diff/14/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
Posted by Qian Zhang <zh...@gmail.com>.
> On Sept. 2, 2020, 9:35 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 238 (patched)
> > <https://reviews.apache.org/r/72728/diff/13/?file=2239017#file2239017line238>
> >
> > Do we really need this method? I see it is only called when we create CSI server in `ROOT_PluginConfigAddedAtRuntime`, can we just create the CSI server with NULL secret generator like what we do in `ROOT_InvalidPluginConfig`?
>
> Greg Mann wrote:
> I'd rather leave it in, since it is also used by the subsequent tests in the next patch in this chain. In order to run these tests with authentication enabled, we need to create a secret generator. Since we have some nontrivial authentication code in these code paths, keeping authentication enabled seems like a good idea to me. What do you think?
Yeah, I agree.
> On Sept. 2, 2020, 9:35 p.m., Qian Zhang wrote:
> > src/tests/containerizer/volume_csi_isolator_tests.cpp
> > Lines 794 (patched)
> > <https://reviews.apache.org/r/72728/diff/13/?file=2239017#file2239017line794>
> >
> > Usually we use `SUDO_USER` as the non-root user in our unit tests, see https://github.com/apache/mesos/blob/1.10.0/src/tests/containerizer/docker_volume_isolator_tests.cpp#L1335 . And the test name should have the `UNPRIVILEGED_USER_` prefix.
> >
> > Just realized in a couple of your tests here, we need to pull Docker image `alpine` from Docker Hub, right? Then I think we need to include the prefix `INTERNET_CURL_` in the test name.
>
> Greg Mann wrote:
> Thanks!!
>
> Greg Mann wrote:
> I ended up switching the tests to use a mixture of tasks with a rootfs and tasks without - let me know what you think.
SGTM!
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/#review221754
-----------------------------------------------------------
On Sept. 3, 2020, 3:05 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72728/
> -----------------------------------------------------------
>
> (Updated Sept. 3, 2020, 3:05 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the 'volume/csi' isolator.
>
>
> Diffs
> -----
>
> src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
> src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
> src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
> src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72728/diff/14/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/#review221754
-----------------------------------------------------------
src/Makefile.am
Lines 2875 (patched)
<https://reviews.apache.org/r/72728/#comment310831>
The indent seems not correct.
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 21-22 (patched)
<https://reviews.apache.org/r/72728/#comment310832>
I think we should swap these two lines and add a newline between them.
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 95 (patched)
<https://reviews.apache.org/r/72728/#comment310833>
This one seems unused.
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 132-133 (patched)
<https://reviews.apache.org/r/72728/#comment310834>
A newline between, or we could just swap these two lines.
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 238 (patched)
<https://reviews.apache.org/r/72728/#comment310835>
Do we really need this method? I see it is only called when we create CSI server in `ROOT_PluginConfigAddedAtRuntime`, can we just create the CSI server with NULL secret generator like what we do in `ROOT_InvalidPluginConfig`?
src/tests/containerizer/volume_csi_isolator_tests.cpp
Lines 794 (patched)
<https://reviews.apache.org/r/72728/#comment310836>
Usually we use `SUDO_USER` as the non-root user in our unit tests, see https://github.com/apache/mesos/blob/1.10.0/src/tests/containerizer/docker_volume_isolator_tests.cpp#L1335 . And the test name should have the `UNPRIVILEGED_USER_` prefix.
Just realized in a couple of your tests here, we need to pull Docker image `alpine` from Docker Hub, right? Then I think we need to include the prefix `INTERNET_CURL_` in the test name.
- Qian Zhang
On Sept. 2, 2020, 1:57 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72728/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2020, 1:57 p.m.)
>
>
> Review request for mesos, Andrei Budnik and Qian Zhang.
>
>
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added tests for the 'volume/csi' isolator.
>
>
> Diffs
> -----
>
> src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
> src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
> src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
> src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72728/diff/13/
>
>
> Testing
> -------
>
> `sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72728/
-----------------------------------------------------------
(Updated Sept. 2, 2020, 5:57 a.m.)
Review request for mesos, Andrei Budnik and Qian Zhang.
Bugs: MESOS-10163
https://issues.apache.org/jira/browse/MESOS-10163
Repository: mesos
Description
-------
Added tests for the 'volume/csi' isolator.
Diffs
-----
src/Makefile.am 1043c7b860372a17dba1e84fe5547388cb6a3b63
src/tests/CMakeLists.txt 6b420d03e85470c16de85c1cbb81ec339142e226
src/tests/cluster.cpp 3c8685565f9de6c314f6be758368f1eff46e2370
src/tests/containerizer/volume_csi_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/72728/diff/13/
Testing (updated)
-------
`sudo bin/mesos-tests.sh --gtest_filter="*VolumeCSIIsolatorTest*"`
Thanks,
Greg Mann