You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/10/27 22:15:53 UTC

Review Request 63377: Added filesystem layout for storage resource providers.

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

Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.


Bugs: MESOS-8141
    https://issues.apache.org/jira/browse/MESOS-8141


Repository: mesos


Description
-------

A storage resource provider can now store the following persistent CSI
data in `<work_dir>/csi/resource_providers/<type>/<name>`:

1. Mount points of CSI volumes: `volumes/<volume_id>`
2. States of CSI volumes: `states/<volume_id>/state`
3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
   would be symlinked by `plugins/<plugin_name>/endpoint`.


Diffs
-----

  src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
  src/resource_provider/storage/paths.hpp PRE-CREATION 
  src/resource_provider/storage/paths.cpp PRE-CREATION 
  src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3 


Diff: https://reviews.apache.org/r/63377/diff/1/


Testing
-------

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63377: Added filesystem layout for CSI plugins.

Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.

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

(Updated Nov. 29, 2017, 3:15 a.m.)


Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
-------

Name changes and added a parse helper function.


Bugs: MESOS-8141
    https://issues.apache.org/jira/browse/MESOS-8141


Repository: mesos


Description
-------

A CSI plugin can now store the following persistent CSI
data in `<work_dir>/csi/<type>/<name>/`:

1. Mount points of CSI volumes: `volumes/<volume_id>`
2. States of CSI volumes: `states/<volume_id>/state`
3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
   would be symlinked by `containers/<container_id>/endpoint`.


Diffs (updated)
-----

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/csi/paths.hpp PRE-CREATION 
  src/csi/paths.cpp PRE-CREATION 
  src/slave/paths.hpp 66dfa4544772d78ccc9229dc861da60c79913f24 
  src/slave/paths.cpp b03ffeeed83cb73228cca27769262fb08df38fb5 


Diff: https://reviews.apache.org/r/63377/diff/6/

Changes: https://reviews.apache.org/r/63377/diff/5-6/


Testing
-------

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63377: Added filesystem layout for CSI plugins.

Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.

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

(Updated Nov. 28, 2017, 8:06 p.m.)


Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
-------

Renamed a file for consistency.


Bugs: MESOS-8141
    https://issues.apache.org/jira/browse/MESOS-8141


Repository: mesos


Description
-------

A CSI plugin can now store the following persistent CSI
data in `<work_dir>/csi/<type>/<name>/`:

1. Mount points of CSI volumes: `volumes/<volume_id>`
2. States of CSI volumes: `states/<volume_id>/state`
3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
   would be symlinked by `containers/<container_id>/endpoint`.


Diffs (updated)
-----

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/csi/paths.hpp PRE-CREATION 
  src/csi/paths.cpp PRE-CREATION 
  src/slave/paths.hpp 66dfa4544772d78ccc9229dc861da60c79913f24 
  src/slave/paths.cpp b03ffeeed83cb73228cca27769262fb08df38fb5 


Diff: https://reviews.apache.org/r/63377/diff/5/

Changes: https://reviews.apache.org/r/63377/diff/4-5/


Testing
-------

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63377: Added filesystem layout for CSI plugins.

Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.

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

(Updated Nov. 24, 2017, 6:48 p.m.)


Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
-------

Addressed jieyu's comment.


Bugs: MESOS-8141
    https://issues.apache.org/jira/browse/MESOS-8141


Repository: mesos


Description
-------

A CSI plugin can now store the following persistent CSI
data in `<work_dir>/csi/<type>/<name>/`:

1. Mount points of CSI volumes: `volumes/<volume_id>`
2. States of CSI volumes: `states/<volume_id>/state`
3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
   would be symlinked by `containers/<container_id>/endpoint`.


Diffs (updated)
-----

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/csi/paths.hpp PRE-CREATION 
  src/csi/paths.cpp PRE-CREATION 
  src/slave/paths.hpp 66dfa4544772d78ccc9229dc861da60c79913f24 
  src/slave/paths.cpp b03ffeeed83cb73228cca27769262fb08df38fb5 


