✨ Add name field to network links and make MAC address optional
#3003
Conversation
metal3-io-bot
added
the
size/L
s3rj1k
changed the title
name field to network links and make MAC address optional
There was a problem hiding this comment.
Pull request overview
This PR enables cloud-init network configuration flexibility by adding interface naming support and making MAC addresses optional. The changes allow two key scenarios: (1) interface renaming via MAC-based matching combined with a target name, and (2) direct interface naming using kernel names without MAC-based discovery. The implementation adds a name field (populated from id) to all network link types and conditionally includes MAC address fields only when provided.
Key Changes
- Added
namefield to network link rendering for Ethernet, Bond, and VLAN interfaces, set from theidfield - Made MAC address fields optional in rendering logic - they are now conditionally included only when non-nil
- Added comprehensive test coverage for both nil MAC address scenarios and interface renaming use cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| baremetal/metal3data_manager.go | Updated renderNetworkLinks to add name field for all link types and conditionally include MAC addresses; added nil check in getLinkMacAddress |
| baremetal/metal3data_manager_test.go | Updated existing tests to expect name field; added extensive test cases for nil MAC addresses and interface renaming scenarios across all link types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
s3rj1k
force-pushed
the
interface-rename
branch
2 times, most recently
from
6339924 to
e518757
Compare
kashifest
left a comment
kashifest
left a comment
There was a problem hiding this comment.
I am still trying to understand the need for this change, not an expert here but when I read the link referenced https://github.com/canonical/cloud-init/blob/main/cloudinit/sources/helpers/openstack.py#L599 it says
# 'name' is not in openstack spec yet, but we will support it if it is
# present. The 'id' in the spec is currently implemented as the host
# nic's name, meaning something like 'tap-adfasdffd'. We do not want
# to name guest devices with such ugly names.
From code point of view I am trying to understand also the need of duplicating one value to two fields.
Lastly, we are also in near future going towards non cloud-init scenarios like ignition, so we need to think about making the format also as generic as possible.
I will want to understand this more before approving.
Let's assume you have a cluster with bunch of nodes, each node has multiple NICs onboard, all NICs on this nodes are of different vendors, so (kernel/udev) naming across each NIC per it is function is not uniform. So for example:
Now let's assume that you what to have a sane keepalived config for VIP on all 3 nodes, but all of you Management NICs have a different naming. So how this problem would be solved by typical Sysadmin, he would collect all MACs of each of those Management NIC, and force Similar predictable (by functional usage) NIC naming across whole fleet of machines is applicable to other use-case when multiple NICs are involved. Now to have this renaming feature one needs to fully-ignore all of CAPM3 IPAM facilities and directly push a networking config via cloud-init files management ( |
Ok, thanks for the explanation. I think this is a valid use case. Recently, @Sunnatillo has worked in predictable naming of interfaces as well. I would have his and @Rozzii's and @lentzi90's take on this as well. |
lentzi90
left a comment
lentzi90
left a comment
There was a problem hiding this comment.
Thank you! I have been fighting this also recently.
I have only one concern and that is about backwards compatibility. Let's say I have a Metal3DataTemplate with the following:
spec:
networkData:
links:
ethernets:
- type: phy
id: eno1 # <- This name is just for reference. I do not want to rename.
macAddress:
fromAnnotation:
object: baremetalhost
annotation: mac
vlans:
- id: eno1.2
vlanID: 2
macAddress:
fromAnnotation:
object: baremetalhost
annotation: mac
vlanLink: eno1 # <- Referenced here.Here I need to set some id to be able to reference it in other places, but it does not mean that it should be renamed.
If I understand the change correctly, this PR would change the behavior so that the interface is renamed, since the MAC is specified.
Is this correct? I think we will need to mark this as a breaking change in that case.
Is there some other way to reference a link without also renaming it?
I think in case of using exactly same IDs as actual NIC names inside OS, rename will be a NO-OP, like it will rename but to same name. Not sure if there is a breaking change actually, we need to verify this: before the change, if actual NIC name was I ask this because, before the change there was no reason to use IDs that are not exactly using actual NIC names. We could introduce "name" field into Metal3DataTemplate and ask consumers to use that explicitly, it will solve your concern. |
I think there is reason. I have been using it without knowing the NIC name. As long as I know the MAC, I just put that and then "anything" for the ID. This is configuration that I am currently using. It allows me to have one Metal3DataTemplate for many different BareMetalHosts with completely different NICs. What I have currently deployed: apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: Metal3DataTemplate
metadata:
name: metal3-k8s-template
spec:
clusterName: metal3-k8s
networkData:
links:
ethernets:
- type: phy
id: eno1
macAddress:
fromAnnotation:
object: baremetalhost
annotation: mac
vlans:
- id: eno1.2
vlanID: 2
macAddress:
fromAnnotation:
object: baremetalhost
annotation: mac
vlanLink: eno1
- id: eno1.5
vlanID: 5
macAddress:
fromAnnotation:
object: baremetalhost
annotation: mac
vlanLink: eno1
networks:
ipv4:
- id: internal
link: eno1
ipAddressFromIPPool: internal-ipv4-metal3-k8s
- id: oob
link: eno1.2
ipAddressFromIPPool: oob-ipv4-metal3-k8sTwo different hosts have been configured through this template: [centos@eselda13u31s06 ~]$ ip a
...
2: eno49: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
link/ether e4:11:5b:95:7e:98 brd ff:ff:ff:ff:ff:ff
altname enp6s0f0
altname enxe4115b957e98
inet 192.168.0.152/24 brd 192.168.0.255 scope global noprefixroute eno49
valid_lft forever preferred_lft forever
...
4: eno49.2@eno49: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
link/ether e4:11:5b:95:7e:98 brd ff:ff:ff:ff:ff:ff
inet 192.168.1.130/24 brd 192.168.1.255 scope global noprefixroute eno49.2
valid_lft forever preferred_lft forever
...
[centos@eseldb12u02 ~]$ ip a
...
11: ens2f3np3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
link/ether 40:a6:b7:c9:74:bf brd ff:ff:ff:ff:ff:ff
altname enp181s0f3np3
altname enx40a6b7c974bf
inet 192.168.0.153/24 brd 192.168.0.255 scope global noprefixroute ens2f3np3
valid_lft forever preferred_lft forever
13: ens2f3np3.2@ens2f3np3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
link/ether 40:a6:b7:c9:74:bf brd ff:ff:ff:ff:ff:ff
inet 192.168.1.131/24 brd 192.168.1.255 scope global noprefixroute ens2f3np3.2
valid_lft forever preferred_lft forever
...As you can see, it works and it does not rename them. Don't get me wrong, I am very happy to adopt this new feature and rename them. But if I have been doing it this way without renaming, I am also sure there are others and there is guaranteed to be someone who doesn't like that the names suddenly start to change. Mandatory xkcd reference... 😅 |
I am fine extending this change to have a "name" field if we have a consensus that this is what we want to do. |
s3rj1k
force-pushed
the
interface-rename
branch
from
e518757 to
7c7331e
Compare
lentzi90
left a comment
lentzi90
left a comment
There was a problem hiding this comment.
This looks good to me (with squash before merge)!
@Rozzii @Sunnatillo any comments from you?
For me it looks like a nice clean new feature now, with no change in behavior for existing use-cases 🙂
This change enables interface renaming, add 'name' field to network
link output, set from the 'name' field. Cloud-init uses the 'name'
field to rename interfaces when combined with MAC address matching.
```
networkData:
links:
ethernets:
- id: eth0
name: enp1s0 # cloud-init will rename interface to enp1s0
macAddress:
fromHostInterface: eth0 # find by MAC, rename to enp1s0
```
(ref: cloudinit/sources/helpers/openstack.py)
Signed-off-by: s3rj1k <[email protected]>
s3rj1k
force-pushed
the
interface-rename
branch
from
e8073ff to
906b0d8
Compare
I've rebased and squashed the commits just in case (+ updated PR description) |
lentzi90
left a comment
lentzi90
left a comment
There was a problem hiding this comment.
/approve
Still would like some input from @Sunnatillo and @Rozzii before merging
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lentzi90 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
the
approved
5 participants
This change enables interface renaming, add 'name' field to network link output, set from the 'name' field. Cloud-init uses the 'name' field to rename interfaces when combined with MAC address matching.
(ref: cloudinit/sources/helpers/openstack.py)