You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/03/24 03:44:14 UTC
Review Request 32426: Symlink the namespace handle with ContainerID
for the port mapping isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/
-----------------------------------------------------------
Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
Bugs: MESOS-2528
https://issues.apache.org/jira/browse/MESOS-2528
Repository: mesos
Description
-------
Symlink the namespace handle with ContainerID for the port mapping isolator.
See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
src/tests/port_mapping_tests.cpp 8192deac8d9b7ea1896bb62a8b5961ef90326fa4
Diff: https://reviews.apache.org/r/32426/diff/
Testing
-------
sudo make check
Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
Will add a compatibility test tomorrow.
Thanks,
Jie Yu
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 24, 2015, 8:01 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1512
> > <https://reviews.apache.org/r/32426/diff/1/?file=903757#file903757line1512>
> >
> > Why delete from linkees here? AFAICT, it's not used after this loop.
Yes, but i would rather keep linkers and linkees in sync.
> On March 24, 2015, 8:01 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1813
> > <https://reviews.apache.org/r/32426/diff/1/?file=903757#file903757line1813>
> >
> > What happens if the slave dies before doing the symlink?
It's OK because we will not even try to recover that container because veth does not exist. MESOS-2547 trackes the removal of stale bind mount handles.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77596
-----------------------------------------------------------
On March 24, 2015, 2:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 2:44 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8192deac8d9b7ea1896bb62a8b5961ef90326fa4
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 24, 2015, 8:01 p.m., Vinod Kone wrote:
> > The recovery code is a bit confusing with a bunch of linkers and linkees. Is it possible to clean that up a bit?
> >
> > More importantly, IIUC there are 3 entities here. links (veth), pids (file bind mounted) and containerids (symlink to pid). Why is that there is no symmetry in their creation (isolate(): pid -> cid -> link) and deletion (cleanup(): cid -> pid -> link)? Shouldn't 'link' be removed first in deletion?
> >
> > Also, have you considered all the cases when a slave fails at any point during isolate() or cleanup()?
> >
> > 1) pid, no cid, no link (slave dies during isolate() or cleanup())
> > 2) pid, cid, no link (slave dies during isolate() or cleanup())
> > 3) pid, cid, link (steady state)
> >
> > If there is symmetry these are the only cases to consider. If not, there are more cases to consider.
Yeah, I agree with you. Previously, I want to remove the link in the end so that we don't leak a bind mount if slave crashes between veth removal and umount. However, as you pointed out, the bind mount will be leaked anyway if the slave crashes in isolate() since we create the bind mount first (and we must do because we need to have a reference to the namespace handle before creating veth, otherwise, it'll be removed automatically if the child process exits early).
So I'll swap the deletion order in cleanup() and augument the stale bind mounts cleanup (https://issues.apache.org/jira/browse/MESOS-2547).
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77596
-----------------------------------------------------------
On March 24, 2015, 2:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 2:44 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8192deac8d9b7ea1896bb62a8b5961ef90326fa4
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77596
-----------------------------------------------------------
The recovery code is a bit confusing with a bunch of linkers and linkees. Is it possible to clean that up a bit?
More importantly, IIUC there are 3 entities here. links (veth), pids (file bind mounted) and containerids (symlink to pid). Why is that there is no symmetry in their creation (isolate(): pid -> cid -> link) and deletion (cleanup(): cid -> pid -> link)? Shouldn't 'link' be removed first in deletion?
Also, have you considered all the cases when a slave fails at any point during isolate() or cleanup()?
1) pid, no cid, no link (slave dies during isolate() or cleanup())
2) pid, cid, no link (slave dies during isolate() or cleanup())
3) pid, cid, link (steady state)
If there is symmetry these are the only cases to consider. If not, there are more cases to consider.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125745>
Why not do this in the above for loop itself?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125723>
s/./ and a pid is reused by a new container/ ?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125724>
s/pid/pids/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125762>
Why delete from linkees here? AFAICT, it's not used after this loop.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125753>
s/meta/checkpointed meta data/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125754>
What happens if the slave dies before doing the symlink?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125758>
s/older versions/older (pre 0.23.0) versions/
- Vinod Kone
On March 24, 2015, 2:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 2:44 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8192deac8d9b7ea1896bb62a8b5961ef90326fa4
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77781
-----------------------------------------------------------
Patch looks great!
Reviews applied: [32426]
All tests passed.
- Mesos ReviewBot
On March 25, 2015, 6:43 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 25, 2015, 11 p.m., Chi Zhang wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1414
> > <https://reviews.apache.org/r/32426/diff/2/?file=905705#file905705line1414>
> >
> > Here in recover, the first thing we look at is the veths on the system to determine _all_ candidates for recover or orphan cleanup. This is why veth used to be cleaned up as the last step in '_cleanup'.
> >
> > I think you need to keep that in '_cleanup'. Maybe you can re-adjust the creation order in this patch to add veth, touch pid file, mount pid file and touch symlink in isolate, and do the reverse in '_cleanup'?
See my comments above. I don't want to re-adjust the creation order because the veth will be gone if the child process exits early. I would rather avoid that bahavior. As I already pointed out, stale bind mount is inevitable. What we need is a more robust cleanup logic during recovery for those stale bind mounts (MESOS-2547). Dropping this issue.
> On March 25, 2015, 11 p.m., Chi Zhang wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1431
> > <https://reviews.apache.org/r/32426/diff/2/?file=905705#file905705line1431>
> >
> > I have to agree with Vinod that the linker and linkee made it quite hard for me to understand too.
> >
> > Can we use more explicit names like pidToContainerIdMap (just an example)?
We use linker-->linkee concept in libprocess as well
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/process.cpp#L303
I could add a comment.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77818
-----------------------------------------------------------
On March 25, 2015, 6:43 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77818
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126112>
Here in recover, the first thing we look at is the veths on the system to determine _all_ candidates for recover or orphan cleanup. This is why veth used to be cleaned up as the last step in '_cleanup'.
I think you need to keep that in '_cleanup'. Maybe you can re-adjust the creation order in this patch to add veth, touch pid file, mount pid file and touch symlink in isolate, and do the reverse in '_cleanup'?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126113>
I have to agree with Vinod that the linker and linkee made it quite hard for me to understand too.
Can we use more explicit names like pidToContainerIdMap (just an example)?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126115>
Is this by design that you don't want to do this section in the above for loop (over all files from ls)?
Also I think you can achieve this without using _linkers explictly.
- Chi Zhang
On March 25, 2015, 6:43 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 26, 2015, 11:43 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1659
> > <https://reviews.apache.org/r/32426/diff/2-3/?file=905705#file905705line1659>
> >
> > Shouldn't this be a CHECK?
It could be 0:)
> On March 26, 2015, 11:43 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1579
> > <https://reviews.apache.org/r/32426/diff/2-3/?file=905705#file905705line1579>
> >
> > When do the namespace handles and veth links for these get cleaned up?
When `_cleanup` is called. All pid in linkers have veth associated with. So they will be removed in `_cleanup`.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77964
-----------------------------------------------------------
On March 26, 2015, 10:56 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 10:56 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8fc854e77ef6b5ebb76f25a9c9691f5a0d8c872b
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77964
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126327>
How about
```
return getPidFromNamespaceHandle(target.get());
```
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126326>
s/Do not expect/Not expecting/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126328>
s/ID/IDs/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126337>
Instead of (or in addition to) saying what it is used to, can you mention what this map represents? Is it all the containers that have valid veth links, name space handles and container id symlinks?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126329>
s/ids/IDs/ ? to be consistent with #1456
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126330>
s/are/to be/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126331>
s/handle/handles/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126333>
Can you also add a bit about how this is possible?
Since 'linkers' only contains pids which have veth links, is it possible for two different container ids to have valid veth links at the same time?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126335>
Can you add a bit about why this is OK?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126338>
When do the namespace handles and veth links for these get cleaned up?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126339>
Shouldn't this be a CHECK?
- Vinod Kone
On March 26, 2015, 10:56 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 10:56 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8fc854e77ef6b5ebb76f25a9c9691f5a0d8c872b
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77962
-----------------------------------------------------------
BTW, the new patch also fixed MESOS-2547
- Jie Yu
On March 26, 2015, 10:56 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 10:56 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8fc854e77ef6b5ebb76f25a9c9691f5a0d8c872b
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77973
-----------------------------------------------------------
Patch looks great!
Reviews applied: [32426]
All tests passed.
- Mesos ReviewBot
On March 26, 2015, 10:56 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 10:56 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8fc854e77ef6b5ebb76f25a9c9691f5a0d8c872b
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77986
-----------------------------------------------------------
Ship it!
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126371>
s/root/that have valid veth links and namespace handles/
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126370>
s/NOTE/Note/
- Vinod Kone
On March 27, 2015, 12:05 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 27, 2015, 12:05 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8fc854e77ef6b5ebb76f25a9c9691f5a0d8c872b
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/
-----------------------------------------------------------
(Updated March 27, 2015, 12:05 a.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
Changes
-------
Review comments.
Bugs: MESOS-2528
https://issues.apache.org/jira/browse/MESOS-2528
Repository: mesos
Description
-------
Symlink the namespace handle with ContainerID for the port mapping isolator.
See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
src/tests/port_mapping_tests.cpp 8fc854e77ef6b5ebb76f25a9c9691f5a0d8c872b
Diff: https://reviews.apache.org/r/32426/diff/
Testing
-------
sudo make check
Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
Will add a compatibility test tomorrow.
Thanks,
Jie Yu
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/
-----------------------------------------------------------
(Updated March 26, 2015, 10:56 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
Changes
-------
Review comments. Added a test.
Bugs: MESOS-2528
https://issues.apache.org/jira/browse/MESOS-2528
Repository: mesos
Description
-------
Symlink the namespace handle with ContainerID for the port mapping isolator.
See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
src/tests/port_mapping_tests.cpp 8fc854e77ef6b5ebb76f25a9c9691f5a0d8c872b
Diff: https://reviews.apache.org/r/32426/diff/
Testing
-------
sudo make check
Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
Will add a compatibility test tomorrow.
Thanks,
Jie Yu
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 26, 2015, 7:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1459
> > <https://reviews.apache.org/r/32426/diff/2/?file=905705#file905705line1459>
> >
> > If this returns a Result as commented above, we could do a CHECK(!pid.isNone()), because that's the invariant right?
I would rather not use a CHECK here. THe cleanup logic in the isolator is best effort. So it's possible (though unlikely) that symlink removal failed but the removal of bind mounts succeeds.
Instead of using a CHECK here, I'll just remove the dangling symlink and continue the recovery logic.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77780
-----------------------------------------------------------
On March 25, 2015, 6:43 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 26, 2015, 7:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, lines 1484-1486
> > <https://reviews.apache.org/r/32426/diff/2/?file=905705#file905705line1484>
> >
> > How is this possible? IIUC, if a slave crashes after it removes the 'veth link' but before cleaning up the namespace handle and/or container id simlink, wouldn't that be cleaned up during recovery before a new container is launched? Are you worried about non-crash case where we ignore removal of symlink?
Yes, I just wanna be more defensive.
> On March 26, 2015, 7:03 p.m., Vinod Kone wrote:
> > src/tests/port_mapping_tests.cpp, line 140
> > <https://reviews.apache.org/r/32426/diff/2/?file=905706#file905706line140>
> >
> > Any new tests that explicitly verify the fix?
Added a test.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77780
-----------------------------------------------------------
On March 25, 2015, 6:43 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77780
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126269>
Is it worth differentiating the case where there is some unexpected error vs when it's a dangling symlink? i.e., return Result<pid_t> instead of Try<pid_it>?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126262>
Is this a TODO or a NOTE?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126271>
s/handles/handles, i.e., handles which doesn't have symlinks/ ?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126267>
Include the 'path' in the failure because getContainerIdFromSymlink doesn't always include the 'path' in its returned failure.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126270>
If this returns a Result as commented above, we could do a CHECK(!pid.isNone()), because that's the invariant right?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126268>
ditto. include pid.
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126272>
Don't you also want to remove the network namespace handle itself?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126273>
How is this possible? IIUC, if a slave crashes after it removes the 'veth link' but before cleaning up the namespace handle and/or container id simlink, wouldn't that be cleaned up during recovery before a new container is launched? Are you worried about non-crash case where we ignore removal of symlink?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126275>
Instead of linkees, linkers and _linkers, how about just using a multihashmap of linkers.
```
// We use a multihashmap here because multiple container ids can map to the same pid when...
multihashmap<pid_t, ContainerID> linkers;
// Construct the linkers map from bind mount.....
for (all files in bind mount) {
// Do validation.
// If valid entry.
linkers.put(pid, containerid);
}
// If multiple container ids point to same pid, we remove all those symlinks. This is safe because...
foreach (pid_t pid, linkers.keys()) {
if (linkers.get(pid).size() > 1) {
// Remove symlinks etc.
// Remove it from the map.
linkers.remove(pid);
}
}
```
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126276>
Delete network namespace handle too?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment126277>
Why is a removal error ok? What does it have to do with backwards compatibility? Is it because os::rm(linker) returns an error if 'linker' doesn't exist? If yes, you probably want to just do the rm inside "if (os::exists(linkers))".
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125991>
s/later/in 0.23.0/
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32426/#comment125994>
s/mount file/mount file or its symlink/ ?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32426/#comment126278>
Any new tests that explicitly verify the fix?
- Vinod Kone
On March 25, 2015, 6:43 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/
-----------------------------------------------------------
(Updated March 25, 2015, 6:43 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
Changes
-------
Review comments.
Bugs: MESOS-2528
https://issues.apache.org/jira/browse/MESOS-2528
Repository: mesos
Description
-------
Symlink the namespace handle with ContainerID for the port mapping isolator.
See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
Diff: https://reviews.apache.org/r/32426/diff/
Testing
-------
sudo make check
Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
Will add a compatibility test tomorrow.
Thanks,
Jie Yu
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77567
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rojas
On March 24, 2015, 3:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 3:44 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8192deac8d9b7ea1896bb62a8b5961ef90326fa4
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77520
-----------------------------------------------------------
Patch looks great!
Reviews applied: [32426]
All tests passed.
- Mesos ReviewBot
On March 24, 2015, 2:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 2:44 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8192deac8d9b7ea1896bb62a8b5961ef90326fa4
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On March 24, 2015, 2:15 p.m., Alexander Rojas wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1449
> > <https://reviews.apache.org/r/32426/diff/1/?file=903757#file903757line1449>
> >
> > I don't have any solution for this, but I feel that if any other application creates a symlink in the `PORT_MAPPING_BIND_MOUNT_ROOT()`, then it will be parsed as a `ContainerID`, since its value is nothing but a string.
> >
> > Perhaps we should discuss ideas on how to validate that an ID is actually an ID.
I added a TODO in the code.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77563
-----------------------------------------------------------
On March 25, 2015, 6:43 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 623840e71938791a562a32989775818275e6d37e
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 32426: Symlink the namespace handle with
ContainerID for the port mapping isolator.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32426/#review77563
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/32426/#comment125685>
I don't have any solution for this, but I feel that if any other application creates a symlink in the `PORT_MAPPING_BIND_MOUNT_ROOT()`, then it will be parsed as a `ContainerID`, since its value is nothing but a string.
Perhaps we should discuss ideas on how to validate that an ID is actually an ID.
- Alexander Rojas
On March 24, 2015, 3:44 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32426/
> -----------------------------------------------------------
>
> (Updated March 24, 2015, 3:44 a.m.)
>
>
> Review request for mesos, Chi Zhang, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-2528
> https://issues.apache.org/jira/browse/MESOS-2528
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Symlink the namespace handle with ContainerID for the port mapping isolator.
>
> See ticket for details. This patch will allow a smooth upgrade (i.e., rolling foward and back are both safe).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.hpp 4dd066a47d43cb1d52f93294d86309151738743e
> src/slave/containerizer/isolators/network/port_mapping.cpp 4bf0adeeac1cb6fe59f9c2ca8d5980b1500f5ddd
> src/tests/port_mapping_tests.cpp 8192deac8d9b7ea1896bb62a8b5961ef90326fa4
>
> Diff: https://reviews.apache.org/r/32426/diff/
>
>
> Testing
> -------
>
> sudo make check
>
> Many existing tests should already capture the regression. For example, ROOT_CleanUpOrphanTest checks if the bind mount dir is empty after the container is destroyed.
>
> Will add a compatibility test tomorrow.
>
>
> Thanks,
>
> Jie Yu
>
>