Skip to content

Fixed the check for network namespace path.#290

Merged
dcbw merged 1 commit intocontainernetworking:masterfrom
asridharan:dcos
Aug 31, 2016
Merged

Fixed the check for network namespace path.#290
dcbw merged 1 commit intocontainernetworking:masterfrom
asridharan:dcos

Conversation

@asridharan
Copy link

@asridharan asridharan commented Aug 25, 2016

The expectation on older kernels (< 3.19) was to have the network
namespace always be a directory. This is not true if the network
namespace is bind mounted to a file, and will make the plugin fail
erroneously in such cases.

I have tried this with Mesos on a CentOS 7 cluster, running 3.10 kernel and it works fine. Moreover, have also tried it on Ubuntu 14.04 which has 3.13 kernel and works fine on that as well.

This should fix #288

@jieyu
Copy link
Contributor

jieyu commented Aug 25, 2016

I suggest we completely remove this check. As @asridharan pointed out, namespace handles can be bind mounted. Container runtime might choose to do the bind mount to keep the namespace open. This is a very common practice.

@dcbw
Copy link
Member

dcbw commented Aug 25, 2016

Seems OK to me... I think the main reason for the "ns" path check was to capture stuff like /proc/18768/task/18798/ns/net.

@tomdee
Copy link
Member

tomdee commented Aug 25, 2016

Yep, no objection from me either. @steveej any thoughts?

@asridharan
Copy link
Author

asridharan commented Aug 25, 2016

So the consensus is to remove the check completely under the PROCFS_MAGIC block?
So I guess the point of the switch stat.Type check would be to only check that the file system type returned is PROCFS or NSFS.

@asridharan
Copy link
Author

@tomdee @steveej thoughts on the new changes?

We were blocked on this for a DC/OS 1.8 release and would like to get these changes in, if the PR looks good.

Thanks a lot !!

@rosenhouse
Copy link
Contributor

LGTM, perhaps pending a change to the commit-message style so that it follows the contribution guidelines?

@asridharan
Copy link
Author

@rosenhouse updated the commit message to follow the contribution guidelines. For the subsystem field I used packages. If there is a pre-defined list of sub-system that we can pick from and this needs to be changed please advise.

@dcbw
Copy link
Member

dcbw commented Aug 31, 2016

@asridharan if you change the prefix to "pkg/ns" then I'll merge it. Thanks!

@dcbw
Copy link
Member

dcbw commented Aug 31, 2016

@asridharan I don't think there's a predefined list, but a quick "git log pkg/ns" or for whatever module will show what previous commits have used.

The expectation on older kernels (< 3.19) was to have the network
namespace always be a directory. This is not true if the network
namespace is bind mounted to a file, and will make the plugin fail
erroneously in such cases. The fix is to remove this assumption
completely and just do a basic check on the file system types being
returned.

Fixes #288
@asridharan
Copy link
Author

Ah !! @dcbw thanks!! Just updated the commit message.

@dcbw
Copy link
Member

dcbw commented Aug 31, 2016

@steveej this seems reasonable to me for now; if we want to catch random /proc paths passed in I guess we should figure out what to do there and take bind-mounted paths into account too. Merging for now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CNI bridge plugin fails to read a bind mounted network namespace in CentOS 7

5 participants