Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194726
---


Fix it, then Ship it!




It's probably worth taking a look at these docs using the website container 
script in the codebase to ensure that our website's markdown rendering likes it.


docs/csi.md
Lines 65 (patched)


s/resource providing/resource-providing/

s/in Mesos/of Mesos/



docs/csi.md
Lines 66 (patched)


s/hard coded/hard-coded/



docs/csi.md
Lines 78 (patched)


s/HTTP based/HTTP-based/



docs/csi.md
Lines 81 (patched)


s/called/called the/



docs/csi.md
Lines 87 (patched)


s/Resource/The resource/



docs/csi.md
Lines 90 (patched)


s/CSI compatible/CSI-compatible/



docs/csi.md
Lines 101 (patched)


s/long running/long-running/



docs/csi.md
Lines 102 (patched)


s/using/using the/



docs/csi.md
Lines 108 (patched)


s/restart/restarting/



docs/csi.md
Line 152 (original), 206 (patched)


Nit: too many spaces after period.



docs/csi.md
Lines 273 (patched)


s/operates/operate/

s/non `RAW`/non-`RAW`/



docs/csi.md
Line 242 (original), 301 (patched)


s/presence an/presence of an/



docs/csi.md
Lines 290-296 (original), 349-355 (patched)


As you mentioned, inspecting master state is another way to get feedback - 
do we want to explicitly call that out here?



docs/csi.md
Line 328 (original), 371 (patched)


Nit: too many spaces after period.



docs/csi.md
Lines 443-445 (patched)


Nit: I think the linebreaks could be shifted here?



docs/csi.md
Line 442 (original), 491 (patched)


Nit: unnecessary space after "documentation".



docs/csi.md
Line 478 (original), 527 (patched)


s/has/have/



docs/csi.md
Lines 601-602 (patched)


"the old code path has already been removed from the agent"; it's not clear 
to me precisely what you mean here, could you re-word?



docs/operator-http-api.md
Lines 3915 (patched)


Suggestion:

s/Here are the possible responses/Possible responses/

Here and below.



docs/operator-http-api.md
Lines 3917-3921 (patched)


They could also receive a 401 Unauthorized if HTTP authentication fails.

Here and below.



docs/operator-http-api.md
Lines 3921 (patched)


s/If anything goes wrong/If an unexpected error occurs/



docs/operator-http-api.md
Lines 3990 (patched)


See my comment below regarding Bad Request; might want to make the 
explanation for this more generic, since there are other cases in which we 
return 400.

Perhaps "If the request is not well-formed."



docs/operator-http-api.md
Lines 3998-3999 (patched)


"prevents it from being launched again" - as discussed in chat, I think we 
can make this wording a bit more precise.



docs/operator-http-api.md
Lines 4039-4042 (patched)


Looks like we may still return a Bad Request if the content type is not 
present or is malformed, if the body is empty, or if parsing the call fails.


- Greg Mann


On Jan. 3, 2018, 5:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Jan. 3, 2018, 5:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
>   docs/home.md 

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 415 (patched)
> > 
> >
> > this is a protobuf3 type, for which there are specific rules re: 
> > JSON-ification: 
> > https://developers.google.com/protocol-buffers/docs/proto3#json
> > 
> > In particular:
> > > Message field names are mapped to lowerCamelCase and become JSON 
> > object keys. null is accepted and treated as the default value of the 
> > corresponding field type.
> > 
> > This difference is probably important to note. The example provided 
> > continues to use snake_case for protobuf3 json fields (e.g. fs_type vs 
> > fsType).

In fact, I changed it back. See tests here
https://github.com/apache/mesos/blob/master/src/tests/volume_profile_tests.cpp#L394-L438


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194600
---


On Jan. 3, 2018, 5:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Jan. 3, 2018, 5:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
>   docs/home.md 0fa901fce91cc5e385c310f5e441c85678568547 
>   docs/images/csi-architecture.png PRE-CREATION 
>   docs/operator-http-api.md af6a3a6ec7d80859b640c8b7db49f93a803d55d0 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/3/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/
---

