k8snetworkplumbingwg / sriov-cni

DPDK & SR-IOV CNI plugin
Apache License 2.0
307 stars 146 forks source link

Remove interface name from alt name if exist #292

Closed SchSeba closed 6 months ago

SchSeba commented 7 months ago

Fixes: https://github.com/k8snetworkplumbingwg/sriov-cni/issues/250

coveralls commented 7 months ago

Pull Request Test Coverage Report for Build 8220774282

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/utils/mocks/netlink_manager_mock.go 9 11 81.82%
pkg/utils/netlink_manager.go 0 3 0.0%
pkg/sriov/sriov.go 17 21 80.95%
pkg/sriov/mocks/pci_utils_mock.go 0 13 0.0%
<!-- Total: 26 48 54.17% -->
Files with Coverage Reduction New Missed Lines %
pkg/utils/mocks/netlink_manager_mock.go 1 84.93%
pkg/sriov/mocks/pci_utils_mock.go 1 12.16%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 8189210689: 0.2%
Covered Lines: 582
Relevant Lines: 1261

💛 - Coveralls
SchSeba commented 7 months ago

@adrianchiris @zeeke please take a look :)

zeeke commented 7 months ago

Only one minor comment. PR looks good to me

SchSeba commented 7 months ago

Thanks @zeeke done

SchSeba commented 6 months ago

Hi @zeeke @e0ne @Eoghan1232 when the time allows please review this PR thanks :)

Eoghan1232 commented 6 months ago

Hi @SchSeba I am okay with the change, but not sure why it's an issue. for example, other cni's use alt-name as a way to restore the original VF name I think host-device cni uses this logic I know sriov-cni caches the cmdAdd anyways, so we can restore from cache, thus I am okay with this change.

SchSeba commented 6 months ago

@Eoghan1232 thanks for the comment!

the reason we need this change (it's better explained in the issue) is that we are not able to return the vf to the host network namespace with is original name because it contains the same name in alt-name.

btw if host-cni use this feature it also needs to remove the alt-name before it restore it or it will not work

Eoghan1232 commented 6 months ago

thanks for the info. I'm happy for the merge :)