WIP: Set GUID and MAC for VF with default driver loaded #245
Conversation
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
fix #244 |
|
/test-all |
|
Do we still need #233 with this fix? |
|
I change my PR to contain also this change (it was implemented there in another method) so maybe we can just merge #233 ? |
pkg/utils/utils.go
Outdated
| if err = setVfGuid(addr, pfLink); err != nil { | ||
| return err | ||
| } | ||
| } else if err = setVfAdminMac(addr, pfLink); err != nil { |
There was a problem hiding this comment.
I think we can still set VF admin mac w/ vfio-pci driver, right?
Just like sriov-cni can set VF attributes(admin-mac, vlan, trust, spoofchk etc) in vfio-pci mode.
There was a problem hiding this comment.
Does it make any sense to set the VF MAC when using vfio-pci driver? We invoke vfIsReady in setVfAdminMac, it returns error with vfio-pci driver.
There was a problem hiding this comment.
This is to resolve #244
There was a problem hiding this comment.
I am afraid this will result in adapters with vfio-pci to use the ff:ff:ff:ff:ff:ff:ff address. Can we just skip the setVfAdminMac in case we did not change the vfNum? Considering this agent always creates the VFs, the mac will always be set, because it would happen when we created them.
There was a problem hiding this comment.
Does it make any sense to set the VF MAC when using vfio-pci driver?
I think yes. For some vendor NICs, the default admin MAC is all-zero, which means all packet (including packets with spoofed MAC) can be sent by app using the VF (when VF spoofChk is set to on).
pliurh
changed the title
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Changing other VF driver type or other parameter will no longer trigger setting GUID and MAC
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
| @@ -287,15 +287,18 @@ func configSriovDevice(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetwor | |||
| break | |||
| } | |||
| } | |||
| if strings.EqualFold(iface.LinkType, "IB") { | |||
| if err = setVfGuid(addr, pfLink); err != nil { | |||
| if iface.NumVfs != ifaceStatus.NumVfs { | |||
There was a problem hiding this comment.
its not clear to me why we skip this step, i think its worth adding a comment here
looking at @mskrocki comment
Considering this agent always creates the VFs, the mac will always be set, because it would happen when we created them.
There was a problem hiding this comment.
Agree. The change in this PR will be merged into #233, and this PR will be deprecated.
There was a problem hiding this comment.
Hi @pliurh @adrianchiris @mskrocki can you please take a look on #233
I think my implementation there is better I will explain on that PR
No description provided.