Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removed subshelling from veth between containers #123

Closed
wants to merge 6 commits into from
Closed

Conversation

steiler
Copy link
Collaborator

@steiler steiler commented Nov 13, 2020

Added MTU capability
Had to remove concurrency -> further investigation required

This is still work in progress, fixing #68

Added MTU capability
Had to remove concurrency -> further investigation required
@hellt
Copy link
Member

hellt commented Nov 13, 2020

@steiler when I was researching the topic, I found out that the CNI repo is the best one to draw inspiration from on how to create p2p links. https://github.com/containernetworking/plugins

They have a good explanation how dangerous is it to use netns switching with goroutines - https://github.com/containernetworking/plugins/tree/master/pkg/ns

For our cause of p2p links creation we should browse this repo and see how they create veth pairs, instead of using vishavanda/netns package directly

and maybe even a better reference will be that project and their APIs - https://github.com/redhat-nfvpe/koko/blob/8a6435ee27d70ab2fdf42b05023ec8d0a75276b1/api/koko_api.go#L73

@karimra
Copy link
Member

karimra commented Nov 14, 2020

@steiler here is a blog post about locking goroutines to a thread and thus to a network namespace,
https://www.weave.works/blog/linux-namespaces-golang-followup
this is probably there reason you had to disable concurrency

@steiler
Copy link
Collaborator Author

steiler commented Dec 17, 2020

I've tested this now and it works fine for me.

This should implicitly also resolve #197 and lays foundation for #196 which is already available as a field in the internal struct LinkAttributes which just needs to be filled. hence just the front-end needs implementation.

@steiler steiler marked this pull request as ready for review December 17, 2020 14:19
@hellt
Copy link
Member

hellt commented Dec 19, 2020

@steiler it seems this PR doesn't address #197? In that issue the prime focus is on the single-interfaces linux containers (eth0) which we do not explicitly create
This is created by docker

@steiler
Copy link
Collaborator Author

steiler commented Dec 23, 2020

@hellt you're right. It wasn't clear to me that the default docker bridge was meant.
Any additional feedback on this pull request?

@hellt
Copy link
Member

hellt commented Dec 23, 2020

I think I will get to it starting next year
I still want to see if reusing the koko code will make it cleaner

@hellt
Copy link
Member

hellt commented Jan 5, 2021

Changes are made in #209

@hellt hellt closed this Jan 5, 2021
@hellt hellt deleted the subshelling branch January 31, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants