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

veth creation: make first node create the veth endpoints #1914

Merged
merged 16 commits into from
Feb 29, 2024
Merged

Conversation

steiler
Copy link
Collaborator

@steiler steiler commented Feb 23, 2024

This is the PR to tackle the link creation issues described in #1907.

This PR now changes the link creation strategy. For the veth, where there are two endpoints located in two network namespaces. The link is created by the first node that reaches the create-links phases. It creates the link with its two endpoints in the host namespace with randomized interface names. One endpoint (netlink.link) is then transferred to the netns of the container, renamed to the designated name, and set admin up.
The other endpoint remains in the root/host netns (wiht its random clab- name) until the second node reaches the create-links phase. The second node, instead of creating the whole veth link, only pulls in the existing endpoint, renames it and sets it admin up.

This way, the defined interfaces will exist, as soon as the create-links phase is reached. However, it is not guarantied that the link is already operational. But anyways will this help to perform ip link and ip address commands and further also prevents deadlocks in case of vrnetlab nodes, where the nodes would only boot the VMs when all the interfaces are available to the network namespace.

Further did I remove the reference from nodes to links. Nodes do own endpoints that belong to links. Hence to access Link attributes, the endpoint should be consulted. This prevents us from drifting state, if an endpoint is added but the link is missed or the other way around.

- remove links from nodes -> should go via Endpoints->link
- nodes deploy links, whch then call deploy on the link
@steiler steiler marked this pull request as ready for review February 26, 2024 11:39
@steiler steiler requested a review from hellt February 26, 2024 13:23
@steiler steiler added the enhancement New feature or request label Feb 27, 2024
@robotwalk
Copy link
Contributor

Hi @steiler,

I just tested this PR and it works like a charm.

  • Boot up duration increased.
  • Most resource hungry vms get healthy first

Topology needed before around 48min to become healthy and there was always the possibilty that the virtual asr9k will never become healthy because there are not enough resources.
With this PR asr9k become healthy after 17min and the complete topology after 41mins.

If I did the math right speed increased by 15%

@steiler
Copy link
Collaborator Author

steiler commented Feb 28, 2024

Great, then it is now up to @hellt to do the final review.

links/link_veth.go Outdated Show resolved Hide resolved
links/link_veth.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 53.20%. Comparing base (f2b4a62) to head (78cd584).
Report is 230 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1914      +/-   ##
==========================================
+ Coverage   48.59%   53.20%   +4.61%     
==========================================
  Files         133      155      +22     
  Lines       12789    11303    -1486     
==========================================
- Hits         6215     6014     -201     
+ Misses       5870     4437    -1433     
- Partials      704      852     +148     
Files Coverage Δ
cmd/tools_veth.go 74.57% <100.00%> (+6.47%) ⬆️
cmd/vxlan.go 66.66% <100.00%> (+37.43%) ⬆️
links/endpoint.go 88.00% <ø> (+8.33%) ⬆️
links/endpoint_bridge.go 60.00% <100.00%> (+18.06%) ⬆️
links/endpoint_host.go 66.66% <100.00%> (+30.95%) ⬆️
links/endpoint_raw.go 80.00% <100.00%> (+7.65%) ⬆️
links/endpoint_veth.go 100.00% <100.00%> (ø)
links/generic_link_node.go 61.53% <100.00%> (-7.43%) ⬇️
links/link.go 63.77% <ø> (+2.15%) ⬆️
links/link_host.go 64.81% <ø> (-2.69%) ⬇️
... and 11 more

... and 109 files with indirect coverage changes

@hellt
Copy link
Member

hellt commented Feb 29, 2024

Thanks mate, this is solid
Shipping it

@hellt hellt merged commit 5333ca2 into main Feb 29, 2024
62 checks passed
@hellt hellt deleted the vethcreationorder branch February 29, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants