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

Implement configurators for VLANs and Bonds #2573

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Mar 29, 2022

This is the last missing piece needed to enable support for VLANs
and LAGs for EVE management and local network instances.
This functionality is described and designed in this document:
https://wiki.lfedge.org/display/EVE/EVE+VLAN+support+-+create+VLAN+and+bond+interfaces

A part of this commit is also a fix for Switch network VLANs,
where we forgot to configure the network uplink port as trunk, therefore
all tagged packets coming in and out of it would be dropped by bridge VLAN
filtering. As a result, VLANs were working correctly only for air-gapped
switch networks.

Test for VLANs and LAGs: lf-edge/eden#753

Signed-off-by: Milan Lenco milan@zededa.com

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

The single "k" string raises a question about where and how we do the k swap.

If we have a vlan I assume we create vlan10 on top of eth0 and then we do a kvlan10 rename and create a vlan10 bridge. Is that correct?
In such a case do we also allow creating a network instance directly on eth0 (for untagged frames?) or not. That would end up causing a eth0->keth0 rename etc.

I don't think bonds cause the same potential complexity since a child of a bond is exclusive hence can not be used directly AFAIK. But bond0 could be used by both a local network instance and a switch network instance (just like vlan10 can be) which means we'd need a kbond0 rename AFAIU

@milan-zededa
Copy link
Contributor Author

milan-zededa commented Apr 4, 2022

If we have a vlan I assume we create vlan10 on top of eth0 and then we do a kvlan10 rename and create a vlan10 bridge. Is that correct?
In such a case do we also allow creating a network instance directly on eth0 (for untagged frames?) or not. That would end up causing a eth0->keth0 rename etc.

Actually it was not planned to support this case, at least not for now (design document is here, note the sentence "NIM service should take care of not creating the kethx interfaces for physical IO adapters that are underlying adapters for other Vland or bond adapters"). Mostly to simplify (both EVE and the controller), we decided that it would not be allowed to attach network instance to a port which has higher-layer adapters (see this diagram).
So if eth has VLANs then it will not be possible to use it directly anymore. Do you think that this is a too much of a limitation for the future users of EVE?

I don't think bonds cause the same potential complexity since a child of a bond is exclusive hence can not be used directly AFAIK. But bond0 could be used by both a local network instance and a switch network instance (just like vlan10 can be) which means we'd need a kbond0 rename AFAIU

Yes, renaming is done and bridge is created for bond IF there is nothing defined on top of it (like vlans).

@eriknordmark
Copy link
Contributor

So if eth has VLANs then it will not be possible to use it directly anymore. Do you think that this is a too much of a limitation for the future users of EVE?

I think that restriction is fine (at least for now) as long as we enforce it in the code and return an error if a NI tries to use it directly.

@milan-zededa
Copy link
Contributor Author

So if eth has VLANs then it will not be possible to use it directly anymore. Do you think that this is a too much of a limitation for the future users of EVE?

I think that restriction is fine (at least for now) as long as we enforce it in the code and return an error if a NI tries to use it directly.

It is enforced in zedagent in parseSystemAdapterConfig (tested by TestInvalidLowerLayerReferences) and error is returned in DevicePortStatus. Basically we do not allow SystemAdapter to reference port which is used by higher-layer adapters (and NIs reference system adapters, not ports directly).

This is the last missing piece needed to enable support for VLANs
and LAGs for EVE management and local network instances.
This functionality is described and designed in this document:
https://wiki.lfedge.org/display/EVE/EVE+VLAN+support+-+create+VLAN+and+bond+interfaces

A part of this commit is also a fix for Switch network VLANs,
where we forgot to configure the network uplink port as trunk, therefore
all tagged packets coming in and out of it would be dropped by bridge VLAN
filtering. As a result, VLANs were working correctly only for air-gapped
switch networks.

Signed-off-by: Milan Lenco <milan@zededa.com>
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknordmark eriknordmark merged commit b5469d9 into lf-edge:master Apr 5, 2022
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.

2 participants