fix: set mac address for macvtap if specified #231
Conversation
Codecov Report
@@ Coverage Diff @@
## main #231 +/- ##
==========================================
+ Coverage 55.81% 56.27% +0.45%
==========================================
Files 44 44
Lines 2012 1985 -27
==========================================
- Hits 1123 1117 -6
+ Misses 781 767 -14
+ Partials 108 101 -7
Continue to review full report at Codecov.
|
| @@ -80,14 +90,15 @@ func (n *networkService) IfaceCreate(ctx context.Context, input ports.IfaceCreat | |||
| return nil, fmt.Errorf("creating interface %s using netlink: %w", link.Attrs().Name, err) | |||
| } | |||
|
|
|||
| macIf, err := netlink.LinkByName(link.Attrs().Name) | |||
| macIf, err := netlink.LinkByName(input.DeviceName) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("getting interface %s using netlink: %w", link.Attrs().Name, err) | |||
There was a problem hiding this comment.
is this error still "correct"? input.DeviceName now? (i get they are the same, but for clarity maybe since you changed the above?)
There was a problem hiding this comment.
I changed it back as there was no need to change that part. And with the original its more obvious that we are re-getting the link we just added.
There was a problem hiding this comment.
zero tests for this file 😆 so adding an issue to backfill #244
There is a problem with using an auto generated mac address with a macvtap interface. The kernel appears to change the mac address from what it was originally created with. The firecracker and cloud init configuration contain the aoriginal mac address. When the guest boots and cloud-init gets to configured the network the mac address has changed and this then causes the network configuration to fail as it can't match on the mac address. We need to find actual evidence of the above but this has been a known behaviour with other types of interface and for the those the advice is to specify a mac address when you create the interface. This change will use the `guest_mac` if its specified for the mac address on the host if the interface type is macvtap. For macvtap the mac address on the host is the same as in the guest. Signed-off-by: Richard Case <richard@weave.works>
| // This whole block is unreachable right now, because | ||
| // the Do function is called only if ShouldDo returns | ||
| // true. It retruns false if IfaceExists returns true. | ||
| // Line 76 will never return exists=true |
There was a problem hiding this comment.
It's still a half-dead code here and I left this comment it here:
- It's visible we try to do that and in case we call Do without ShouldDo, it works
- If the device status is not update and we someone checks the code "but hey we do an update here", they can see "nope we don't" and don't start debugging for hours to get to the same results that it's not executed if it's called only on ShouldDo==true
What this PR does / why we need it:
There is a problem with using an auto generated mac address with a macvtap interface. The
kernel appears to change the mac address from what it was originally
created with. The firecracker and cloud init configuration contain the original mac address.
When the guest boots and cloud-init gets to configure the network the
mac address has changed and this then causes the network configuration to fail as it can't
match on the mac address.
We need to find actual evidence of the above, but this has been a known
behaviour with other types of interface and for the those the advice is
to specify a mac address when you create the interface.
This change will use the
guest_macif it's specified for the macaddress on the host if the interface type is macvtap. For macvtap the
mac address on the host is the same as in the guest.
Also removed an erroneous comment about a block of code not being hit.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist: