You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jason Lai <ja...@jasonlai.net> on 2018/03/15 22:31:14 UTC
Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/
-----------------------------------------------------------
Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
Bugs: MESOS-8651
https://issues.apache.org/jira/browse/MESOS-8651
Repository: mesos
Description
-------
The `volume/sandbox_path` isolator inserts a string of the sandbox path
to its `sandboxes` hashmap instance variable upon the launch of each
container. However, it never cleans it up properly and can cause
unbounded growth of the hashmap object, as isolators are global
singleton objects.
The patch ensures the sandbox path associated with a given container ID
gets removed from the `sandboxes` hashmap upon container cleanup.
Diffs
-----
src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
Diff: https://reviews.apache.org/r/66104/diff/1/
Testing
-------
Thanks,
Jason Lai
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199367
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On March 15, 2018, 10:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 10:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199301
-----------------------------------------------------------
PASS: Mesos patch 66104 was successfully built and tested.
Reviews applied: `['66104']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66104
- Mesos Reviewbot Windows
On March 15, 2018, 3:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 3:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199308
-----------------------------------------------------------
Patch looks great!
Reviews applied: [66104]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 15, 2018, 10:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 10:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199298
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On March 15, 2018, 3:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 3:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 16, 2018, 6:47 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
> > Lines 404 (patched)
> > <https://reviews.apache.org/r/66104/diff/1/?file=1978194#file1978194line404>
> >
> > Let's not have this CHECK here. It's possible that cleanup is called multiple times (caller assumes that this function is idempotent).
Correction, cleanup won't be called multiple times for a containre, but might be called for an unknown container.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199336
-----------------------------------------------------------
On March 15, 2018, 10:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 10:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199336
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
Lines 404 (patched)
<https://reviews.apache.org/r/66104/#comment279646>
Let's not have this CHECK here. It's possible that cleanup is called multiple times (caller assumes that this function is idempotent).
- Jie Yu
On March 15, 2018, 10:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 10:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Zhitao Li <zh...@apache.org>.
> On March 15, 2018, 4:33 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
> > Lines 404 (patched)
> > <https://reviews.apache.org/r/66104/diff/1/?file=1978194#file1978194line404>
> >
> > Instead of the check failure. We probably should return a failure instead.
Do we even need to return a failure here? Is a warning log sufficient?
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199300
-----------------------------------------------------------
On March 15, 2018, 3:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 3:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 66104: Fixed potential memory leak in the
`volume/sandbox_path` isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66104/#review199300
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp
Lines 404 (patched)
<https://reviews.apache.org/r/66104/#comment279598>
Instead of the check failure. We probably should return a failure instead.
- Gilbert Song
On March 15, 2018, 3:31 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66104/
> -----------------------------------------------------------
>
> (Updated March 15, 2018, 3:31 p.m.)
>
>
> Review request for mesos, Eric Chung, Gilbert Song, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8651
> https://issues.apache.org/jira/browse/MESOS-8651
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `volume/sandbox_path` isolator inserts a string of the sandbox path
> to its `sandboxes` hashmap instance variable upon the launch of each
> container. However, it never cleans it up properly and can cause
> unbounded growth of the hashmap object, as isolators are global
> singleton objects.
>
> The patch ensures the sandbox path associated with a given container ID
> gets removed from the `sandboxes` hashmap upon container cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 5801977e93bcb8f463a2635f73e763098f2aa97d
>
>
> Diff: https://reviews.apache.org/r/66104/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>