Diff: https://reviews.apache.org/r/63377/diff/4/

Changes: https://reviews.apache.org/r/63377/diff/3-4/


Testing
-------

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63377: Added filesystem layout for CSI plugins.

Posted by Jie Yu <yu...@gmail.com>.

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


Fix it, then Ship it!





src/csi/paths.cpp
Lines 149 (patched)
<https://reviews.apache.org/r/63377/#comment269746>

    Can you add a comment about why you want to do that (checking socket path length)? It not quite obvious from the code.


- Jie Yu


On Nov. 21, 2017, 5:36 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63377/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 5:36 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8141
>     https://issues.apache.org/jira/browse/MESOS-8141
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A CSI plugin can now store the following persistent CSI
> data in `<work_dir>/csi/<type>/<name>/`:
> 
> 1. Mount points of CSI volumes: `volumes/<volume_id>`
> 2. States of CSI volumes: `states/<volume_id>/state`
> 3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
>    would be symlinked by `containers/<container_id>/endpoint`.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
>   src/csi/paths.hpp PRE-CREATION 
>   src/csi/paths.cpp PRE-CREATION 
>   src/slave/paths.hpp 66dfa4544772d78ccc9229dc861da60c79913f24 
>   src/slave/paths.cpp b03ffeeed83cb73228cca27769262fb08df38fb5 
> 
> 
> Diff: https://reviews.apache.org/r/63377/diff/3/
> 
> 
> Testing
> -------
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63377: Added filesystem layout for CSI plugins.

Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.

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

(Updated Nov. 21, 2017, 5:36 a.m.)


Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
-------

Moved the helper functions to namespace `mesos::csi::paths` and rebased on r63904.


Summary (updated)
-----------------

Added filesystem layout for CSI plugins.


Bugs: MESOS-8141
    https://issues.apache.org/jira/browse/MESOS-8141


Repository: mesos


Description (updated)
-------

A CSI plugin can now store the following persistent CSI
data in `<work_dir>/csi/<type>/<name>/`:

1. Mount points of CSI volumes: `volumes/<volume_id>`
2. States of CSI volumes: `states/<volume_id>/state`
3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
   would be symlinked by `containers/<container_id>/endpoint`.


Diffs (updated)
-----

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/csi/paths.hpp PRE-CREATION 
  src/csi/paths.cpp PRE-CREATION 
  src/slave/paths.hpp 66dfa4544772d78ccc9229dc861da60c79913f24 
  src/slave/paths.cpp b03ffeeed83cb73228cca27769262fb08df38fb5 


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

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


Testing
-------

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63377: Added filesystem layout for storage resource providers.

Posted by Joseph Wu <jo...@mesosphere.io>.

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




src/resource_provider/storage/paths.hpp
Lines 31-34 (patched)
<https://reviews.apache.org/r/63377/#comment268679>

    Consider changing this comment to refer back to `src/slave/paths.hpp`.  Specifically `getResourceProviderAgentRootDir`.
    
    ```
    //   root (<work_dir>/resource_providers/<type>/<name>/)
    ```
    
    It looks like the `rootDir` parameters below all expect an input which includes those elements.



src/resource_provider/storage/paths.cpp
Lines 87-88 (patched)
<https://reviews.apache.org/r/63377/#comment268680>

    It's still a good idea to use `os::temp` even if that means some configurations will fail to create unix sockets.
    
    Instead of hardcoding `/tmp`, just check if the resulting path is too long and error out here.


- Joseph Wu


