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 2019/01/07 00:25:49 UTC
Review Request 69675: Added volume gid manager.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/
-----------------------------------------------------------
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
-------
This manager is used to allocate/deallocate gids for shared persistent
volumes and PARENT type SANDBOX_PATH volumes.
Diffs
-----
src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/69675/diff/1/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 69675: Added volume gid manager.
Posted by Qian Zhang <zh...@gmail.com>.
> On Feb. 25, 2019, 10:15 p.m., Benjamin Bannier wrote:
> > How is this patch related to https://reviews.apache.org/r/69541/ and https://reviews.apache.org/r/69344/? Some issues were raised over there and a discussion started. We should strongly avoid loosing such context as it makes it harder to understand patches later.
Those two patches were discarded because we changed our solution after offline discussion, I will describe why we discarded them in those two patches, sorry for the confusion.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/#review213164
-----------------------------------------------------------
On Jan. 7, 2019, 8:25 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69675/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2019, 8:25 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
> -------
>
> This manager is used to allocate/deallocate gids for shared persistent
> volumes and PARENT type SANDBOX_PATH volumes.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
> src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69675/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 69675: Added volume gid manager.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/#review213164
-----------------------------------------------------------
How is this patch related to https://reviews.apache.org/r/69541/ and https://reviews.apache.org/r/69344/? Some issues were raised over there and a discussion started. We should strongly avoid loosing such context as it makes it harder to understand patches later.
- Benjamin Bannier
On Jan. 7, 2019, 1:25 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69675/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2019, 1:25 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
> -------
>
> This manager is used to allocate/deallocate gids for shared persistent
> volumes and PARENT type SANDBOX_PATH volumes.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
> src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69675/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 69675: Added volume gid manager.
Posted by Qian Zhang <zh...@gmail.com>.
> On Feb. 23, 2019, 3:02 p.m., Gilbert Song wrote:
> > src/slave/volume_gid_manager/volume_gid_manager.cpp
> > Lines 212-237 (patched)
> > <https://reviews.apache.org/r/69675/diff/1/?file=2117959#file2117959line212>
> >
> > could we tweak a little bit?
> >
> > ```
> > for (auto it = volumeGids.begin(); it != volumeGids.end(); it++) {
> > const string& volumePath = it->first;
> > gid_t gid = it->second;
> >
> > if (strings::startsWith(volumePath, path)) {
> > if (volumePath != path) {
> > // This is the case of the PARENT type SANDBOX_PATH volume.
> > sandboxPathVolumes.push_back(volumePath);
> > }
> >
> > LOG(INFO) << "Deallocated gid " << gid << " for the volume path '"
> > << volumePath << "'";
> >
> > // Only return the gid to the free range if it is in the total
> > // range. The gid may not be in the total range in the case that
> > // Mesos agent is restarted with a different total range and we
> > // deallocate gid for a previous volume path from the old range.
> > if (totalGids.contains(gid)) {
> > freeGids += gid;
> > }
> >
> > volumeGids.erase(it);
> > }
> > }
> > ```
I think we cannot do that because in the case of `volumeGids.erase(it);` we should not do `it++`.
> On Feb. 23, 2019, 3:02 p.m., Gilbert Song wrote:
> > src/slave/volume_gid_manager/volume_gid_manager.cpp
> > Lines 229 (patched)
> > <https://reviews.apache.org/r/69675/diff/1/?file=2117959#file2117959line229>
> >
> > probably we don't need this check?
I think we need it because we do not want an out-of-date gid returned to `freeGids`. E.g., agent is started with `--volume_gid_range=[10-20]` and gid 10 is allocated to a volume, and while that volume is still being used the agent is restarted with `--volume_gid_range=[30-40]`, and then when we deallocate gid for that volume, we do not want gid 10 returned to `freeGids`, otherwise it will have the chance to be allocated to a volume while it is not in the range `[30-40]` which may confuse the operator.
> On Feb. 23, 2019, 3:02 p.m., Gilbert Song wrote:
> > src/slave/volume_gid_manager/volume_gid_manager.cpp
> > Lines 283 (patched)
> > <https://reviews.apache.org/r/69675/diff/1/?file=2117959#file2117959line283>
> >
> > do we really need `totalGids`?
Yeah, we need it to prevent out-of-date gid returned to `freeGids` since my comment above for details.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/#review213110
-----------------------------------------------------------
On Jan. 7, 2019, 8:25 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69675/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2019, 8:25 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
> -------
>
> This manager is used to allocate/deallocate gids for shared persistent
> volumes and PARENT type SANDBOX_PATH volumes.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
> src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69675/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 69675: Added volume gid manager.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/#review213110
-----------------------------------------------------------
src/slave/volume_gid_manager/volume_gid_manager.cpp
Lines 212-237 (patched)
<https://reviews.apache.org/r/69675/#comment298966>
could we tweak a little bit?
```
for (auto it = volumeGids.begin(); it != volumeGids.end(); it++) {
const string& volumePath = it->first;
gid_t gid = it->second;
if (strings::startsWith(volumePath, path)) {
if (volumePath != path) {
// This is the case of the PARENT type SANDBOX_PATH volume.
sandboxPathVolumes.push_back(volumePath);
}
LOG(INFO) << "Deallocated gid " << gid << " for the volume path '"
<< volumePath << "'";
// Only return the gid to the free range if it is in the total
// range. The gid may not be in the total range in the case that
// Mesos agent is restarted with a different total range and we
// deallocate gid for a previous volume path from the old range.
if (totalGids.contains(gid)) {
freeGids += gid;
}
volumeGids.erase(it);
}
}
```
src/slave/volume_gid_manager/volume_gid_manager.cpp
Lines 229 (patched)
<https://reviews.apache.org/r/69675/#comment298968>
probably we don't need this check?
src/slave/volume_gid_manager/volume_gid_manager.cpp
Lines 283 (patched)
<https://reviews.apache.org/r/69675/#comment298967>
do we really need `totalGids`?
- Gilbert Song
On Jan. 6, 2019, 4:25 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69675/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2019, 4:25 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
> -------
>
> This manager is used to allocate/deallocate gids for shared persistent
> volumes and PARENT type SANDBOX_PATH volumes.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
> src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69675/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 69675: Added volume gid manager.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/#review213272
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Feb. 27, 2019, 6:45 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69675/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2019, 6:45 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
> -------
>
> This manager is used to allocate/deallocate gids for shared persistent
> volumes and PARENT type SANDBOX_PATH volumes.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 687dc8566a252115ebb2c52647cfed22b82abc87
> src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261
> src/slave/volume_gid_manager/state.hpp PRE-CREATION
> src/slave/volume_gid_manager/state.proto PRE-CREATION
> src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
> src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69675/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 69675: Added volume gid manager.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/
-----------------------------------------------------------
(Updated Feb. 27, 2019, 10:45 p.m.)
Review request for mesos, Andrei Budnik, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.
Changes
-------
Added volume type as a parameter of `allocate` method.
Bugs: MESOS-8810
https://issues.apache.org/jira/browse/MESOS-8810
Repository: mesos
Description
-------
This manager is used to allocate/deallocate gids for shared persistent
volumes and PARENT type SANDBOX_PATH volumes.
Diffs (updated)
-----
src/CMakeLists.txt 687dc8566a252115ebb2c52647cfed22b82abc87
src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261
src/slave/volume_gid_manager/state.hpp PRE-CREATION
src/slave/volume_gid_manager/state.proto PRE-CREATION
src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/69675/diff/2/
Changes: https://reviews.apache.org/r/69675/diff/1-2/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 69675: Added volume gid manager.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69675/#review213180
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Jan. 6, 2019, 4:25 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69675/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2019, 4:25 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
> -------
>
> This manager is used to allocate/deallocate gids for shared persistent
> volumes and PARENT type SANDBOX_PATH volumes.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt bde070445b644e15d46c390d1c983caabaa1fec8
> src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa
> src/slave/volume_gid_manager/volume_gid_manager.hpp PRE-CREATION
> src/slave/volume_gid_manager/volume_gid_manager.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69675/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>