Re: Review Request 69541: Added volume gid manager.

2019-01-06 Thread Qian Zhang 

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

(Updated Jan. 7, 2019, 8:26 a.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya Pronin, 
and Jie Yu.


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


Repository: mesos


Description
---

Added volume gid manager.


Diffs
-

  src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
  src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
PRE-CREATION 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 69541: Added volume gid manager.

2018-12-20 Thread Qian Zhang 

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

(Updated Dec. 21, 2018, 10:57 a.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya Pronin, 
and Jie Yu.


Changes
---

Added more logs.


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


Repository: mesos


Description
---

Added volume gid manager.


Diffs (updated)
-

  src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
  src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
PRE-CREATION 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 69541: Added volume gid manager.

2018-12-19 Thread Qian Zhang 

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

(Updated Dec. 20, 2018, 9:35 a.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya Pronin, 
and Jie Yu.


Changes
---

Addressed review comment.


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


Repository: mesos


Description
---

Added volume gid manager.


Diffs (updated)
-

  src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
  src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
PRE-CREATION 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 69541: Added volume gid manager.

2018-12-18 Thread Andrei Budnik 

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


Fix it, then Ship it!




Ship It!


src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
Lines 35 (patched)


`vector` is not used.


- Andrei Budnik


On Dec. 18, 2018, 8:11 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69541/
> ---
> 
> (Updated Dec. 18, 2018, 8:11 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya 
> Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8810
> https://issues.apache.org/jira/browse/MESOS-8810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume gid manager.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69541/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69541: Added volume gid manager.

2018-12-18 Thread Qian Zhang 

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

(Updated Dec. 18, 2018, 4:11 p.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya Pronin, 
and Jie Yu.


Changes
---

Added a new field `totalGids`.


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


Repository: mesos


Description
---

Added volume gid manager.


Diffs (updated)
-

  src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
  src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
PRE-CREATION 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 69541: Added volume gid manager.

2018-12-14 Thread Qian Zhang 


> On Dec. 13, 2018, 8:36 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
> > Lines 100 (patched)
> > 
> >
> > I have a small concern regarding unconditionally resetting permissions: 
> > what if a volume had these flags enabled before we mounted them? Maybe we 
> > should add a TODO comment here for restoring original permissions?

> what if a volume had these flags enabled before we mounted them?

For both shared PV and sandbox_path volumes, they are created by Mesos agent 
rather than pre-created directories.


- Qian


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


On Dec. 13, 2018, 7:46 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69541/
> ---
> 
> (Updated Dec. 13, 2018, 7:46 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya 
> Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8810
> https://issues.apache.org/jira/browse/MESOS-8810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume gid manager.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69541/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69541: Added volume gid manager.

2018-12-13 Thread Andrei Budnik 

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




src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
Lines 100 (patched)


I have a small concern regarding unconditionally resetting permissions: 
what if a volume had these flags enabled before we mounted them? Maybe we 
should add a TODO comment here for restoring original permissions?


- Andrei Budnik


On Dec. 13, 2018, 11:46 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69541/
> ---
> 
> (Updated Dec. 13, 2018, 11:46 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya 
> Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8810
> https://issues.apache.org/jira/browse/MESOS-8810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume gid manager.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69541/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69541: Added volume gid manager.

2018-12-13 Thread Qian Zhang 

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

(Updated Dec. 13, 2018, 7:46 p.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya Pronin, 
and Jie Yu.


Changes
---

Addressed review comments and also changed setVolumeOwnership a bit by unseting 
the `setgid` bit and write permission when `setgid` parameter is false.


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


Repository: mesos


Description
---

Added volume gid manager.


Diffs (updated)
-

  src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
  src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
PRE-CREATION 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 69541: Added volume gid manager.

2018-12-13 Thread Qian Zhang 


> On Dec. 11, 2018, 10:02 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
> > Lines 180 (patched)
> > 
> >
> > Suggestion: we could also pass `containerId` to the c'tor of the 
> > `struct Info`, where `hashset(std::initializer_list list)` can be 
> > used to initialize `Info::containerIds`.

In https://reviews.apache.org/r/69543 , we also need to do `new Info` in the 
`recover` method, so we have to do `info->containerIds.insert(containerId);` 
there anyway.


> On Dec. 11, 2018, 10:02 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
> > Lines 196 (patched)
> > 
> >
> > Should we print a message here? Or even use `CHECK()`.

`!containerVolumes.contains(containerId)` actually is a normal case because for 
the container no using a volume, it will not be contained in 
`containerVolumes`, and we do not need to do anything for such container in 
volume gid manager.


> On Dec. 11, 2018, 10:02 p.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
> > Lines 219 (patched)
> > 
> >
> > Can we check positive result first `if (os::exists(volume))`?

Actually I think we do not need to call `os::exists` here, instead we can 
directly call `setVolumeOwnership` which calls `fts_open` internally, and 
`fts_open` will return an error if the volume path does not exist.


- Qian


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


On Dec. 11, 2018, 12:59 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69541/
> ---
> 
> (Updated Dec. 11, 2018, 12:59 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya 
> Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8810
> https://issues.apache.org/jira/browse/MESOS-8810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume gid manager.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69541/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69541: Added volume gid manager.

2018-12-11 Thread Andrei Budnik 

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




src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
Lines 168 (patched)


it'd be nice to print `containerId` in the log message as well.


- Andrei Budnik


On Dec. 10, 2018, 4:59 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69541/
> ---
> 
> (Updated Dec. 10, 2018, 4:59 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya 
> Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8810
> https://issues.apache.org/jira/browse/MESOS-8810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume gid manager.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69541/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69541: Added volume gid manager.

2018-12-11 Thread Andrei Budnik 

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




src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
Lines 107-108 (patched)


We should copy and then use `Path path` and `Error error` here as well. 
Otherwise, it might SEGFAULT due to use-after-free flaw.



src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
Lines 180 (patched)


Suggestion: we could also pass `containerId` to the c'tor of the `struct 
Info`, where `hashset(std::initializer_list list)` can be used to 
initialize `Info::containerIds`.



src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
Lines 196 (patched)


Should we print a message here? Or even use `CHECK()`.



src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp
Lines 219 (patched)


Can we check positive result first `if (os::exists(volume))`?


- Andrei Budnik


On Dec. 10, 2018, 4:59 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69541/
> ---
> 
> (Updated Dec. 10, 2018, 4:59 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya 
> Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8810
> https://issues.apache.org/jira/browse/MESOS-8810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume gid manager.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8 
>   src/Makefile.am 35500516e18ff251357b9e8d6bba894c44a9253b 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/volume_gid_manager/volume_gid_manager.cpp 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69541/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>