On Nov. 3, 2017, 5:58 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63377/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2017, 5:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8141
>     https://issues.apache.org/jira/browse/MESOS-8141
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A storage resource provider can now store the following persistent CSI
> data in `<work_dir>/resource_providers/<type>/<name>/csi`:
> 
> 1. Mount points of CSI volumes: `volumes/<volume_id>`
> 2. States of CSI volumes: `states/<volume_id>/state`
> 3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
>    would be symlinked by `plugins/<plugin_name>/endpoint`.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 1c97b1fd8151f87c4e9e6d62884b0ef7d582c312 
>   src/resource_provider/storage/paths.hpp PRE-CREATION 
>   src/resource_provider/storage/paths.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63377/diff/2/
> 
> 
> Testing
> -------
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63377: Added filesystem layout for storage resource providers.

Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.

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

(Updated Nov. 4, 2017, 12:58 a.m.)


Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
-------

Addressed Jie's comments.


Bugs: MESOS-8141
    https://issues.apache.org/jira/browse/MESOS-8141


Repository: mesos


Description (updated)
-------

A storage resource provider can now store the following persistent CSI
data in `<work_dir>/resource_providers/<type>/<name>/csi`:

1. Mount points of CSI volumes: `volumes/<volume_id>`
2. States of CSI volumes: `states/<volume_id>/state`
3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
   would be symlinked by `plugins/<plugin_name>/endpoint`.


Diffs (updated)
-----

  src/Makefile.am 1c97b1fd8151f87c4e9e6d62884b0ef7d582c312 
  src/resource_provider/storage/paths.hpp PRE-CREATION 
  src/resource_provider/storage/paths.cpp PRE-CREATION 


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

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


Testing
-------

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63377: Added filesystem layout for storage resource providers.

Posted by Jie Yu <yu...@gmail.com>.

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




src/Makefile.am
Lines 1441-1442 (patched)
<https://reviews.apache.org/r/63377/#comment266627>

    This change seems irrelevant?



src/resource_provider/storage/paths.hpp
Lines 32 (patched)
<https://reviews.apache.org/r/63377/#comment266628>

    Any reason we need a top level `csi` directory?
    
    In fact, I'd much prefer:
    ```
    <root>/resource_providers/<type>/<name>/csi/plugins...
    ```



src/resource_provider/storage/paths.cpp
Lines 66 (patched)
<https://reviews.apache.org/r/63377/#comment266629>

    See my comment below.



src/resource_provider/storage/paths.cpp
Lines 139-143 (patched)
<https://reviews.apache.org/r/63377/#comment266631>

    This is not really a `create` method because it'll return the realpath if it exists.
    
    I suggest we have a dead simple `createCsiPluginDirectory` method that just does the `mkdir`. Or we don't even need that. I think doing `os::mkdir(getCsiPluginPath(workDIr))` in the caller is probably better.
    
    Then, move all symlink related logic to:
    ```
    Try<string> getCsiEndpointPath()
    ```



src/resource_provider/storage/paths.cpp
Lines 145-148 (patched)
<https://reviews.apache.org/r/63377/#comment266630>

    Let's factor this into a helper (`getCsiPluginPath`). You don't have to expose if other files are not using it.


- Jie Yu


On Oct. 27, 2017, 10:15 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63377/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2017, 10:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8141
>     https://issues.apache.org/jira/browse/MESOS-8141
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A storage resource provider can now store the following persistent CSI
> data in `<work_dir>/csi/resource_providers/<type>/<name>`:
> 
> 1. Mount points of CSI volumes: `volumes/<volume_id>`
> 2. States of CSI volumes: `states/<volume_id>/state`
> 3. Directory to place CSI endpoints: `/tmp/mesos-csi-XXXXXX`, which
>    would be symlinked by `plugins/<plugin_name>/endpoint`.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am b60a54a031260de6f1fb43584ae5083df2dc7e31 
>   src/resource_provider/storage/paths.hpp PRE-CREATION 
>   src/resource_provider/storage/paths.cpp PRE-CREATION 
>   src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3 
> 
> 
> Diff: https://reviews.apache.org/r/63377/diff/1/
> 
> 
> Testing
> -------
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>