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

MACVLAN interfaces #1402

Merged
merged 16 commits into from
Jul 26, 2023
Merged

MACVLAN interfaces #1402

merged 16 commits into from
Jul 26, 2023

Conversation

steiler
Copy link
Collaborator

@steiler steiler commented May 22, 2023

This is to support macvlan and macvtap interfaces in the endpoint section of the topology definition.

  links:
    - endpoints: ["l1:eth1", "macvlan:enp4s0f0"]
    - endpoints: ["l1:eth2", "macvtap:enp4s0f0"]

This PR fixes #1393.

from @hellt: I removed macvtap support in b5dbaf4. If we need to add it later this commit can be reverted/re-added.

@steiler
Copy link
Collaborator Author

steiler commented May 22, 2023

I had to base this PR on top of #1395.

@steiler
Copy link
Collaborator Author

steiler commented May 23, 2023

I am not yet 100% sure if we should probably dissociate from the rather simple format we used for the links and come up with some more formal struct.
We're right now forcing everything into this format of a slice with two elements... that already make up 4 pieces of information. 2x (Node + Interface)
We should have these seperated and probably also define an endpoint type and somehow get rid of the to the most extend procedural way of wiring the labs up.

In this PR now I had to e.g. add interface types 'macvlan/macvtap' to the dependency manager as fake nodes, to prevent it from complaining and a couple of other stunts... To me it appears ugly enough to do something about it.

Opinions? @hellt ?

@hellt
Copy link
Member

hellt commented May 24, 2023

@steiler can you rebase this please on top of the main branch? I just merged #1395

@steiler steiler marked this pull request as ready for review May 30, 2023 09:47
@steiler
Copy link
Collaborator Author

steiler commented May 30, 2023

We're missing docs.
Also in my opinion we should only allow a new kind of link definition for the this type of interfaces, although this PR allows defining them via a fake node "macvlan" or "macvtap" in the links section.

Comment on lines 107 to 121
dm := &defaultDependencyManager{
nodes: map[string]*dependencyNode{},
}

// init helper nodes
// TODO, we need to rework this stuff
for _, n := range []string{"macvlan", "macvtap"} {
dm.AddNode(n)
// artificially indicate that all the helper nodes have reached all the defined states
for _, s := range RegularNodeStates {
dm.SignalDone(n, s)
}
}

return dm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steiler why is this needed to support macvlan, but was not needed to support host kind?

If there is a difference, then maybe we can use host kind for macvlan/tap as well and not touch depmgr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, there is always multiple ways towards rome.
For host and mgmt-net we prevented this dependency from getting inserted.
See https://github.com/srl-labs/containerlab/blob/main/clab/clab.go#L505
I'll extend that filter to also exclude the macvtap and macvlan (fake) nodes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either I removed this with a wrong force push, or you never added this condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yepp you trashed 3 commits

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cherry-picked and pushed them now

@hellt
Copy link
Member

hellt commented Jul 20, 2023

@steiler with #1450 merged we can move this one I believe

@steiler steiler force-pushed the macvInterfaces branch 2 times, most recently from ee1fb6a to dec07c7 Compare July 20, 2023 07:26
@hellt
Copy link
Member

hellt commented Jul 25, 2023

With a recent forced push I have rebased this PR on top of the current main, to include the work done in #1450

@hellt hellt changed the title MacV(tap/lan) interfaces MACVLAN interfaces Jul 25, 2023
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #1402 (1d56674) into main (b17d642) will decrease coverage by 0.40%.
The diff coverage is 1.26%.

@@            Coverage Diff             @@
##             main    #1402      +/-   ##
==========================================
- Coverage   21.77%   21.37%   -0.40%     
==========================================
  Files          65       64       -1     
  Lines        6830     6858      +28     
==========================================
- Hits         1487     1466      -21     
- Misses       5212     5261      +49     
  Partials      131      131              
Files Changed Coverage Δ
clab/clab.go 29.63% <0.00%> (-0.08%) ⬇️
clab/netlink.go 0.00% <0.00%> (ø)
types/link.go 68.00% <ø> (-0.75%) ⬇️
clab/config.go 47.01% <11.11%> (-0.79%) ⬇️

@hellt hellt merged commit 80a6554 into main Jul 26, 2023
18 of 19 checks passed
@hellt hellt deleted the macvInterfaces branch July 26, 2023 11:52
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.

Container interfaces connected to host interfaces via macvlan
2 participants