-
Notifications
You must be signed in to change notification settings - Fork 276
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 for link creation #209
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karimra
reviewed
Dec 30, 2020
Thank you for your thorough review @karimra! |
karimra
reviewed
Dec 31, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @karimra @steiler
I'd had a festive morning where I wanted to look into the #68 more closely. @steiler I decided to create a new PR, since I had to use different approach than you proposed in #123, I will explain why.
1 Link MTU
First, I decided to add an MTU config parameter to the
Link
object to allow for MTU configuration on veth links https://github.com/srl-wim/container-lab/compare/native-veth?expand=1#diff-24901324638614d6ed95797828a77247d9dad583c83606ec43cce4f7ed2e8269R129The Link MTU defaults to 1500 if not set - https://github.com/srl-wim/container-lab/compare/native-veth?expand=1#diff-24901324638614d6ed95797828a77247d9dad583c83606ec43cce4f7ed2e8269R475-R478
2 NSPath is part of Node
I added NSPath field to
Node
struct. This string field contains a path to the netns file descriptor of the namespace this node(container) is in. https://github.com/srl-wim/container-lab/compare/native-veth?expand=1#diff-24901324638614d6ed95797828a77247d9dad583c83606ec43cce4f7ed2e8269R122The NSPath is used in the veth creation code, that takes this path to put a veth endpoint into the relevant netns
3 Removed InitVirtualWiring entirely
I removed the
InitVirtualWiring
func, because all it was doing is cleaning up the interfaces in the host NS which might be left hanging after the previous deployments. It is not needed anymore, since the code that creates the veth interfaces now will clean them up if something goes wrong during the creation.4 veth creation
The root of this change is in the
CreateVirtualWiring
func https://github.com/srl-wim/container-lab/compare/native-veth?expand=1#diff-1a8d111c070c6cf01d39d5f2935d710f1e09fe35101cd75796f8cec113133b32R24It now has a single private function that will handle both vethToNS and vethToBridge creation methods.
It uses a new struct vEthEndpoint which defines the veth endpoint https://github.com/srl-wim/container-lab/compare/native-veth?expand=1#diff-1a8d111c070c6cf01d39d5f2935d710f1e09fe35101cd75796f8cec113133b32R24
@steiler the reason I needed to create a new PR, is because I read how CNI creates veth interfaces, and they use
ns
package (github.com/containernetworking/plugins/pkg/ns) that has aDo()
function that wraps all the thread locking techniques. That enables us to not tackle that ourselves and rely on a proven techniques.I did test the new code on a large scale deployment (20,4,2 scenario) and it worked faster than original subshelling method. But it would be good if you try it yourselves.
close #68, #123