(Updated Jan. 3, 2018, 5:09 a.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
Greg Mann, Joseph Wu, and Jan Schlicht.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Added initial doc about CSI support in Mesos.


Diffs (updated)
-

  docs/csi.md PRE-CREATION 
  docs/home.md 0fa901fce91cc5e385c310f5e441c85678568547 
  docs/images/csi-architecture.png PRE-CREATION 
  docs/operator-http-api.md af6a3a6ec7d80859b640c8b7db49f93a803d55d0 


Diff: https://reviews.apache.org/r/64868/diff/3/

Changes: https://reviews.apache.org/r/64868/diff/2-3/


Testing
---

The rendering can be checked here:
https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md


Thanks,

Jie Yu

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194631
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 385 (patched)
> > 
> >
> > I'd like to see it called out clearly in the text (vs. being embedded 
> > in a link) the specific version of CSI that Mesos currently supports. Maybe 
> > this is already called out and I missed it?

added somne text on the top.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194600
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 151 (patched)
> > 
> >
> > Can we make it clear that we're talking about a hypothetical EBS 
> > plugin? AFAIK the external resource provider stuff isn't in place yet (and 
> > may contain unexpected monsters)

I used LVM instead.


> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 369 (patched)
> > 
> >
> > it's worth noting that because this is based on the CSI specification, 
> > there's an implicit dependency between backward compatibility of the module 
> > interface and the CSI specification version. In other words, there's 
> > already a compatiblity scheme for Mesos modules that was crafted prior to 
> > CSI. CSI doesn't provide a backward compatibility promise. As CSI evolves, 
> > this module interface may implicitly evolve in a way that deviates from the 
> > compatibility promise described here: 
> > https://github.com/apache/mesos/blob/master/docs/modules.md#module-versioning-and-backwards-compatibility
> > 
> > .. or, maybe I misunderstand our guarantees re: module backwards compat.

Clarified, module has to be rebuilt against each release of Mesos.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194600
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 137 (patched)
> > 
> >
> > > Any disk resource ... inherits the same profile as the storage pool.
> > 
> > Why, and who enforces this? Is this because a single logical pool may 
> > have multiple, virtual representations (one per profile)? If so it might be 
> > worth differentiating between a logical pool and a virtual one.

I don't want to overcomplicate here. I think the definition is pretty clear "A 
RAW disk resource not backed by a CSI volume is referred to as a storage pool". 
There's a TODO below capture the explanation of correlated resources, which 
will probably clarify that.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194600
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 298 (patched)
> > 
> >
> > this feels like a separate feature. should it be documented elsewhere?

Removed the protobuf because it's not implemented yet. Just mention it is 
coming soon.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194600
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 10:27 p.m., Gaston Kleiman wrote:
> > docs/csi.md
> > Lines 781-783 (patched)
> > 
> >
> > We should probalby say what's the difference between removing an SLRP 
> > and marking it as gone. I guess that  a SLRP can't be re-added after having 
> > marked it as gone.

Did mention below.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194653
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 10:27 p.m., Gaston Kleiman wrote:
> > docs/csi.md
> > Lines 288-291 (patched)
> > 
> >
> > s/framework/scheduler/
> > s/that is available to the frameworks/that is available to them/
> > s/Here are the tips to/A tip to/

I intended to add more tips here. So keep the plural form.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194653
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 7:53 p.m., Greg Mann wrote:
> > Should we mention somewhere that the `--enable-grpc` configure flag must be 
> > set to enable CSI support in the agent?

Good call. Added.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194646
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 109 (patched)
> > 
> >
> > I'm curious: is this accurate as written, or would "must not be set by 
> > frameworks" be more appropriate?
> > 
> > Similar question regarding the comment for `metadata`.

Yeah, clarified that these two fields must not be set by framweorks.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 192 (patched)
> > 
> >
> > Will we duplicate these offer operation docs in the scheduler API 
> > documentation? To avoid duplication, we could instead add these 
> > instructions to the scheduler API docs, and simply have a link here with a 
> > couple sentences explaining what these operations are used for.

the scheduler API doc is not complete currently. I'll leave this as is for now. 
The scheduler API doc (expecially ACCEPT) needs a major improvement, especially 
around `OPERATIONS`.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 289-290 (patched)
> > 
> >
> > Is it better to just say "by looking at the resources in subsequent 
> > offers"? Are there other sources of information schedulers should use?

