You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2020/09/09 02:33:12 UTC
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 72845: Added doc for the `volume/csi` isolator.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72845/#review222005
-----------------------------------------------------------
Patch looks great!
Reviews applied: [72845]
Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/jenkins/buildbot.sh
- Mesos Reviewbot
On Oct. 8, 2020, 7:57 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72845/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2020, 7:57 a.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 72845: Added doc for the `volume/csi` isolator.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/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.
Posted by Qian Zhang <zh...@gmail.com>.
> 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 72845: Added doc for the `volume/csi` isolator.
Posted by Andrei Sekretenko <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72845/#review222019
-----------------------------------------------------------
Fix it, then Ship it!
Looks almost good to go!
Found two locations where more clarity would IMO be beneficial, and also a couple of very minor typo-like issues.
docs/isolators/csi-volume.md
Lines 51 (patched)
<https://reviews.apache.org/r/72845/#comment311092>
s/mount onto/mount them onto/?
docs/isolators/csi-volume.md
Lines 82 (patched)
<https://reviews.apache.org/r/72845/#comment311093>
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?
docs/isolators/csi-volume.md
Lines 255-256 (patched)
<https://reviews.apache.org/r/72845/#comment311094>
Hmmm... this place is hard to grasp for a person not closely familiar with the CSI External Volumes.
Shouldn't that be
s/of one CSI plugin configuration file **under** the directory/of one **of the** CSI plugin configuration file**s** **in** the directory/?
Not sure I'm fully getting the intended meaning; please check that what I suggest is what you actually intended.
docs/isolators/csi-volume.md
Lines 271-272 (patched)
<https://reviews.apache.org/r/72845/#comment311095>
s/a absolute/an absolute/
- Andrei Sekretenko
On Oct. 8, 2020, 9:57 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72845/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2020, 9:57 a.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 72845: Added doc for the `volume/csi` isolator.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/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 72845: Added doc for the `volume/csi` isolator.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72845/#review221827
-----------------------------------------------------------
Patch looks great!
Reviews applied: [72845]
Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/jenkins/buildbot.sh
- Mesos Reviewbot
On Sept. 9, 2020, 4:43 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72845/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2020, 4:43 a.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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72845: Added doc for the `volume/csi` isolator.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72845/#review221978
-----------------------------------------------------------
docs/isolators/csi-volume.md
Lines 29-31 (patched)
<https://reviews.apache.org/r/72845/#comment311039>
Instead of "The benefit of...", I would recommend something like "Building CSI support allows Mesos to make use of the quickly-growing CSI ecosystem."
docs/isolators/csi-volume.md
Lines 33 (patched)
<https://reviews.apache.org/r/72845/#comment311041>
s/in Mesos/in the Mesos/
docs/isolators/csi-volume.md
Lines 34 (patched)
<https://reviews.apache.org/r/72845/#comment311040>
s/plugins implement/plugins to implement/
docs/isolators/csi-volume.md
Lines 38 (patched)
<https://reviews.apache.org/r/72845/#comment311042>
s/plugins do/plugins that do/
docs/isolators/csi-volume.md
Lines 41 (patched)
<https://reviews.apache.org/r/72845/#comment311044>
I would recommend:
s/So we need/Mesos 1.11.0 provides/
docs/isolators/csi-volume.md
Lines 51 (patched)
<https://reviews.apache.org/r/72845/#comment311045>
s/call CSI/call the CSI/
docs/isolators/csi-volume.md
Lines 53 (patched)
<https://reviews.apache.org/r/72845/#comment311046>
s/call CSI/call the CSI/
docs/isolators/csi-volume.md
Lines 55 (patched)
<https://reviews.apache.org/r/72845/#comment311047>
s/call CSI/call the CSI/
docs/isolators/csi-volume.md
Lines 57 (patched)
<https://reviews.apache.org/r/72845/#comment311048>
s/volumes/volume/
docs/isolators/csi-volume.md
Lines 62 (patched)
<https://reviews.apache.org/r/72845/#comment311049>
s/require controller/require the controller/
docs/isolators/csi-volume.md
Lines 63 (patched)
<https://reviews.apache.org/r/72845/#comment311050>
s/to node/to the node/
docs/isolators/csi-volume.md
Lines 70 (patched)
<https://reviews.apache.org/r/72845/#comment311051>
s/configure/configure the/
docs/isolators/csi-volume.md
Lines 71 (patched)
<https://reviews.apache.org/r/72845/#comment311052>
s/the CSI/CSI/
docs/isolators/csi-volume.md
Lines 80 (patched)
<https://reviews.apache.org/r/72845/#comment311053>
s/Configuring CSI/Configure the CSI/
docs/isolators/csi-volume.md
Lines 97 (patched)
<https://reviews.apache.org/r/72845/#comment311054>
s/Operator/The operator/
docs/isolators/csi-volume.md
Lines 258-259 (patched)
<https://reviews.apache.org/r/72845/#comment311057>
I wonder if it would be helpful here to point out that the fields in `static_provisioning` map directly onto fields in the CSI calls, and if users have questions about these fields they may find more information in the CSI spec and in the documentation for their CSI plugin. WDYT?
docs/isolators/csi-volume.md
Lines 263 (patched)
<https://reviews.apache.org/r/72845/#comment311055>
Instead of referring to "a task without rootfs", would it be more intuitive for readers to say "a task without a container image"?
Similarly, instead of saying "a task with rootfs" below we could say "a task with a container image".
docs/isolators/csi-volume.md
Lines 268-269 (patched)
<https://reviews.apache.org/r/72845/#comment311056>
s/and `container_path` is a relative path/and a relative `container_path`/
The same for the other similar occurrences in this paragraph.
- Greg Mann
On Sept. 9, 2020, 2:43 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72845/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2020, 2:43 a.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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 72845: Added doc for the `volume/csi` isolator.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/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