You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2019/02/23 07:02:50 UTC
Re: Review Request 69675: Added volume gid manager.
-----------------------------------------------------------
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 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
>
>