They could use operator endpoint (e.g., state).


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 291-296 (patched)
> > 
> >
> > Should we also touch on the issue of roles w.r.t. receiving the 
> > converted resource in an offer?
> > 
> > Are volumes created by the new operations similar to persistent volumes 
> > in that they can only be performed on reserved resources?

no, new operation can be used on non-reserved resources.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 298 (patched)
> > 
> >
> > I would recommend simply ommitting this section until we implement it.

OK, i'll mention the limitations and don't mention the protobuf.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 537-539 (patched)
> > 
> >
> > Are these strictly necessary for SLRP support?

Yes. It's unrelated, but these must be specified because the agent code for 
handling the old way has been removed already. add a note.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194631
---


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Gaston Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194653
---




docs/csi.md
Lines 21 (patched)


Another nit: s/third party/third-party/



docs/csi.md
Lines 48 (patched)


s/from the that/from that/



docs/csi.md
Lines 288-291 (patched)


s/framework/scheduler/
s/that is available to the frameworks/that is available to them/
s/Here are the tips to/A tip to/



docs/csi.md
Lines 293 (patched)


Nit: s/  Reservation/ Reservation/



docs/csi.md
Lines 298 (patched)


+1



docs/csi.md
Lines 304 (patched)


s/CSI plugin/a CSI plugin/



docs/csi.md
Lines 529 (patched)


s/SLRP/SLRPs/?



docs/csi.md
Lines 689 (patched)


s/to a directory/in a directory/



docs/csi.md
Lines 690 (patched)


s/set agent flag/set the agent flag/



docs/csi.md
Lines 698 (patched)


s/agent flag/the agent flag/



docs/csi.md
Lines 781-783 (patched)


We should probalby say what's the difference between removing an SLRP and 
marking it as gone. I guess that  a SLRP can't be re-added after having marked 
it as gone.


- Gaston Kleiman


On Dec. 28, 2017, 8:53 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 28, 2017, 8:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194646
---



Should we mention somewhere that the `--enable-grpc` configure flag must be set 
to enable CSI support in the agent?

- Greg Mann


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194631
---




docs/csi.md
Lines 21 (patched)


Nit: s/well defined/well-defined/



docs/csi.md
Lines 25 (patched)


Is "currently" appropriate here? Do we expect this to change? IIUC, this 
feature is distinct from CSI support and still remains in the codebase?



docs/csi.md
Lines 28 (patched)


s/reservation, fair/reservation, and fair/



docs/csi.md
Lines 30 (patched)


Nit: s/Volume Driver/volume driver/



docs/csi.md
Lines 39 (patched)


s/from/from the/



docs/csi.md
Lines 41 (patched)


Suggestion:

s/to have storage vendors write/to allow storage vendors to write/



docs/csi.md
Lines 45 (patched)


s/big/larger/

I would also recommend: s/very//



docs/csi.md
Lines 46 (patched)


Suggestion: s/the users/users/



docs/csi.md
Lines 101 (patched)


Nit: s/plugin specific/plugin-specific/



docs/csi.md
Lines 109 (patched)


I'm curious: is this accurate as written, or would "must not be set by 
frameworks" be more appropriate?

Similar question regarding the comment for `metadata`.



docs/csi.md
Lines 130 (patched)


Suggestion:

s/if the `RAW` disk resource is backed by a CSI Volume or not/whether or 
not the `RAW` disk resource is backed by a CSI Volume/



docs/csi.md
Lines 146 (patched)


s/using/using the/



docs/csi.md
Lines 163 (patched)


For consistency with the scheduler API docs, let's do s/framework 
API/scheduler API/

here and below.



docs/csi.md
Lines 192 (patched)


Will we duplicate these offer operation docs in the scheduler API 
documentation? To avoid duplication, we could instead add these instructions to 
the scheduler API docs, and simply have a link here with a couple sentences 
explaining what these operations are used for.



docs/csi.md
Lines 285 (patched)


either s/disks/disk/ or s/disks/disk's/



docs/csi.md
Lines 289-290 (patched)


Is it better to just say "by looking at the resources in subsequent 
offers"? Are there other sources of information schedulers should use?



docs/csi.md
Lines 291-296 (patched)


