Re: Review Request 72945: Ignored the directoy `/dev/nvidia-caps` when globing Nvidia GPU devices.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72945/ --- (Updated Oct. 13, 2020, 10:31 a.m.) Review request for mesos, Benjamin Mahler and Kevin Klues. Changes --- Added a TODO. Bugs: MESOS-10192 https://issues.apache.org/jira/browse/MESOS-10192 Repository: mesos Description --- The directory `/dev/nvidia-caps` was introduced in CUDA 11.0, just ignore it since we only care about the Nvidia GPU device files. Diffs (updated) - src/slave/containerizer/mesos/isolators/gpu/isolator.cpp a0be1026bdaf7d4bb33f41e4c7d45666bd61f005 Diff: https://reviews.apache.org/r/72945/diff/3/ Changes: https://reviews.apache.org/r/72945/diff/2-3/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 72945: Ignored the directoy `/dev/nvidia-caps` when globing Nvidia GPU devices.
> On Oct. 13, 2020, 9:48 a.m., Benjamin Mahler wrote: > > The part that's not clear is why we can safely ignore it. If we don't have > > a solid answer, perhaps just add a TODO to figure out what we should do > > with it? Agreed. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72945/#review222030 --- On Oct. 13, 2020, 9:39 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72945/ > --- > > (Updated Oct. 13, 2020, 9:39 a.m.) > > > Review request for mesos, Benjamin Mahler and Kevin Klues. > > > Bugs: MESOS-10192 > https://issues.apache.org/jira/browse/MESOS-10192 > > > Repository: mesos > > > Description > --- > > The directory `/dev/nvidia-caps` was introduced in CUDA 11.0, just > ignore it since we only care about the Nvidia GPU device files. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > a0be1026bdaf7d4bb33f41e4c7d45666bd61f005 > > > Diff: https://reviews.apache.org/r/72945/diff/2/ > > > Testing > --- > > sudo make check > > > Thanks, > > Qian Zhang > >
Re: Review Request 72845: Added doc for the `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72845/ --- (Updated Oct. 13, 2020, 10:06 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comments. Bugs: MESOS-10157 https://issues.apache.org/jira/browse/MESOS-10157 Repository: mesos Description --- Added doc for the `volume/csi` isolator. Diffs (updated) - docs/isolators/csi-volume.md PRE-CREATION docs/mesos-containerizer.md 3231cb93481102dfc26a718918566e61b57e3617 Diff: https://reviews.apache.org/r/72845/diff/4/ Changes: https://reviews.apache.org/r/72845/diff/3-4/ Testing --- Thanks, Qian Zhang
Re: Review Request 72845: Added doc for the `volume/csi` isolator.
> On Oct. 12, 2020, 9:28 p.m., Andrei Sekretenko wrote: > > docs/isolators/csi-volume.md > > Lines 82 (patched) > > <https://reviews.apache.org/r/72845/diff/3/?file=2240545#file2240545line82> > > > > Maybe more explicit: s/two flags/the `isolation` and > > `csi_plugin_config_dir` flags/ ? > > > > The values of `--master` and `--work_dir` are irrelevant for CSI > > configuration despite being needed to launch the agent, right? Agreed! - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72845/#review222019 ------- On Oct. 8, 2020, 3:57 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72845/ > --- > > (Updated Oct. 8, 2020, 3:57 p.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10157 > https://issues.apache.org/jira/browse/MESOS-10157 > > > Repository: mesos > > > Description > --- > > Added doc for the `volume/csi` isolator. > > > Diffs > - > > docs/isolators/csi-volume.md PRE-CREATION > docs/mesos-containerizer.md 3231cb93481102dfc26a718918566e61b57e3617 > > > Diff: https://reviews.apache.org/r/72845/diff/3/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 72945: Ignored the directoy `/dev/nvidia-caps` when globing Nvidia GPU devices.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72945/ --- (Updated Oct. 13, 2020, 9:39 a.m.) Review request for mesos, Benjamin Mahler and Kevin Klues. Changes --- Addressed review comments. Summary (updated) - Ignored the directoy `/dev/nvidia-caps` when globing Nvidia GPU devices. Bugs: MESOS-10192 https://issues.apache.org/jira/browse/MESOS-10192 Repository: mesos Description (updated) --- The directory `/dev/nvidia-caps` was introduced in CUDA 11.0, just ignore it since we only care about the Nvidia GPU device files. Diffs (updated) - src/slave/containerizer/mesos/isolators/gpu/isolator.cpp a0be1026bdaf7d4bb33f41e4c7d45666bd61f005 Diff: https://reviews.apache.org/r/72945/diff/2/ Changes: https://reviews.apache.org/r/72945/diff/1-2/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 72945: Ignored directoy entries when globing Nvidia GPU devices.
> On Oct. 13, 2020, 3:20 a.m., Benjamin Mahler wrote: > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > > Lines 446-450 (patched) > > <https://reviews.apache.org/r/72945/diff/1/?file=2240565#file2240565line446> > > > > The reader will wonder why this was introduced? > > > > Perhaps a comment explaining that now there was an `nvidia-caps` folder > > introduced? > > > > Personally, I would have expected one of the following approaches: > > > > * Ignore all non-device files, or > > * Ignore `nvidia-caps` specifically, since we probably want to know in > > the future if there's some other non device stuff getting added Agreed, let's ignore `nvidia-caps` specifically. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72945/#review222025 ------- On Oct. 12, 2020, 9:50 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72945/ > --- > > (Updated Oct. 12, 2020, 9:50 a.m.) > > > Review request for mesos, Benjamin Mahler and Kevin Klues. > > > Bugs: MESOS-10192 > https://issues.apache.org/jira/browse/MESOS-10192 > > > Repository: mesos > > > Description > --- > > Ignored directoy entries when globing Nvidia GPU devices. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > a0be1026bdaf7d4bb33f41e4c7d45666bd61f005 > > > Diff: https://reviews.apache.org/r/72945/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Qian Zhang > >
Re: Review Request 72945: Ignored directoy entries when globing Nvidia GPU devices.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72945/ --- (Updated Oct. 10, 2020, 9:39 p.m.) Review request for mesos and Benjamin Mahler. Bugs: MESOS-10192 https://issues.apache.org/jira/browse/MESOS-10192 Repository: mesos Description --- Ignored directoy entries when globing Nvidia GPU devices. Diffs - src/slave/containerizer/mesos/isolators/gpu/isolator.cpp a0be1026bdaf7d4bb33f41e4c7d45666bd61f005 Diff: https://reviews.apache.org/r/72945/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 72945: Ignored directoy entries when globing Nvidia GPU devices.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72945/ --- Review request for mesos. Repository: mesos Description --- Ignored directoy entries when globing Nvidia GPU devices. Diffs - src/slave/containerizer/mesos/isolators/gpu/isolator.cpp a0be1026bdaf7d4bb33f41e4c7d45666bd61f005 Diff: https://reviews.apache.org/r/72945/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 72845: Added doc for the `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72845/ --- (Updated Oct. 8, 2020, 3:57 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comments. Bugs: MESOS-10157 https://issues.apache.org/jira/browse/MESOS-10157 Repository: mesos Description --- Added doc for the `volume/csi` isolator. Diffs (updated) - docs/isolators/csi-volume.md PRE-CREATION docs/mesos-containerizer.md 3231cb93481102dfc26a718918566e61b57e3617 Diff: https://reviews.apache.org/r/72845/diff/3/ Changes: https://reviews.apache.org/r/72845/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 72888: Inferred CSI volume's `readonly` field from volume mode.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72888/ --- (Updated Sept. 19, 2020, 4:05 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Inferred CSI volume's `readonly` field from volume mode. Diffs - include/mesos/mesos.proto a10084439d5f86855ce179e7a2eba1a46b15c948 include/mesos/v1/mesos.proto 09973abb94ef016c1fca4055ea781b21be5cb4fb src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp 4349acd93687f2a218ed6bf0b09e0cbc63c10822 src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 79a68606ab1f1a2dc5c68acf4f50bdfec44ff4ec src/slave/csi_server.hpp de5c6b6a859b4f5cb0db18636196850b660e17bf src/slave/csi_server.cpp 14fa8664e4d0a6645510babcbe4c8cfa69bb4c96 src/tests/containerizer/volume_csi_isolator_tests.cpp d51d3c94e1a707f54d6b36ce6484068243e696c2 src/tests/mesos.hpp 49abfc2aabaa151f9b7642db2ba34bf63554d48e Diff: https://reviews.apache.org/r/72888/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Review Request 72888: Inferred CSI volume's `readonly` field from volume mode.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72888/ --- Review request for mesos, Andrei Budnik and Greg Mann. Repository: mesos Description --- Inferred CSI volume's `readonly` field from volume mode. Diffs - include/mesos/mesos.proto a10084439d5f86855ce179e7a2eba1a46b15c948 include/mesos/v1/mesos.proto 09973abb94ef016c1fca4055ea781b21be5cb4fb src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp 4349acd93687f2a218ed6bf0b09e0cbc63c10822 src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 79a68606ab1f1a2dc5c68acf4f50bdfec44ff4ec src/slave/csi_server.hpp de5c6b6a859b4f5cb0db18636196850b660e17bf src/slave/csi_server.cpp 14fa8664e4d0a6645510babcbe4c8cfa69bb4c96 src/tests/containerizer/volume_csi_isolator_tests.cpp d51d3c94e1a707f54d6b36ce6484068243e696c2 src/tests/mesos.hpp 49abfc2aabaa151f9b7642db2ba34bf63554d48e Diff: https://reviews.apache.org/r/72888/diff/1/ Testing --- sudo make check Thanks, Qian Zhang
Re: Review Request 72846: Corrected the example of the managed CSI plugin.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72846/ --- (Updated Sept. 9, 2020, 10:46 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10151 https://issues.apache.org/jira/browse/MESOS-10151 Repository: mesos Description --- Corrected the example of the managed CSI plugin. Diffs - docs/configuration/agent.md 4899202e1afa2988acb3c066829e26aa9612fd29 src/slave/flags.cpp 878788c0cb675a16ef69abd4facc8ca89cbd19d3 Diff: https://reviews.apache.org/r/72846/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 72846: Corrected the example of the managed CSI plugin.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72846/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10151 https://issues.apache.org/jira/browse/MESOS-10151 Repository: mesos Description --- Corrected the example of the managed CSI plugin. Diffs - docs/configuration/agent.md 4899202e1afa2988acb3c066829e26aa9612fd29 src/slave/flags.cpp 878788c0cb675a16ef69abd4facc8ca89cbd19d3 Diff: https://reviews.apache.org/r/72846/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72845: Added doc for the `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72845/ --- (Updated Sept. 9, 2020, 10:43 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Minor fix. Bugs: MESOS-10157 https://issues.apache.org/jira/browse/MESOS-10157 Repository: mesos Description --- Added doc for the `volume/csi` isolator. Diffs (updated) - docs/isolators/csi-volume.md PRE-CREATION docs/mesos-containerizer.md 3231cb93481102dfc26a718918566e61b57e3617 Diff: https://reviews.apache.org/r/72845/diff/2/ Changes: https://reviews.apache.org/r/72845/diff/1-2/ Testing --- Thanks, Qian Zhang
Review Request 72845: Added doc for the `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72845/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10157 https://issues.apache.org/jira/browse/MESOS-10157 Repository: mesos Description --- Added doc for the `volume/csi` isolator. Diffs - docs/isolators/csi-volume.md PRE-CREATION docs/mesos-containerizer.md 3231cb93481102dfc26a718918566e61b57e3617 Diff: https://reviews.apache.org/r/72845/diff/1/ Testing --- Thanks, Qian Zhang
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/#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.
--- 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.
--- 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 72728: Added tests for the 'volume/csi' isolator.
> 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.
--- 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 72794: Added a test for discarding image pull on discard of getting image.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72794/#review221792 --- Fix it, then Ship it! Ship It! src/tests/containerizer/provisioner_docker_tests.cpp Lines 437-438 (patched) <https://reviews.apache.org/r/72794/#comment310868> A newline between. - Qian Zhang On Aug. 21, 2020, 1:08 a.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72794/ > --- > > (Updated Aug. 21, 2020, 1:08 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10169 > https://issues.apache.org/jira/browse/MESOS-10169 > > > Repository: mesos > > > Description > --- > > This test checks that when all futures returned by docker store's > `get()` that are pending image pull are discarded by the callers, > the pull future (returned by the puller to the store) is discarded > by the store as well. > > > Diffs > - > > src/tests/containerizer/provisioner_docker_tests.cpp > c3ef4a0beb3091043f51ab840adbb5da9da400fc > > > Diff: https://reviews.apache.org/r/72794/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72793: Added discarding a future returned by get() into the docker store test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72793/#review221791 --- Ship it! Ship It! - Qian Zhang On Aug. 21, 2020, 1:07 a.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72793/ > --- > > (Updated Aug. 21, 2020, 1:07 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10169 > https://issues.apache.org/jira/browse/MESOS-10169 > > > Repository: mesos > > > Description > --- > > This patch extends the `PullingSameImageSimultaneously` test > with discarding one of the futures returned by `store->get()` > and making sure that the pull still completes. > > > Diffs > - > > src/tests/containerizer/provisioner_docker_tests.cpp > c3ef4a0beb3091043f51ab840adbb5da9da400fc > > > Diff: https://reviews.apache.org/r/72793/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72792: Fixed typos in the name of `PullingSameImageSimutanuously` test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72792/#review221790 --- Ship it! Ship It! - Qian Zhang On Aug. 21, 2020, 1:07 a.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72792/ > --- > > (Updated Aug. 21, 2020, 1:07 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10169 > https://issues.apache.org/jira/browse/MESOS-10169 > > > Repository: mesos > > > Description > --- > > Fixed typos in the name of `PullingSameImageSimutanuously` test. > > > Diffs > - > > src/tests/containerizer/provisioner_docker_tests.cpp > c3ef4a0beb3091043f51ab840adbb5da9da400fc > > > Diff: https://reviews.apache.org/r/72792/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72791: Reverted removal of the PullingSameImageSimutanuously test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72791/#review221789 --- Ship it! Ship It! - Qian Zhang On Aug. 21, 2020, 1:06 a.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72791/ > --- > > (Updated Aug. 21, 2020, 1:06 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10169 > https://issues.apache.org/jira/browse/MESOS-10169 > > > Repository: mesos > > > Description > --- > > Now that the docker store triggers pull at most once per multiple > simultaneous requests to the store, removal of the > `PullingSameImageSimutanuously` test in > 33c61a1907129126f3b2e37b1f53827a04e89a34 can be reverted. > > > Diffs > - > > src/tests/containerizer/provisioner_docker_tests.cpp > c3ef4a0beb3091043f51ab840adbb5da9da400fc > > > Diff: https://reviews.apache.org/r/72791/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 72790: Deduplicated concurrent image pulls by docker store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72790/#review221778 --- Fix it, then Ship it! src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 152 (patched) <https://reviews.apache.org/r/72790/#comment310840> Can we add a comment for this `hashmap`? Like what is its key. src/slave/containerizer/mesos/provisioner/docker/store.cpp Line 362 (original), 384-385 (patched) <https://reviews.apache.org/r/72790/#comment310841> A newline between. - Qian Zhang On Aug. 21, 2020, 1:06 a.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72790/ > --- > > (Updated Aug. 21, 2020, 1:06 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10169 > https://issues.apache.org/jira/browse/MESOS-10169 > > > Repository: mesos > > > Description > --- > > This patch makes the docker store reuse a pending pull if asked for > an image that is already being pulled. > > The pull caching follows the same approach as the earlier attempt in > https://reviews.apache.org/r/39331 . However, handing out futures to > the store callers and handling their discards are performed differently, > using a form of reference counting, so that the pull itself is discarded > only if all the futures returned by `Store::get()` have been discarded. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/store.cpp > bf2be90aa4fcd7541849a157eb6930b491ccf03a > > > Diff: https://reviews.apache.org/r/72790/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
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/#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 72805: Added a test helper for CSI volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72805/#review221775 --- Ship it! Ship It! - Qian Zhang On Sept. 2, 2020, 1:50 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72805/ > --- > > (Updated Sept. 2, 2020, 1:50 p.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Added a test helper for CSI volumes. > > > Diffs > - > > src/tests/mesos.hpp 8f89d7ca5642a475ecbc176d8bba277a6774a8a1 > > > Diff: https://reviews.apache.org/r/72805/diff/2/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Review Request 72829: Moved the `volume/csi` isolator's root dir under work dir.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72829/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- The `volume/csi` isolator needs to checkpoint CSI volume state under work dir rather than runtime dir to be consistent with what volume manager does. Otherwise after agent host is rebooted, volume manager may publish some volumes during recovery, and those volumes will never get chance to be unpublished since the `volume/csi` isolator does not know those volumes at all (the contents in runtime dir will be gone after reboot). Diffs - src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp d5d8835f00798884bcf46f728470ee18868b405c src/slave/containerizer/mesos/isolators/volume/csi/paths.hpp 5b4a4eeed0e85a5bed90056638414acf3a9e7ab5 Diff: https://reviews.apache.org/r/72829/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72804: Enabled CSI volume access for non-root users.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72804/ --- (Updated Sept. 1, 2020, 8:50 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comments. Summary (updated) - Enabled CSI volume access for non-root users. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description (updated) --- Enabled CSI volume access for non-root users. Diffs (updated) - src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp 373b629a984b6d7f207bc65899e32d5f023685ee src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 02ef1f215eeb0e6436d42146b922d8a48e023607 Diff: https://reviews.apache.org/r/72804/diff/3/ Changes: https://reviews.apache.org/r/72804/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 72816: Fixed broken authorization in the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72816/#review221751 --- Ship it! So we actually rely on `LocalImplicitResourceProviderObjectApprover` to authorize CSI server, right? https://github.com/apache/mesos/blob/1.10.0/src/authorizer/local/authorizer.cpp#L1128:L1140 - Qian Zhang On Aug. 29, 2020, 8:44 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72816/ > --- > > (Updated Aug. 29, 2020, 8:44 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > The CSI server must use a principal when authenticating > which contains a claim that allows the authorizer to > implicitly approve requests from the CSI server to the > agent's HTTP API. > > > Diffs > - > > src/slave/csi_server.cpp 3f29a814daf5335a9079a9a33d77c6bee72d321d > > > Diff: https://reviews.apache.org/r/72816/diff/1/ > > > Testing > --- > > Testing details at the end of this chain. This patch is required for the > upcoming tests to pass when Mesos is built with SSL enabled. > > > Thanks, > > Greg Mann > >
Review Request 72820: Relaxed unknown volume check when unpublishing volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72820/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10150 https://issues.apache.org/jira/browse/MESOS-10150 Repository: mesos Description --- Relaxed unknown volume check when unpublishing volumes. Diffs - src/csi/v0_volume_manager.cpp 8ba6100880aeff8109690166aef1bad345fcea76 src/csi/v1_volume_manager.cpp 29ae821f0d7411816de9363aa6364b4874097942 Diff: https://reviews.apache.org/r/72820/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72804: Supported chown CSI volumes in the `volume/csi` isolator.
> On Aug. 27, 2020, 7:18 a.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > > Lines 439 (patched) > > <https://reviews.apache.org/r/72804/diff/1/?file=2238759#file2238759line439> > > > > Can we just make this the default behavior instead of adding the > > `csi_volume_chown` flag? Yeah, I agree. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72804/#review221726 ------- On Aug. 30, 2020, 9:36 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72804/ > --- > > (Updated Aug. 30, 2020, 9:36 a.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10153 > https://issues.apache.org/jira/browse/MESOS-10153 > > > Repository: mesos > > > Description > --- > > Supported chown CSI volumes in the `volume/csi` isolator. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp > 373b629a984b6d7f207bc65899e32d5f023685ee > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > 02ef1f215eeb0e6436d42146b922d8a48e023607 > > > Diff: https://reviews.apache.org/r/72804/diff/2/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 72804: Supported chown CSI volumes in the `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72804/ --- (Updated Aug. 30, 2020, 9:36 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comment. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Supported chown CSI volumes in the `volume/csi` isolator. Diffs (updated) - src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp 373b629a984b6d7f207bc65899e32d5f023685ee src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 02ef1f215eeb0e6436d42146b922d8a48e023607 Diff: https://reviews.apache.org/r/72804/diff/2/ Changes: https://reviews.apache.org/r/72804/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
> On Aug. 25, 2020, 10:41 p.m., Qian Zhang wrote: > > src/tests/csi_server_tests.cpp > > Lines 457 (patched) > > <https://reviews.apache.org/r/72728/diff/9/?file=2238569#file2238569line457> > > > > It seems we only want to verify the behavior of CSI server but not > > launch a task from end to end, maybe we do not need to start master and > > agent just like what we do in `ROOT_InvalidPluginConfig`? > > Greg Mann wrote: > In this case, we still need the agent so that the CSI plugin can actually > be initialized successfully. > > Another option for this test would be to use a task launch to test this > same thing from end-to-end. I left it as-is because the task launching > boilerplate makes the test much larger, so this version is more concise. WDYT? > > Qian Zhang wrote: > > In this case, we still need the agent so that the CSI plugin can > actually be initialized successfully. > > Could you please elaborate a bit on this? Why do we need agent to make > sure the CSI plugin can actually be initialized? It seems we only need agent > ID when calling `csiServer.get()->start()`, but I think we can pass it a mock > agent ID like what `ROOT_InvalidPluginConfig` does, right? > > Greg Mann wrote: > Plugin initialization makes calls against the agent API in order to > launch the plugin. Without doing that, publish/unpublish calls cannot succeed. I see, thanks! - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221703 --- On Aug. 27, 2020, 2:12 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72728/ > --- > > (Updated Aug. 27, 2020, 2:12 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 8b95611baff2f925910ea46addf462cf9f33071b > 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/11/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
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/#review221714 --- Fix it, then Ship it! src/tests/CMakeLists.txt Lines 236 (patched) <https://reviews.apache.org/r/72728/#comment310766> I think this line should be moved to line 215 below. Ditto for the `src/Makefile.am` below. src/tests/containerizer/volume_csi_isolator_tests.cpp Lines 19 (patched) <https://reviews.apache.org/r/72728/#comment310767> Do we need this? src/tests/containerizer/volume_csi_isolator_tests.cpp Lines 254 (patched) <https://reviews.apache.org/r/72728/#comment310768> Can we add a description for this test? src/tests/containerizer/volume_csi_isolator_tests.cpp Lines 274 (patched) <https://reviews.apache.org/r/72728/#comment310769> Kill this newline. src/tests/containerizer/volume_csi_isolator_tests.cpp Lines 278-282 (patched) <https://reviews.apache.org/r/72728/#comment310770> Can we just do `Try> slave = StartSlave(detector.get(), agentFlags);` instead? src/tests/containerizer/volume_csi_isolator_tests.cpp Lines 541 (patched) <https://reviews.apache.org/r/72728/#comment310771> Do we want to unpublish the volume at the end of this test? Otherwise there will be a mount left on the host filesystem? - Qian Zhang On Aug. 26, 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 Aug. 26, 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 8b95611baff2f925910ea46addf462cf9f33071b > 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/10/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72728: Added tests for the 'volume/csi' isolator.
> On Aug. 25, 2020, 10:41 p.m., Qian Zhang wrote: > > src/tests/csi_server_tests.cpp > > Lines 457 (patched) > > <https://reviews.apache.org/r/72728/diff/9/?file=2238569#file2238569line457> > > > > It seems we only want to verify the behavior of CSI server but not > > launch a task from end to end, maybe we do not need to start master and > > agent just like what we do in `ROOT_InvalidPluginConfig`? > > Greg Mann wrote: > In this case, we still need the agent so that the CSI plugin can actually > be initialized successfully. > > Another option for this test would be to use a task launch to test this > same thing from end-to-end. I left it as-is because the task launching > boilerplate makes the test much larger, so this version is more concise. WDYT? > In this case, we still need the agent so that the CSI plugin can actually be > initialized successfully. Could you please elaborate a bit on this? Why do we need agent to make sure the CSI plugin can actually be initialized? It seems we only need agent ID when calling `csiServer.get()->start()`, but I think we can pass it a mock agent ID like what `ROOT_InvalidPluginConfig` does, right? - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221703 --- On Aug. 26, 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 Aug. 26, 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 8b95611baff2f925910ea46addf462cf9f33071b > 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/10/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72805: Added a test helper for CSI volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72805/#review221713 --- Fix it, then Ship it! src/tests/mesos.hpp Lines 873-874 (patched) <https://reviews.apache.org/r/72805/#comment310764> A newline between. src/tests/mesos.hpp Lines 2097 (patched) <https://reviews.apache.org/r/72805/#comment310765> Do we need another `createVolumeHostPath` in the `internal` namespace at line 1804? - Qian Zhang On Aug. 26, 2020, 1:50 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72805/ > --- > > (Updated Aug. 26, 2020, 1:50 p.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Added a test helper for CSI volumes. > > > Diffs > - > > src/tests/mesos.hpp 8f89d7ca5642a475ecbc176d8bba277a6774a8a1 > > > Diff: https://reviews.apache.org/r/72805/diff/1/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Review Request 72803: Added a new agent flag `--csi_volume_chown`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72803/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Added a new agent flag `--csi_volume_chown`. Diffs - docs/configuration/agent.md 4899202e1afa2988acb3c066829e26aa9612fd29 src/slave/flags.hpp 51770f5d6e8ba853cb5ae2e82739329a26f01ff4 src/slave/flags.cpp 878788c0cb675a16ef69abd4facc8ca89cbd19d3 Diff: https://reviews.apache.org/r/72803/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 72804: Supported chown CSI volumes in the `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72804/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Supported chown CSI volumes in the `volume/csi` isolator. Diffs - src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp 373b629a984b6d7f207bc65899e32d5f023685ee src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 02ef1f215eeb0e6436d42146b922d8a48e023607 Diff: https://reviews.apache.org/r/72804/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72728: Added unit tests for the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221703 --- src/tests/csi_server_tests.cpp Lines 19 (patched) <https://reviews.apache.org/r/72728/#comment310753> It seems we do not need this? src/tests/csi_server_tests.cpp Lines 106-107 (patched) <https://reviews.apache.org/r/72728/#comment310754> A newline between. Ditto for line 111-112. src/tests/csi_server_tests.cpp Lines 403 (patched) <https://reviews.apache.org/r/72728/#comment310758> I do not see how this volume is used by the task, I think we should set the volume into the task's `containerInfo.volumes`. src/tests/csi_server_tests.cpp Lines 408 (patched) <https://reviews.apache.org/r/72728/#comment310757> Can we launch the task to run a command to write to the volume rather than just `exit 0`? src/tests/csi_server_tests.cpp Lines 457 (patched) <https://reviews.apache.org/r/72728/#comment310759> It seems we only want to verify the behavior of CSI server but not launch a task from end to end, maybe we do not need to start master and agent just like what we do in `ROOT_InvalidPluginConfig`? - Qian Zhang On Aug. 25, 2020, 5:20 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72728/ > --- > > (Updated Aug. 25, 2020, 5:20 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 unit tests for the CSI server. > > > Diffs > - > > src/Makefile.am 8b95611baff2f925910ea46addf462cf9f33071b > src/tests/csi_server_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72728/diff/9/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72727: Updated the test CSI plugin for CSI server testing.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72727/#review221702 --- Ship it! Ship It! - Qian Zhang On Aug. 18, 2020, 9:23 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72727/ > --- > > (Updated Aug. 18, 2020, 9:23 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 > --- > > This patch adds additional configuration flags to the > test CSI plugin which are necessary in order to test > the agent's CSI server. > > > Diffs > - > > src/examples/test_csi_plugin.cpp 214a3ee4481512b025ff054db6ffe15d476b0264 > > > Diff: https://reviews.apache.org/r/72727/diff/4/ > > > Testing > --- > > These new flags are used in a subsequent test in this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72788: Introduced the `CSIPluginInfo.target_path_exists` field.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72788/ --- (Updated Aug. 25, 2020, 9:23 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10148 https://issues.apache.org/jira/browse/MESOS-10148 Repository: mesos Description --- Introduced the `CSIPluginInfo.target_path_exists` field. Diffs - include/mesos/mesos.proto 661f746dda9c1d2350867bd9139e73c3e3a34f5d include/mesos/v1/mesos.proto ffe45c305ab46c73f0da5908287aa3eab8ecf5dd src/csi/v1_volume_manager.cpp c05265cd514de745e824c16a0acef276deed1510 Diff: https://reviews.apache.org/r/72788/diff/2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72789: Refactored state recovery in `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72789/ --- (Updated Aug. 25, 2020, 9:21 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10155 https://issues.apache.org/jira/browse/MESOS-10155 Repository: mesos Description --- Read the checkpointed CSI volume state directly in protobuf message way. Diffs - src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 535974bc9ef4049551d7ea7b464bd000deccd7e8 Diff: https://reviews.apache.org/r/72789/diff/3/ Testing --- Thanks, Qian Zhang
Re: Review Request 72789: Refactored state recovery in `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72789/ --- (Updated Aug. 25, 2020, 9:18 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comment. Summary (updated) - Refactored state recovery in `volume/csi` isolator. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Read the checkpointed CSI volume state directly in protobuf message way. Diffs (updated) - src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 535974bc9ef4049551d7ea7b464bd000deccd7e8 Diff: https://reviews.apache.org/r/72789/diff/3/ Changes: https://reviews.apache.org/r/72789/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 72799: Fixed a bug in CSI server initialization.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72799/#review221700 --- Ship it! Ship It! - Qian Zhang On Aug. 25, 2020, 8:16 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72799/ > --- > > (Updated Aug. 25, 2020, 8:16 a.m.) > > > Review request for mesos, Andrei Budnik and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Previously, the CSI server would initialize the service > managers before the auth token was generated, meaning > that requests made by the service managers to an agent > which requires HTTP authentication would fail. > > This patch changes the order of initialization so that > the service managers will be initialized with a valid > auth token when necessary. > > > Diffs > - > > src/slave/csi_server.cpp 0ffe020412dd3170d34052ebbbdc7f320c4cb31a > > > Diff: https://reviews.apache.org/r/72799/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72789: Read the checkpointed CSI volume state directly in protobuf message way.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72789/ --- (Updated Aug. 23, 2020, 10:33 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Read the checkpointed CSI volume state directly in protobuf message way. Diffs - src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 535974bc9ef4049551d7ea7b464bd000deccd7e8 Diff: https://reviews.apache.org/r/72789/diff/2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72789: Read the checkpointed CSI volume state directly in protobuf message way.
> On Aug. 22, 2020, 12:28 a.m., Greg Mann wrote: > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > > Line 201 (original), 201 (patched) > > <https://reviews.apache.org/r/72789/diff/1/?file=2238534#file2238534line201> > > > > Can we do `state::read(volumesPath)` here so that we don't > > need to do any extra JSON/protobuf parsing? Then I think this change may > > not be necessary? Yes, you are right! - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72789/#review221678 --- On Aug. 23, 2020, 10:32 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72789/ > --- > > (Updated Aug. 23, 2020, 10:32 p.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10153 > https://issues.apache.org/jira/browse/MESOS-10153 > > > Repository: mesos > > > Description > --- > > Read the checkpointed CSI volume state directly in protobuf message way. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp > 535974bc9ef4049551d7ea7b464bd000deccd7e8 > > > Diff: https://reviews.apache.org/r/72789/diff/2/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 72789: Read the checkpointed CSI volume state directly in protobuf message way.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72789/ --- (Updated Aug. 23, 2020, 10:32 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comment. Summary (updated) - Read the checkpointed CSI volume state directly in protobuf message way. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description (updated) --- Read the checkpointed CSI volume state directly in protobuf message way. Diffs (updated) - src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 535974bc9ef4049551d7ea7b464bd000deccd7e8 Diff: https://reviews.apache.org/r/72789/diff/2/ Changes: https://reviews.apache.org/r/72789/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72788: Introduced the `CSIPluginInfo.target_path_exists` field.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72788/ --- (Updated Aug. 21, 2020, 10:37 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Changed the solution by introducing a protobuf message field `CSIPluginInfo.target_path_exists`. Summary (updated) - Introduced the `CSIPluginInfo.target_path_exists` field. Bugs: MESOS-10150 https://issues.apache.org/jira/browse/MESOS-10150 Repository: mesos Description (updated) --- Introduced the `CSIPluginInfo.target_path_exists` field. Diffs (updated) - include/mesos/mesos.proto 661f746dda9c1d2350867bd9139e73c3e3a34f5d include/mesos/v1/mesos.proto ffe45c305ab46c73f0da5908287aa3eab8ecf5dd src/csi/v1_volume_manager.cpp c05265cd514de745e824c16a0acef276deed1510 Diff: https://reviews.apache.org/r/72788/diff/2/ Changes: https://reviews.apache.org/r/72788/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221672 --- Ship it! Ship It! - Qian Zhang On Aug. 21, 2020, 7:14 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 21, 2020, 7:14 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Initialized plugins lazily in the CSI server. > > > Diffs > - > > src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 > src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a > > > Diff: https://reviews.apache.org/r/72779/diff/4/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
> On Aug. 20, 2020, 10:17 a.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Lines 280 (patched) > > <https://reviews.apache.org/r/72779/diff/3/?file=2238503#file2238503line284> > > > > So here we erase the plugin while `plugin.initialized` is already > > associated at line 249 and `CSIServerProcess::publishVolume()` or > > `CSIServerProcess::unpublishVolume` may already be waiting on > > `plugins.at(name).initialized.future()`, will the erasing cause any > > problems? > > Greg Mann wrote: > We should be OK here since this callback is deferred onto the CSI server > actor. If this `repair()` callback is invoked, that means that > `plugins.at(name).initialized` has failed, which means that any callbacks > chained onto `plugins.at(name).initialized` by `publishVolume()` or > `unpublishVolume()` will be executed. By the time the deferred `repair()` > callback is executed on the CSI server actor, failures will have already been > returned for any previously-pending publish/unpublish calls. Does that make > sense? Yeah, I agree. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221653 --- On Aug. 21, 2020, 7:14 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 21, 2020, 7:14 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Initialized plugins lazily in the CSI server. > > > Diffs > - > > src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 > src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a > > > Diff: https://reviews.apache.org/r/72779/diff/4/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72789: Checkpointed CSI volume state in stringified JSON.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72789/ --- (Updated Aug. 20, 2020, 5:16 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Checkpointed CSI volume state in stringified JSON. Diffs - src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 535974bc9ef4049551d7ea7b464bd000deccd7e8 Diff: https://reviews.apache.org/r/72789/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 72789: Checkpointed CSI volume state in stringified JSON.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72789/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Checkpointed CSI volume state in stringified JSON. Diffs - src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp 535974bc9ef4049551d7ea7b464bd000deccd7e8 Diff: https://reviews.apache.org/r/72789/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 72788: Added a work around in v1 volume manager for Portworx CSI plugin.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72788/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10150 https://issues.apache.org/jira/browse/MESOS-10150 Repository: mesos Description --- Added a work around in v1 volume manager for Portworx CSI plugin. Diffs - src/csi/v1_volume_manager.cpp c05265cd514de745e824c16a0acef276deed1510 Diff: https://reviews.apache.org/r/72788/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
> On Aug. 20, 2020, 2:41 p.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Lines 280-282 (patched) > > <https://reviews.apache.org/r/72779/diff/3/?file=2238503#file2238503line284> > > > > I think we should log an error message here, otherwise when > > `initializePlugin()` is called in `start()` we will not know which plugin > > is failed to initialize. And it seems we do not have a log message to state that a plugin has been successfully initialized which I think would be helpful for debuggability. So maybe we should change this `repair()` method to `onAny()` where we can log an INFO message for the successful plugin initialization and still do `plugins.erase(name)` on any failures, WDYT? - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221655 --- On Aug. 20, 2020, 3:04 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 20, 2020, 3:04 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Initialized plugins lazily in the CSI server. > > > Diffs > - > > src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 > src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a > > > Diff: https://reviews.apache.org/r/72779/diff/3/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221655 --- src/slave/csi_server.cpp Line 169 (original), 219 (patched) <https://reviews.apache.org/r/72779/#comment310724> I'd suggest to rename this `name` local var to something like `_name` since we already have a `name` parameter in this method. And when the `name` parameter is specified, I think we should only initialize the plugin of that name rather than initializing all the loaded plugins, right? Otherwise a plugin may be intialized multiple times in the runtime. src/slave/csi_server.cpp Lines 280-282 (patched) <https://reviews.apache.org/r/72779/#comment310723> I think we should log an error message here, otherwise when `initializePlugin()` is called in `start()` we will not know which plugin is failed to initialize. - Qian Zhang On Aug. 20, 2020, 3:04 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 20, 2020, 3:04 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Initialized plugins lazily in the CSI server. > > > Diffs > - > > src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 > src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a > > > Diff: https://reviews.apache.org/r/72779/diff/3/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
> On Aug. 19, 2020, 3:43 p.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Line 136 (original), 155 (patched) > > <https://reviews.apache.org/r/72779/diff/2/?file=2238466#file2238466line158> > > > > If `name` is specified, do we need to do this `os::ls`? I think we can > > just load and initialize the specified plugin directly without traversing > > the entries in `pluginConfigDir`, right? > > Greg Mann wrote: > Are we going to introduce a convention for the naming of the files in > that directory? If not, how will we load the correct file on the first try? Ah, you are right, we have to do the `os::ls` here. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221636 --- On Aug. 20, 2020, 3:04 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 20, 2020, 3:04 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Initialized plugins lazily in the CSI server. > > > Diffs > - > > src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 > src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a > > > Diff: https://reviews.apache.org/r/72779/diff/3/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221653 --- src/slave/csi_server.cpp Lines 186 (patched) <https://reviews.apache.org/r/72779/#comment310720> I'd suggest to add `path` in this error message so we could know which plugin config file has a problem. Ditto for the `Protobuf parse failed ...` error message below. src/slave/csi_server.cpp Lines 280 (patched) <https://reviews.apache.org/r/72779/#comment310721> So here we erase the plugin while `plugin.initialized` is already associated at line 249 and `CSIServerProcess::publishVolume()` or `CSIServerProcess::unpublishVolume` may already be waiting on `plugins.at(name).initialized.future()`, will the erasing cause any problems? src/slave/csi_server.cpp Lines 308 (patched) <https://reviews.apache.org/r/72779/#comment310719> Kill this newline. Ditto for the similar code in `CSIServerProcess::publishVolume` and `CSIServerProcess::unpublishVolume` - Qian Zhang On Aug. 20, 2020, 3:04 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 20, 2020, 3:04 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Initialized plugins lazily in the CSI server. > > > Diffs > - > > src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 > src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a > > > Diff: https://reviews.apache.org/r/72779/diff/3/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72761: Added the CSI server to the Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72761/#review221652 --- Ship it! Ship It! - Qian Zhang On Aug. 20, 2020, 5:48 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72761/ > --- > > (Updated Aug. 20, 2020, 5:48 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > --- > > This patch adds a CSI server to the Mesos agent in both > the agent binary and in tests. > > > Diffs > - > > src/local/local.cpp 895057027165609c792089cd336988b65a3b305d > src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 > src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d > src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 > src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d > src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 > src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 > src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed > src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 > src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 > > > Diff: https://reviews.apache.org/r/72761/diff/5/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72761: Added the CSI server to the Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72761/#review221637 --- Ship it! Ship It! - Qian Zhang On Aug. 19, 2020, 2:16 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72761/ > --- > > (Updated Aug. 19, 2020, 2:16 p.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > --- > > This patch adds a CSI server to the Mesos agent in both > the agent binary and in tests. > > > Diffs > - > > src/local/local.cpp 895057027165609c792089cd336988b65a3b305d > src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 > src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d > src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 > src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d > src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 > src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 > src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed > src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 > src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 > > > Diff: https://reviews.apache.org/r/72761/diff/4/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221636 --- src/slave/csi_server.cpp Lines 130-132 (patched) <https://reviews.apache.org/r/72779/#comment310706> I see this method will always be called with name specifed in `CSIServerProcess::publishVolume()` and `CSIServerProcess::unpublishVolume()`. So in which case will it be called with no name specified? src/slave/csi_server.cpp Line 127 (original), 145 (patched) <https://reviews.apache.org/r/72779/#comment310707> When is this hashmap populated? I see what is populated in `CSIServerProcess::initializePlugin()` is a local variable with the exactly same name. src/slave/csi_server.cpp Line 132 (original), 149 (patched) <https://reviews.apache.org/r/72779/#comment310709> So basically this method does two things: load the plugin config files and initialize the plugins. I would suggest we still do the former in `CSIServer::create()` so if there is any errors (like `JSON::parse()` or `protobuf::parse()` fails) in the plugin config files we can error out eariler, otherwise users may only find the error in the runtime and they have to fix the error with restarting agent. src/slave/csi_server.cpp Line 136 (original), 155 (patched) <https://reviews.apache.org/r/72779/#comment310708> If `name` is specified, do we need to do this `os::ls`? I think we can just load and initialize the specified plugin directly without traversing the entries in `pluginConfigDir`, right? - Qian Zhang On Aug. 19, 2020, 2:23 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 19, 2020, 2:23 p.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > This patch updates the CSI server to attempt to > initialize unknown plugins when it receives publish > or unpublish calls meant for plugins that it does > not recognize. > > > Diffs > - > > src/slave/csi_server.hpp f5ec7663926731483589c5e30e7060751ed01e55 > src/slave/csi_server.cpp 2ba4f22b92370b019722d845d6113fb37f1b876a > > > Diff: https://reviews.apache.org/r/72779/diff/2/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72761: Added the CSI server to the Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72761/#review221632 --- src/slave/slave.cpp Lines 1747 (patched) <https://reviews.apache.org/r/72761/#comment310702> As we discussed in https://reviews.apache.org/r/72759/#comment310644 , this should be `csiServer->start(info.id())` because we need to expose agent ID to managed CSI plugin. Ditto for the code in `Slave::reregistered`. And `Slave::reregistered` can be called multiple times when agent is running, so I think we need this logic https://github.com/apache/mesos/blob/1.10.0/src/resource_provider/daemon.cpp#L165:L176 in `CSIServer::start()` as well rather than always generating auth token every time when `CSIServer::start()` is called. WDYT? - Qian Zhang On Aug. 18, 2020, 1:13 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72761/ > --- > > (Updated Aug. 18, 2020, 1:13 p.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > --- > > This patch adds a CSI server to the Mesos agent in both > the agent binary and in tests. > > > Diffs > - > > src/local/local.cpp 895057027165609c792089cd336988b65a3b305d > src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 > src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d > src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 > src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d > src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 > src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 > src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed > src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 > src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 > > > Diff: https://reviews.apache.org/r/72761/diff/3/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72759: Exposed Mesos agent ID to managed CSI plugins.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72759/ --- (Updated Aug. 19, 2020, 10:52 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Minor changes. Bugs: MESOS-10175 https://issues.apache.org/jira/browse/MESOS-10175 Repository: mesos Description --- Exposed Mesos agent ID to managed CSI plugins. Diffs (updated) - src/csi/service_manager.hpp 76a80fb793d2293e56dec221da23290df6a4acc6 src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 src/resource_provider/storage/provider.cpp 0a8dc26e66db0242474bcbbd0b2ff9cec81c58f5 src/slave/csi_server.cpp b435d5db194ffad736838a5b98494944d3264045 Diff: https://reviews.apache.org/r/72759/diff/3/ Changes: https://reviews.apache.org/r/72759/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 72759: Improved CSI service manager to set node ID for managed CSI plugins.
> On Aug. 14, 2020, 6:21 a.m., Greg Mann wrote: > > src/csi/service_manager.cpp > > Lines 740 (patched) > > <https://reviews.apache.org/r/72759/diff/1/?file=2237900#file2237900line740> > > > > Where does this env var name come from, 'MESOS_NODE_ID'? > > Qian Zhang wrote: > Orginially I wanted to just name it `NODE_ID`, however I think it is too > generic and may be conflict with other env var used by the CSI plugin. So I > changed to add a `MESOS_` prefix just like other env vars that Mesos sets for > containers, e.g. `MESOS_FRAMEWORK_ID`, `MESOS_SLAVE_ID`, > `MESOS_AGENT_ENDPOINT`. Or maybe we should name it `MESOS_AGENT_IP`? > > Greg Mann wrote: > Hm, I think this may not yield a good ID for the agent, since I think > it's possible that the value of `self().address.ip` could be the same across > all nodes in the cluster - I think it could be `0.0.0.0`/`INADDR_ANY`, for > example. Do you think we need to inject the Mesos agent ID into the service > manager so that we can use it as the node ID here? I think `self().address.ip` could not be `0.0.0.0/INADDR_ANY`, see https://github.com/apache/mesos/blob/1.10.0/3rdparty/libprocess/src/process.cpp#L1142:L1159 for details. > Do you think we need to inject the Mesos agent ID into the service manager so > that we can use it as the node ID here? Yeah, that's my original thought, but it may involve a bit more code changes in some other components, like CSI server, SLRP. But after second thought I think it should be OK and makes more sense to use Mesos agent ID. I have updated this patch accordingly. And could you please update https://reviews.apache.org/r/72761 by passing Mesos agent ID to CSI server (e.g. as a parameter of `CSIServer::start()`). - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72759/#review221574 --- On Aug. 12, 2020, 7:47 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72759/ > --- > > (Updated Aug. 12, 2020, 7:47 p.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10175 > https://issues.apache.org/jira/browse/MESOS-10175 > > > Repository: mesos > > > Description > --- > > Improved CSI service manager to set node ID for managed CSI plugins. > > > Diffs > - > > src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 > > > Diff: https://reviews.apache.org/r/72759/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 72759: Exposed Mesos agent ID to managed CSI plugins.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72759/ --- (Updated Aug. 19, 2020, 12:22 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comment. Summary (updated) - Exposed Mesos agent ID to managed CSI plugins. Bugs: MESOS-10175 https://issues.apache.org/jira/browse/MESOS-10175 Repository: mesos Description (updated) --- Exposed Mesos agent ID to managed CSI plugins. Diffs (updated) - src/csi/service_manager.hpp 76a80fb793d2293e56dec221da23290df6a4acc6 src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 src/resource_provider/storage/provider.cpp 0a8dc26e66db0242474bcbbd0b2ff9cec81c58f5 src/slave/csi_server.cpp b435d5db194ffad736838a5b98494944d3264045 Diff: https://reviews.apache.org/r/72759/diff/2/ Changes: https://reviews.apache.org/r/72759/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72754: Enabled the `volume/csi` isolator in `MesosContainerizer`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72754/ --- (Updated Aug. 18, 2020, 2:59 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comments. Bugs: MESOS-10156 https://issues.apache.org/jira/browse/MESOS-10156 Repository: mesos Description --- Enabled the `volume/csi` isolator in `MesosContainerizer`. Diffs (updated) - src/slave/containerizer/containerizer.hpp 2b3c4c0363a6362c62695f087ea25248657d99df src/slave/containerizer/containerizer.cpp 9e44e5e0a41e48121f147778295a07b10b03bcf2 src/slave/containerizer/mesos/containerizer.hpp 56e4c49733ac4e236b81ebf9e3238bf8a189c9f2 src/slave/containerizer/mesos/containerizer.cpp 3c1840cae4befe09581ea4efae855d552bd19b05 Diff: https://reviews.apache.org/r/72754/diff/3/ Changes: https://reviews.apache.org/r/72754/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 72761: Added the CSI server to the Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72761/#review221616 --- src/slave/slave.cpp Lines 1746-1749 (original), 1746-1751 (patched) <https://reviews.apache.org/r/72761/#comment310685> So do you want to implement `CSIServer::start()` in the way that I mentioned in this comment (https://reviews.apache.org/r/72779/#comment310664 )? If yes, then I think `CSIServer::start()` do not need to return anything (just a void method), and here we do not need to handle its result. - Qian Zhang On Aug. 18, 2020, 1:13 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72761/ > --- > > (Updated Aug. 18, 2020, 1:13 p.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > --- > > This patch adds a CSI server to the Mesos agent in both > the agent binary and in tests. > > > Diffs > - > > src/local/local.cpp 895057027165609c792089cd336988b65a3b305d > src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 > src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d > src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 > src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d > src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 > src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 > src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed > src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 > src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 > > > Diff: https://reviews.apache.org/r/72761/diff/3/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72761: Added the CSI server to the Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72761/#review221611 --- src/slave/main.cpp Lines 583 (patched) <https://reviews.apache.org/r/72761/#comment310681> I think it should be `--ip_discovery_command` or `--ip` flag, right? https://github.com/apache/mesos/blob/1.10.0/src/slave/main.cpp#L328:L338 Ditto for the code in `src/tests/cluster.cpp`. src/slave/slave.cpp Lines 1746-1749 (patched) <https://reviews.apache.org/r/72761/#comment310682> We need to do this in `Slave::reregistered()` as well, because when agent is restarted, it will reregister rather than register with master in which case we want to call `CSIServer::start()` too. And if we decide to implement `CSIServer::start()` in the way that I mentioned in this comment (https://reviews.apache.org/r/72779/#comment310664), then I think `CSIServer::start()` do not need to return anything (just a void method), and here we do not need to handle its result. - Qian Zhang On Aug. 18, 2020, 9:25 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72761/ > --- > > (Updated Aug. 18, 2020, 9:25 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > --- > > This patch adds a CSI server to the Mesos agent in both > the agent binary and in tests. > > > Diffs > - > > src/local/local.cpp 895057027165609c792089cd336988b65a3b305d > src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 > src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d > src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 > src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d > src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 > src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 > src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed > src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 > src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 > > > Diff: https://reviews.apache.org/r/72761/diff/2/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Review Request 72781: Updated volume manager to support user specified target path root.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72781/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10150 https://issues.apache.org/jira/browse/MESOS-10150 Repository: mesos Description --- Updated volume manager to support user specified target path root. Diffs - src/csi/v0_volume_manager.cpp 9e840a762d9785ce48e011354dbb10295d9c6b38 src/csi/v0_volume_manager_process.hpp 11629559dff7f17eee489ff3e0e4b9566ff50d59 src/csi/v1_volume_manager.cpp 7230676c2d1ba0cafce9bc6e45c00ece7fb09f0c src/csi/v1_volume_manager_process.hpp 63dc03ee52923d2639c9f8253e84924a8c8897b4 src/slave/csi_server.cpp a9a399579c6f7683b36e72582da20ea39311a222 Diff: https://reviews.apache.org/r/72781/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221599 --- src/slave/csi_server.cpp Line 225 (original), 212 (patched) <https://reviews.apache.org/r/72779/#comment310664> What if this method fails somewhere and returns a `Failure`? As the result `CSIServer::started.future()` will be a failed future and then all the `CSIServer::publishVolume` and `CSIServer::unpublishVolume` calls will just fail in the runtime. This seems unfortunate, since the only purpose of this method is to generate `authToken`, can we just do it in `CSIServer::create` in which way we can error out earlier when agent is just started. And then I think we do not need this method `CSIServerProcess::start()` at all, we can just keep the method `CSIServer::start()` where we can just do `started.set(Nothing());`. WDYT? - Qian Zhang On Aug. 15, 2020, 11:54 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 15, 2020, 11:54 p.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > This patch updates the CSI server to lazily initialize CSI > plugins on-demand when the server receives publish or > unpublish calls for a plugin which is not already > successfully initialized. > > > Diffs > - > > src/slave/csi_server.cpp a9a399579c6f7683b36e72582da20ea39311a222 > > > Diff: https://reviews.apache.org/r/72779/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72779: Initialized plugins lazily in the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72779/#review221596 --- src/slave/csi_server.cpp Lines 119 (patched) <https://reviews.apache.org/r/72779/#comment310660> s/future/promise/ src/slave/csi_server.cpp Lines 171-172 (original), 148-152 (patched) <https://reviews.apache.org/r/72779/#comment310661> Can we change the constructor of `CSIPlugin` by passing `info` into it as well? Then we do not have to assign `info` at line 152. src/slave/csi_server.cpp Lines 204 (patched) <https://reviews.apache.org/r/72779/#comment310662> Will this `repair` get invoked when each of the above `then()` fails? Or it will get invoked only when the last `then()` fails? - Qian Zhang On Aug. 15, 2020, 11:54 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72779/ > --- > > (Updated Aug. 15, 2020, 11:54 p.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > This patch updates the CSI server to lazily initialize CSI > plugins on-demand when the server receives publish or > unpublish calls for a plugin which is not already > successfully initialized. > > > Diffs > - > > src/slave/csi_server.cpp a9a399579c6f7683b36e72582da20ea39311a222 > > > Diff: https://reviews.apache.org/r/72779/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*CSIServerTest*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 72761: Added the CSI server to the Mesos agent.
> On Aug. 13, 2020, 10:42 a.m., Qian Zhang wrote: > > src/slave/slave.hpp > > Lines 892 (patched) > > <https://reviews.apache.org/r/72761/diff/1/?file=2237909#file2237909line892> > > > > Why does slave need CSI server as its member? I think slave needs to call the `start()` method of CSI server. - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72761/#review221560 --- On Aug. 13, 2020, 3:19 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72761/ > --- > > (Updated Aug. 13, 2020, 3:19 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > --- > > This patch adds a CSI server to the Mesos agent in both > the agent binary and in tests. > > > Diffs > - > > src/local/local.cpp 895057027165609c792089cd336988b65a3b305d > src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367 > src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d > src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547 > src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d > src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0 > src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253 > src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed > src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8 > src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0 > > > Diff: https://reviews.apache.org/r/72761/diff/1/ > > > Testing > --- > > Testing details at the end of this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72759: Improved CSI service manager to set node ID for managed CSI plugins.
> On Aug. 14, 2020, 6:21 a.m., Greg Mann wrote: > > src/csi/service_manager.cpp > > Lines 740 (patched) > > <https://reviews.apache.org/r/72759/diff/1/?file=2237900#file2237900line740> > > > > Where does this env var name come from, 'MESOS_NODE_ID'? Orginially I wanted to just name it `NODE_ID`, however I think it is too generic and may be conflict with other env var used by the CSI plugin. So I changed to add a `MESOS_` prefix just like other env vars that Mesos sets for containers, e.g. `MESOS_FRAMEWORK_ID`, `MESOS_SLAVE_ID`, `MESOS_AGENT_ENDPOINT`. Or maybe we should name it `MESOS_AGENT_IP`? - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72759/#review221574 --- On Aug. 12, 2020, 7:47 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72759/ > --- > > (Updated Aug. 12, 2020, 7:47 p.m.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10175 > https://issues.apache.org/jira/browse/MESOS-10175 > > > Repository: mesos > > > Description > --- > > Improved CSI service manager to set node ID for managed CSI plugins. > > > Diffs > - > > src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 > > > Diff: https://reviews.apache.org/r/72759/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Review Request 72770: Updated the help message of the agent flag `--csi_plugin_config_dir`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72770/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10151 https://issues.apache.org/jira/browse/MESOS-10151 Repository: mesos Description --- This is to make the help message of the agent flag `--csi_plugin_config_dir` aligned with the latest protobuf message `CSIPluginInfo`. Diffs - docs/configuration/agent.md e8608d977652baf02b4474bb48d3f4598cfa5f3f src/slave/flags.cpp 02a5568f29927e767029a53f05f3b3e97ed0d1f8 Diff: https://reviews.apache.org/r/72770/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72761: Added the CSI server to the Mesos agent.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72761/#review221563
---
src/slave/main.cpp
Lines 570 (patched)
<https://reviews.apache.org/r/72761/#comment310629>
This line cannot compile:
```
../../src/slave/main.cpp: In function ‘int main(int, char**)’:
../../src/slave/main.cpp:570:9: error: ‘flags’ is not a member of
‘process::network::openssl’
if (process::network::openssl::flags().enabled) {
```
I used the following configuration parameters, anything missed?
```
../configure --disable-libtool-wrappers --disable-python --disable-java
--enable-ssl --enable-libevent --enable-debug
```
- Qian Zhang
On Aug. 13, 2020, 3:19 a.m., Greg Mann wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72761/
> ---
>
> (Updated Aug. 13, 2020, 3:19 a.m.)
>
>
> Review request for mesos and Qian Zhang.
>
>
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch adds a CSI server to the Mesos agent in both
> the agent binary and in tests.
>
>
> Diffs
> -
>
> src/local/local.cpp 895057027165609c792089cd336988b65a3b305d
> src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367
> src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d
> src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547
> src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d
> src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0
> src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253
> src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed
> src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8
> src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0
>
>
> Diff: https://reviews.apache.org/r/72761/diff/1/
>
>
> Testing
> ---
>
> Testing details at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72754: Enabled the `volume/csi` isolator in `MesosContainerizer`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72754/ --- (Updated Aug. 13, 2020, 2:14 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Minor fix. Bugs: MESOS-10156 https://issues.apache.org/jira/browse/MESOS-10156 Repository: mesos Description --- Enabled the `volume/csi` isolator in `MesosContainerizer`. Diffs (updated) - src/slave/containerizer/containerizer.hpp 2b3c4c0363a6362c62695f087ea25248657d99df src/slave/containerizer/containerizer.cpp 9e44e5e0a41e48121f147778295a07b10b03bcf2 src/slave/containerizer/mesos/containerizer.hpp 56e4c49733ac4e236b81ebf9e3238bf8a189c9f2 src/slave/containerizer/mesos/containerizer.cpp 3c1840cae4befe09581ea4efae855d552bd19b05 Diff: https://reviews.apache.org/r/72754/diff/2/ Changes: https://reviews.apache.org/r/72754/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72760: Passed the CSI server into the Mesos containerizer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72760/#review221561 --- Ship it! Ship It! - Qian Zhang On Aug. 13, 2020, 3:21 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72760/ > --- > > (Updated Aug. 13, 2020, 3:21 a.m.) > > > Review request for mesos and Qian Zhang. > > > Bugs: MESOS-10163 > https://issues.apache.org/jira/browse/MESOS-10163 > > > Repository: mesos > > > Description > --- > > Passed the CSI server into the Mesos containerizer. > > > Diffs > - > > src/slave/containerizer/containerizer.cpp > 9e44e5e0a41e48121f147778295a07b10b03bcf2 > > > Diff: https://reviews.apache.org/r/72760/diff/1/ > > > Testing > --- > > > Thanks, > > Greg Mann > >
Re: Review Request 72761: Added the CSI server to the Mesos agent.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72761/#review221560
---
src/slave/main.cpp
Lines 577 (patched)
<https://reviews.apache.org/r/72761/#comment310626>
This seems not correct to me,
`process::network::inet4::Address::ANY_ANY().ip` is something like `0.0.0.0`,
right?
I think here we should use the actual IP the agent listens on, I see we set
the env var `LIBPROCESS_IP` at line 336, maybe we should use that as the agent
IP here? And if `os::getenv("LIBPROCESS_IP")` returns `None` then we should use
`process::network::inet4::Address::LOOPBACK_ANY().ip`.
Ditto for the same code in `src/tests/cluster.cpp`.
src/slave/slave.hpp
Lines 892 (patched)
<https://reviews.apache.org/r/72761/#comment310627>
Why does slave need CSI server as its member?
src/tests/cluster.hpp
Lines 59 (patched)
<https://reviews.apache.org/r/72761/#comment310628>
This line should be put below the line `#include "slave/constants.hpp"`.
- Qian Zhang
On Aug. 13, 2020, 3:19 a.m., Greg Mann wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72761/
> ---
>
> (Updated Aug. 13, 2020, 3:19 a.m.)
>
>
> Review request for mesos and Qian Zhang.
>
>
> Bugs: MESOS-10163
> https://issues.apache.org/jira/browse/MESOS-10163
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch adds a CSI server to the Mesos agent in both
> the agent binary and in tests.
>
>
> Diffs
> -
>
> src/local/local.cpp 895057027165609c792089cd336988b65a3b305d
> src/slave/main.cpp 0aa2cc9d254d0b74485a0584b9f55c2266f29367
> src/slave/slave.hpp 2cf45c616d5c50508bc5cd77096b0fef86542b1d
> src/slave/slave.cpp c828d99ca9909b904767cc9ea5671b462714f547
> src/tests/cluster.hpp 415a60f5fd4881bca329ffd718f58d676653a17d
> src/tests/cluster.cpp ab4cea49eb3e8cf18d320fbb30beaa3c454a08b0
> src/tests/mesos.hpp 0ad0999fba185539c90b053bda43be001da26253
> src/tests/mesos.cpp d6933a60c55802e6f9e89242608fbfad7631faed
> src/tests/mock_slave.hpp 58daefa4459eaf15c2394f14db9f94527984b2b8
> src/tests/mock_slave.cpp fa2a0f5c8dadeb8aea87815741bc4fe071d5fbe0
>
>
> Diff: https://reviews.apache.org/r/72761/diff/1/
>
>
> Testing
> ---
>
> Testing details at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Review Request 72759: Improved CSI service manager to set node ID for managed CSI plugins.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72759/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10175 https://issues.apache.org/jira/browse/MESOS-10175 Repository: mesos Description --- Improved CSI service manager to set node ID for managed CSI plugins. Diffs - src/csi/service_manager.cpp 7a8d8e5dc3c3bae5251284652d73b33ca554f783 Diff: https://reviews.apache.org/r/72759/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72728: Added a unit test for the CSI server.
> On Aug. 12, 2020, 11:18 a.m., Qian Zhang wrote: > > src/tests/csi_server_tests.cpp > > Lines 114-121 (patched) > > <https://reviews.apache.org/r/72728/diff/3/?file=2237793#file2237793line114> > > > > It seems the required flag > > [--endpoint](https://github.com/apache/mesos/blob/1.10.0/src/examples/test_csi_plugin.cpp#L130:L132) > > is not set here? Sorry, the link to the `--endpoint` should be https://github.com/apache/mesos/blob/1.10.0/src/examples/test_csi_plugin.cpp#L130:L132 - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221553 --- On Aug. 11, 2020, 11:08 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72728/ > --- > > (Updated Aug. 11, 2020, 11:08 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 > --- > > The new test 'CSIServerTest.ROOT_LoadPlugin' verifies that > the CSI server can load a plugin and execute publish and > unpublish calls against it. > > > Diffs > - > > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/tests/csi_server_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72728/diff/3/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72728: Added a unit test for the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221553 --- src/tests/csi_server_tests.cpp Lines 96 (patched) <https://reviews.apache.org/r/72728/#comment310621> Why do we need this parameter? src/tests/csi_server_tests.cpp Lines 114-121 (patched) <https://reviews.apache.org/r/72728/#comment310620> It seems the required flag [--endpoint](https://github.com/apache/mesos/blob/1.10.0/src/examples/test_csi_plugin.cpp#L130:L132) is not set here? - Qian Zhang On Aug. 11, 2020, 11:08 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72728/ > --- > > (Updated Aug. 11, 2020, 11:08 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 > --- > > The new test 'CSIServerTest.ROOT_LoadPlugin' verifies that > the CSI server can load a plugin and execute publish and > unpublish calls against it. > > > Diffs > - > > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/tests/csi_server_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72728/diff/3/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72727: Updated the test CSI plugin for CSI server testing.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72727/#review221554 --- src/examples/test_csi_plugin.cpp Lines 174-177 (patched) <https://reviews.apache.org/r/72727/#comment310622> Why do we need this flag? Without this flag, would the unit test in https://reviews.apache.org/r/72728 be affected? src/examples/test_csi_plugin.cpp Line 1590 (original), 1614 (patched) <https://reviews.apache.org/r/72727/#comment310623> I think this will break the case when `--volume_id_path` is set to true, in which case `volumeInfo.id` is already set to `getVolumePath(capacity, name)` in the constructor and now here we call `getVolumePath` again which will give us a wrong volume path. - Qian Zhang On Aug. 12, 2020, 2:55 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72727/ > --- > > (Updated Aug. 12, 2020, 2:55 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 > --- > > This patch adds additional configuration flags to the > test CSI plugin which are necessary in order to test > the agent's CSI server. > > > Diffs > - > > src/examples/test_csi_plugin.cpp 214a3ee4481512b025ff054db6ffe15d476b0264 > > > Diff: https://reviews.apache.org/r/72727/diff/3/ > > > Testing > --- > > These new flags are used in a subsequent test in this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72728: Added a unit test for the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221552 --- src/tests/csi_server_tests.cpp Lines 89-92 (original) <https://reviews.apache.org/r/72728/#comment310619> Instead of removing this, I think we may need to implement the `TearDown` method to do some cleanup like: https://github.com/apache/mesos/blob/1.10.0/src/tests/storage_local_resource_provider_tests.cpp#L163:L187 WDYT? - Qian Zhang On Aug. 11, 2020, 11:08 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72728/ > --- > > (Updated Aug. 11, 2020, 11:08 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 > --- > > The new test 'CSIServerTest.ROOT_LoadPlugin' verifies that > the CSI server can load a plugin and execute publish and > unpublish calls against it. > > > Diffs > - > > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/tests/csi_server_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72728/diff/3/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72727: Updated the test CSI plugin for CSI server testing.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72727/#review221544 --- src/examples/test_csi_plugin.cpp Lines 1589-1590 (original), 1613-1614 (patched) <https://reviews.apache.org/r/72727/#comment310599> Here we assume the volume ID is the volume path, but that is not true when `--volume_id_path` is false. In that case, the volume ID and volume path will be different, so I think the mount operation here will fail. - Qian Zhang On Aug. 11, 2020, 11:07 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72727/ > --- > > (Updated Aug. 11, 2020, 11:07 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 > --- > > This patch adds additional configuration flags to the > test CSI plugin which are necessary in order to test > the agent's CSI server. > > > Diffs > - > > src/examples/test_csi_plugin.cpp 214a3ee4481512b025ff054db6ffe15d476b0264 > > > Diff: https://reviews.apache.org/r/72727/diff/2/ > > > Testing > --- > > These new flags are used in a subsequent test in this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72733: Implemented the `prepare` method of `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72733/ --- (Updated Aug. 11, 2020, 4:32 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comments. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Implemented the `prepare` method of `volume/csi` isolator. Diffs (updated) - include/mesos/mesos.proto 0f91d88abba4bd49b46676e956211f1cf4e7219b include/mesos/v1/mesos.proto f25db8aeefd721653b2027282abbae8f2e6a40f3 src/CMakeLists.txt c60d98a21bbd422a5ee1c22f7281bc701e5ade14 src/Makefile.am 49dab4b6488b75d32e6ee9d54d68afe36549c353 src/common/validation.cpp 14a8c7b9d70c303b527e5e43012999471ced2d78 src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/state.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/state.proto PRE-CREATION Diff: https://reviews.apache.org/r/72733/diff/5/ Changes: https://reviews.apache.org/r/72733/diff/4-5/ Testing --- Thanks, Qian Zhang
Re: Review Request 72753: Implemented the `recover` method of `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72753/ --- (Updated Aug. 11, 2020, 4:31 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Addressed review comments. Bugs: MESOS-10155 https://issues.apache.org/jira/browse/MESOS-10155 Repository: mesos Description --- Implemented the `recover` method of `volume/csi` isolator. Diffs (updated) - src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION Diff: https://reviews.apache.org/r/72753/diff/2/ Changes: https://reviews.apache.org/r/72753/diff/1-2/ Testing --- Thanks, Qian Zhang
Re: Review Request 72732: Added support for secrets to the CSI volume managers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72732/#review221534 --- Ship it! Ship It! - Qian Zhang On Aug. 11, 2020, 12:20 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72732/ > --- > > (Updated Aug. 11, 2020, 12:20 p.m.) > > > Review request for mesos, Andrei Budnik and Qian Zhang. > > > Bugs: MESOS-10168 > https://issues.apache.org/jira/browse/MESOS-10168 > > > Repository: mesos > > > Description > --- > > Added support for secrets to the CSI volume managers. > > > Diffs > - > > src/csi/state.proto 836e30c74026ef87896cbb537b14a3b97445bea1 > src/csi/v0_volume_manager.hpp 93183c2b0187ceb41533c7e022f2a5ef1f23d9a1 > src/csi/v0_volume_manager.cpp 89a6da5ffbcc30040a1ec33ffc2895e8bdec3423 > src/csi/v0_volume_manager_process.hpp > 7548c43aa982759296f1413866f14b5280404185 > src/csi/v1_volume_manager.hpp 2f7897d416895f4be0efd32bdce90df9049bdf58 > src/csi/v1_volume_manager.cpp 5178b2fd2cb2919fdf6fb3d073ac1f83ac934f56 > src/csi/v1_volume_manager_process.hpp > b8a1ef7d58438633e497f86ff6604664d3a80b8c > src/csi/volume_manager.hpp 57e7c51d0baf296ce5ceb0385a7da0776b3545aa > src/csi/volume_manager.cpp c47adfe426419cf71dc1f2cb435aa241904ed8a2 > > > Diff: https://reviews.apache.org/r/72732/diff/4/ > > > Testing > --- > > > Thanks, > > Greg Mann > >
Re: Review Request 72716: Added implementation of the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72716/#review221531 --- Ship it! Ship It! - Qian Zhang On Aug. 11, 2020, 10:30 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72716/ > --- > > (Updated Aug. 11, 2020, 10:30 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 implementation of the CSI server. > > > Diffs > - > > src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c > src/slave/csi_server.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72716/diff/6/ > > > Testing > --- > > Details at the end of this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72732: Added support for secrets to the CSI volume managers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72732/#review221520 --- src/csi/v0_volume_manager_process.hpp Lines 186 (patched) <https://reviews.apache.org/r/72732/#comment310576> I do not see how this secret resolver is used in v0 volume manager. src/csi/v1_volume_manager.cpp Lines 998 (patched) <https://reviews.apache.org/r/72732/#comment310575> s/Map/Map&/ And we should use `process::defer()` here, right? Ditto for the node stage volume call below. src/csi/v1_volume_manager.cpp Lines 1316 (patched) <https://reviews.apache.org/r/72732/#comment310577> Is it possible for `secretResolver` to be null? src/csi/volume_manager.hpp Lines 68 (patched) <https://reviews.apache.org/r/72732/#comment310568> I see in https://reviews.apache.org/r/72716 (csi_server.cpp#L223), you already passed `secretResolver` as a parameter of `VolumeManager::create`, so I think this patch should be moved before that patch in the chain, otherwise that patch cannot compile: ``` ../../../src/slave/csi_server.cpp: In lambda function: ../../../src/slave/csi_server.cpp:231:27: error: no matching function for call to 'mesos::csi::VolumeManager::create(const string&, const mesos::CSIPluginInfo&, hashset, const string&, process::grpc::client::Runtime&, mesos::csi::ServiceManager*, mesos::csi::Metrics*, mesos::SecretResolver*&)' secretResolver); ^ In file included from ../../../src/slave/csi_server.cpp:47:0: ../../../src/csi/volume_manager.hpp:58:45: note: candidate: static Try > mesos::csi::VolumeManager::create(const string&, const mesos::CSIPluginInfo&, const hashset&, const string&, const process::grpc::client::Runtime&, mesos::csi::ServiceManager*, mesos::csi::Metrics*) static Try> create( ^ ../../../src/csi/volume_manager.hpp:58:45: note: candidate expects 7 arguments, 8 provided ``` - Qian Zhang On Aug. 7, 2020, 3:03 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72732/ > --- > > (Updated Aug. 7, 2020, 3:03 p.m.) > > > Review request for mesos, Andrei Budnik and Qian Zhang. > > > Bugs: MESOS-10168 > https://issues.apache.org/jira/browse/MESOS-10168 > > > Repository: mesos > > > Description > --- > > Added support for secrets to the CSI volume managers. > > > Diffs > - > > src/csi/state.proto 836e30c74026ef87896cbb537b14a3b97445bea1 > src/csi/v0_volume_manager.hpp 93183c2b0187ceb41533c7e022f2a5ef1f23d9a1 > src/csi/v0_volume_manager.cpp 89a6da5ffbcc30040a1ec33ffc2895e8bdec3423 > src/csi/v0_volume_manager_process.hpp > 7548c43aa982759296f1413866f14b5280404185 > src/csi/v1_volume_manager.hpp 2f7897d416895f4be0efd32bdce90df9049bdf58 > src/csi/v1_volume_manager.cpp 5178b2fd2cb2919fdf6fb3d073ac1f83ac934f56 > src/csi/v1_volume_manager_process.hpp > b8a1ef7d58438633e497f86ff6604664d3a80b8c > src/csi/volume_manager.hpp 57e7c51d0baf296ce5ceb0385a7da0776b3545aa > src/csi/volume_manager.cpp c47adfe426419cf71dc1f2cb435aa241904ed8a2 > > > Diff: https://reviews.apache.org/r/72732/diff/3/ > > > Testing > --- > > > Thanks, > > Greg Mann > >
Re: Review Request 72727: Updated the test CSI plugin for CSI server testing.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72727/#review221524 --- src/examples/test_csi_plugin.cpp Lines 232-235 (original), 249-254 (patched) <https://reviews.apache.org/r/72727/#comment310574> It seems when `volumeIdPath` is set to `false` we will call `mkdir` to create directory for the volume under the current working directory? But I think we should create it under `TestCSIPlugin::workDir` just like what we did when `volumeIdPath` is set to `true`. - Qian Zhang On Aug. 1, 2020, 3:06 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72727/ > --- > > (Updated Aug. 1, 2020, 3:06 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 > --- > > This patch adds additional configuration flags to the > test CSI plugin which are necessary in order to test > the agent's CSI server. > > > Diffs > - > > src/examples/test_csi_plugin.cpp 6202173844a93b8d9642208bd86839b3cf56bce2 > > > Diff: https://reviews.apache.org/r/72727/diff/1/ > > > Testing > --- > > These new flags are used in a subsequent test in this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72728: Added a unit test for the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72728/#review221523 --- src/Makefile.am Lines 2831 (patched) <https://reviews.apache.org/r/72728/#comment310571> It seems we need 3 more tabs between `csi_server_tests.cpp` and ``. src/tests/csi_server_tests.cpp Lines 22-23 (patched) <https://reviews.apache.org/r/72728/#comment310572> These two lines should be put above the line `#include `. src/tests/csi_server_tests.cpp Lines 92 (patched) <https://reviews.apache.org/r/72728/#comment310573> I do not see how `slaveWorkDirs` is used afterward. - Qian Zhang On Aug. 7, 2020, 3:02 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72728/ > --- > > (Updated Aug. 7, 2020, 3:02 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 > --- > > The new test 'CSIServerTest.ROOT_LoadPlugin' verifies that > the CSI server can load a plugin and execute publish and > unpublish calls against it. > > > Diffs > - > > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/tests/csi_server_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72728/diff/2/ > > > Testing > --- > > `sudo make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 72716: Added implementation of the CSI server.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72716/#review221517 --- Ship it! Ship It! - Qian Zhang On Aug. 7, 2020, 3 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72716/ > --- > > (Updated Aug. 7, 2020, 3 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 implementation of the CSI server. > > > Diffs > - > > src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c > src/slave/csi_server.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72716/diff/5/ > > > Testing > --- > > Details at the end of this chain. > > > Thanks, > > Greg Mann > >
Re: Review Request 72716: Added implementation of the CSI server.
> On Aug. 7, 2020, 8:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 200 (patched)
> > <https://reviews.apache.org/r/72716/diff/5/?file=2237439#file2237439line200>
> >
> > Do we need to include `info.type()` in the container prefix? Otherwise
> > the container prefix for all the managed CSI plugins will be same which may
> > not be good for debugging.
>
> Greg Mann wrote:
> The plugin name and type already get added into the container ID after
> the prefix, so I think the current code is sufficient:
> https://github.com/apache/mesos/blob/c78dc333fc893a43d40dc33299a61987198a6ea9/src/csi/service_manager.cpp#L117-L121
>
> WDYT?
Yeah, you are right!
> On Aug. 7, 2020, 8:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 336 (patched)
> > <https://reviews.apache.org/r/72716/diff/5/?file=2237439#file2237439line336>
> >
> > Do we need to check if `volume/csi` isolator is enabled? Like:
> > ```
> > if (!strings::contains(flags.isolation, "volume/csi")) {
> > return Error("...");
> > }
> > ```
> >
> > I think to make the whole CSI volume feature work, both `CSIServer` and
> > `volume/csi` isolator need to be enabled.
> >
> > And in which condition will we call `CSIServer::create` to create
> > `CSISever`? When `--csi_plugin_config_dir` is specified? If so, I think
> > here we need a `CHECK` rather than an `if`.
>
> Greg Mann wrote:
> I agree that we should check for the CSI isolator. However, I don't think
> we should use a CHECK within this `create()` method. The `Try` return type
> provides the perfect mechanism for surfacing any failures, which we can
> handle in the agent.
Yeah, I agree. So could you please add the check for the CSI isolator here?
And I guess we will call `CSIServer::create` to create CSI sever **only** when
`--csi_plugin_config_dir` is specified, right?
> On Aug. 7, 2020, 8:01 p.m., Qian Zhang wrote:
> > src/slave/csi_server.cpp
> > Lines 413-415 (patched)
> > <https://reviews.apache.org/r/72716/diff/5/?file=2237439#file2237439line413>
> >
> > Do we need to define `started` as a promise here? Can we just define it
> > as a future?
> > ```
> > started = process::dispatch(process.get(), ::start);
> > return started;
> > ```
>
> Greg Mann wrote:
> Yep, we can't use a simple Future because we don't initiate startup
> during server construction, as is the case in the volume manager.
> `started.associate()` allows us to initiate startup using the existing Future
> within the Promise. If we use a raw Future, then the only way to initiate
> startup is to overwrite the stored Future, onto which some publish/unpublish
> calls may have already been chained.
I see, thank you!
- Qian
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72716/#review221499
---
On Aug. 7, 2020, 3 p.m., Greg Mann wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72716/
> ---
>
> (Updated Aug. 7, 2020, 3 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 implementation of the CSI server.
>
>
> Diffs
> -
>
> src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea
> src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89
> src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c
> src/slave/csi_server.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/72716/diff/5/
>
>
> Testing
> ---
>
> Details at the end of this chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 72733: Implemented the `prepare` method of `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72733/ --- (Updated Aug. 10, 2020, 10:37 a.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Rebased. Bugs: MESOS-10153 https://issues.apache.org/jira/browse/MESOS-10153 Repository: mesos Description --- Implemented the `prepare` method of `volume/csi` isolator. Diffs (updated) - include/mesos/mesos.proto 0f91d88abba4bd49b46676e956211f1cf4e7219b include/mesos/v1/mesos.proto f25db8aeefd721653b2027282abbae8f2e6a40f3 src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 src/common/validation.cpp 14a8c7b9d70c303b527e5e43012999471ced2d78 src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/state.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/state.proto PRE-CREATION Diff: https://reviews.apache.org/r/72733/diff/4/ Changes: https://reviews.apache.org/r/72733/diff/3-4/ Testing --- Thanks, Qian Zhang
Re: Review Request 72716: Added implementation of the CSI server.
> On July 29, 2020, 11:27 p.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Lines 233-235 (patched) > > <https://reviews.apache.org/r/72716/diff/2/?file=2236468#file2236468line233> > > > > I am just curious what would happen if any of the initialization logic > > fail, how will the failure be propogated back? > > Greg Mann wrote: > I updated the server so that now `start()` returns a future associated > with the initialization. > > Qian Zhang wrote: > I see. And I guess `CSIServer::start()` will be called in > `Slave::registered` and `Slave::reregistered`, right? I am just wondering how > we are going to handle the returned future there. Are we going to register an > `onAny` callback and log an error message if it is a failed future? > > Greg Mann wrote: > Yea I think we have to decide how to handle failures of CSI server > initialization. I might propose a timeout in the agent, after which we log an > error? And we could provide a task status message perhaps when task launches > fail because the CSI server failed to initialize? > > In any case, I think the interface offered by the current patch set will > be sufficient to let us handle the failed initialization case, WDYT? > > Qian Zhang wrote: > I took a look at the code of local resource provider daemon and I found > it just log an error message in its `start` method: > > https://github.com/apache/mesos/blob/1.10.0/src/slave/slave.cpp#L1740:L1742 > > https://github.com/apache/mesos/blob/1.10.0/src/resource_provider/daemon.cpp#L188:L191 > > Do you think if we can do the similar? > > Greg Mann wrote: > I was worried about maintaining ordering of calls made before startup, > but as we discussed offline, I think I was being too paranoid :) > > I'm switching the patch to the kind of approach used in the daemon. > > Greg Mann wrote: > lol sorry I replied on the wrong issue :) > > I think we can log an error message as well, but it does also seem > appropriate to me to return a future associated with the startup. We can > decide what to do with that future in the agent, but might as well return it? > > Greg Mann wrote: > I decided not to log an error in the server, I think the proper place to > log would be at the callsite where we handle the future returned by > `start()`, WDYT? > > Qian Zhang wrote: > So do you mean we should log an error in `Slave::registered` and > `Slave::reregistered`? > > Greg Mann wrote: > It depends on how exactly we initialize the CSI server in the agent; I'm > not sure that it would necessarily be in those functions. In any case, since > `start()` returns a Future I don't think we need to log anything here in the > server, unless we think there are multiple classes of failures, some of which > we would want to crash for and some of which we would want to just log? IMHO we need to call `CSIServer::start` in `Slave::registered` and `Slave::reregistered` (i.e. right after the agent's state is changed to `RUNNING`), do we have any other options? - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72716/#review221405 --- On Aug. 7, 2020, 3 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72716/ > --- > > (Updated Aug. 7, 2020, 3 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 implementation of the CSI server. > > > Diffs > - > > src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c > src/slave/csi_server.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72716/diff/5/ > > > Testing > --- > > Details at the end of this chain. > > > Thanks, > > Greg Mann > >
Review Request 72754: Enabled the `volume/csi` isolator in `MesosContainerizer`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72754/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10156 https://issues.apache.org/jira/browse/MESOS-10156 Repository: mesos Description --- Enabled the `volume/csi` isolator in `MesosContainerizer`. Diffs - src/slave/containerizer/containerizer.hpp 2b3c4c0363a6362c62695f087ea25248657d99df src/slave/containerizer/containerizer.cpp 9e44e5e0a41e48121f147778295a07b10b03bcf2 src/slave/containerizer/mesos/containerizer.hpp 56e4c49733ac4e236b81ebf9e3238bf8a189c9f2 src/slave/containerizer/mesos/containerizer.cpp 3c1840cae4befe09581ea4efae855d552bd19b05 Diff: https://reviews.apache.org/r/72754/diff/1/ Testing --- Thanks, Qian Zhang
Review Request 72753: Implemented the `recover` method of `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72753/ --- Review request for mesos, Andrei Budnik and Greg Mann. Bugs: MESOS-10155 https://issues.apache.org/jira/browse/MESOS-10155 Repository: mesos Description --- Implemented the `recover` method of `volume/csi` isolator. Diffs - src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION Diff: https://reviews.apache.org/r/72753/diff/1/ Testing --- Thanks, Qian Zhang
Re: Review Request 72690: Implemented the framework and `create` method of `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72690/ --- (Updated Aug. 8, 2020, 11:58 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Added a missing include. Bugs: MESOS-10152 https://issues.apache.org/jira/browse/MESOS-10152 Repository: mesos Description --- Implemented the framework and `create` method of `volume/csi` isolator. Diffs (updated) - src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/paths.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/paths.cpp PRE-CREATION Diff: https://reviews.apache.org/r/72690/diff/10/ Changes: https://reviews.apache.org/r/72690/diff/9-10/ Testing --- Thanks, Qian Zhang
Re: Review Request 72734: Implemented the `cleanup` method of `volume/csi` isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72734/ --- (Updated Aug. 8, 2020, 11:18 p.m.) Review request for mesos, Andrei Budnik and Greg Mann. Changes --- Minor changes. Bugs: MESOS-10154 https://issues.apache.org/jira/browse/MESOS-10154 Repository: mesos Description --- Implemented the `cleanup` method of `volume/csi` isolator. Diffs (updated) - src/slave/containerizer/mesos/isolators/volume/csi/isolator.hpp PRE-CREATION src/slave/containerizer/mesos/isolators/volume/csi/isolator.cpp PRE-CREATION Diff: https://reviews.apache.org/r/72734/diff/3/ Changes: https://reviews.apache.org/r/72734/diff/2-3/ Testing --- Thanks, Qian Zhang
Re: Review Request 72716: Added implementation of the CSI server.
> On July 29, 2020, 11:27 p.m., Qian Zhang wrote: > > src/slave/csi_server.cpp > > Lines 233-235 (patched) > > <https://reviews.apache.org/r/72716/diff/2/?file=2236468#file2236468line233> > > > > I am just curious what would happen if any of the initialization logic > > fail, how will the failure be propogated back? > > Greg Mann wrote: > I updated the server so that now `start()` returns a future associated > with the initialization. > > Qian Zhang wrote: > I see. And I guess `CSIServer::start()` will be called in > `Slave::registered` and `Slave::reregistered`, right? I am just wondering how > we are going to handle the returned future there. Are we going to register an > `onAny` callback and log an error message if it is a failed future? > > Greg Mann wrote: > Yea I think we have to decide how to handle failures of CSI server > initialization. I might propose a timeout in the agent, after which we log an > error? And we could provide a task status message perhaps when task launches > fail because the CSI server failed to initialize? > > In any case, I think the interface offered by the current patch set will > be sufficient to let us handle the failed initialization case, WDYT? > > Qian Zhang wrote: > I took a look at the code of local resource provider daemon and I found > it just log an error message in its `start` method: > > https://github.com/apache/mesos/blob/1.10.0/src/slave/slave.cpp#L1740:L1742 > > https://github.com/apache/mesos/blob/1.10.0/src/resource_provider/daemon.cpp#L188:L191 > > Do you think if we can do the similar? > > Greg Mann wrote: > I was worried about maintaining ordering of calls made before startup, > but as we discussed offline, I think I was being too paranoid :) > > I'm switching the patch to the kind of approach used in the daemon. > > Greg Mann wrote: > lol sorry I replied on the wrong issue :) > > I think we can log an error message as well, but it does also seem > appropriate to me to return a future associated with the startup. We can > decide what to do with that future in the agent, but might as well return it? > > Greg Mann wrote: > I decided not to log an error in the server, I think the proper place to > log would be at the callsite where we handle the future returned by > `start()`, WDYT? So do you mean we should log an error in `Slave::registered` and `Slave::reregistered`? - Qian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72716/#review221405 --- On Aug. 7, 2020, 3 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72716/ > --- > > (Updated Aug. 7, 2020, 3 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 implementation of the CSI server. > > > Diffs > - > > src/CMakeLists.txt 4e15e3d99aa2cce2403fe07e762fef2fb4a27dea > src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 > src/slave/csi_server.hpp 17882e1be5a6c38ca34d7b50d4a6041530e8908c > src/slave/csi_server.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/72716/diff/5/ > > > Testing > --- > > Details at the end of this chain. > > > Thanks, > > Greg Mann > >