Should we also touch on the issue of roles w.r.t. receiving the converted 
resource in an offer?

Are volumes created by the new operations similar to persistent volumes in 
that they can only be performed on reserved resources?



docs/csi.md
Lines 298 (patched)


I would recommend simply ommitting this section until we implement it.



docs/csi.md
Lines 328 (patched)


Nit: s/vendor specific/vendor-specific/

Here and in the comments below.



docs/csi.md
Lines 330 (patched)


Nit: s/low level/low-level/

Here and just below the proto snippet.



docs/csi.md
Lines 360 (patched)


s/system specific/system-specific/



docs/csi.md
Lines 386 (patched)


s/will/will be/



docs/csi.md
Lines 387 (patched)


s/call/call the/



docs/csi.md
Lines 408 (patched)


s/under a/under/



docs/csi.md
Lines 408-409 (patched)


Suggestion:

s/a free-form string-string mapping/arbitrary key-value pairs/



docs/csi.md
Lines 432 (patched)


s/--uri/uri/

Here and below.



docs/csi.md
Lines 433-435 (patched)


Suggestion:

"If the poll interval has elapsed since the last fetch, then the URI is 
re-fetched; otherwise, a cached `ProfileInfo` is returned. If not specified, 
the URI is only fetched once."

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[James DeFelice]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/#review194600
---




docs/csi.md
Lines 137 (patched)


> Any disk resource ... inherits the same profile as the storage pool.

Why, and who enforces this? Is this because a single logical pool may have 
multiple, virtual representations (one per profile)? If so it might be worth 
differentiating between a logical pool and a virtual one.



docs/csi.md
Lines 151 (patched)


Can we make it clear that we're talking about a hypothetical EBS plugin? 
AFAIK the external resource provider stuff isn't in place yet (and may contain 
unexpected monsters)



docs/csi.md
Lines 156 (patched)


s/is likely to/may/



docs/csi.md
Lines 166 (patched)


s/about//



docs/csi.md
Lines 194 (patched)


it may be worth elaborating on the difference between CREATE and 
CREATE_VOLUME. it's a very confusing naming scheme.



docs/csi.md
Lines 242 (patched)


suggest: Currently, Mesos infers the result based on the presence an 
assigned profile in the disk resource.



docs/csi.md
Lines 290 (patched)


s/that is/that are/



docs/csi.md
Lines 298 (patched)


this feels like a separate feature. should it be documented elsewhere?



docs/csi.md
Lines 369 (patched)


it's worth noting that because this is based on the CSI specification, 
there's an implicit dependency between backward compatibility of the module 
interface and the CSI specification version. In other words, there's already a 
compatiblity scheme for Mesos modules that was crafted prior to CSI. CSI 
doesn't provide a backward compatibility promise. As CSI evolves, this module 
interface may implicitly evolve in a way that deviates from the compatibility 
promise described here: 
https://github.com/apache/mesos/blob/master/docs/modules.md#module-versioning-and-backwards-compatibility

.. or, maybe I misunderstand our guarantees re: module backwards compat.



docs/csi.md
Lines 385 (patched)


I'd like to see it called out clearly in the text (vs. being embedded in a 
link) the specific version of CSI that Mesos currently supports. Maybe this is 
already called out and I missed it?



docs/csi.md
Lines 415 (patched)


this is a protobuf3 type, for which there are specific rules re: 
JSON-ification: https://developers.google.com/protocol-buffers/docs/proto3#json

In particular:
> Message field names are mapped to lowerCamelCase and become JSON object 
keys. null is accepted and treated as the default value of the corresponding 
field type.

This difference is probably important to note. The example provided 
continues to use snake_case for protobuf3 json fields (e.g. fs_type vs fsType).


- James DeFelice


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64868/
---

(Updated Dec. 29, 2017, 4:53 a.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
Greg Mann, Joseph Wu, and Jan Schlicht.


Changes
---

Updated.


Repository: mesos


Description
---

Added initial doc about CSI support in Mesos.


Diffs (updated)
-

  docs/csi.md PRE-CREATION 


Diff: https://reviews.apache.org/r/64868/diff/2/

Changes: https://reviews.apache.org/r/64868/diff/1-2/


Testing
---

The rendering can be checked here:
https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md


Thanks,

Jie Yu