k8snetworkplumbingwg / sriov-network-operator

Operator for provisioning and configuring SR-IOV CNI plugin and device plugin
Apache License 2.0
84 stars 114 forks source link

introduce a recheck for the VF interface name #720

Closed SchSeba closed 4 months ago

SchSeba commented 5 months ago

It's possible to have a race in the VFIsReady function. vf netdevice can have a default eth0 device name and be the time we call the netlink syscall to get the device information eth0 can be a different device.

this cause duplicate mac allocation on vf admin mac address

github-actions[bot] commented 5 months ago

Thanks for your PR, To run vendors CIs use one of:

SchSeba commented 5 months ago

cc @zeeke @adrianchiris please take a look on this race condition

github-actions[bot] commented 5 months ago

Thanks for your PR, To run vendors CIs use one of:

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 9613496848

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/sriov/sriov.go 5 6 83.33%
<!-- Total: 5 6 83.33% -->
Files with Coverage Reduction New Missed Lines %
pkg/host/internal/sriov/sriov.go 1 44.09%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 9549902217: 0.05%
Covered Lines: 5185
Relevant Lines: 13410

💛 - Coveralls
adrianchiris commented 5 months ago

discussed in community meeting. we should modify the way we get the Link device by relying on interface index instead of name.

interface index is found under sysfs /sys/bus/pci/deivces/<address>/net/<ifaceName/ifindex if path does not exist we should retry (udev rule renamed interface)

github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

SchSeba commented 4 months ago

/hold

waiting for https://github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pull/569

THIS PR CONTAINS A REPLACE IN GO MODS!

github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9700937218

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/helper/mock/mock_helper.go 0 10 0.0%
pkg/host/internal/network/network.go 0 24 0.0%
<!-- Total: 25 62 40.32% -->
Totals Coverage Status
Change from base Build 9700329314: 0.003%
Covered Lines: 5538
Relevant Lines: 13647

💛 - Coveralls
coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9699009559

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/helper/mock/mock_helper.go 0 10 0.0%
pkg/host/internal/network/network.go 0 26 0.0%
<!-- Total: 25 64 39.06% -->
Files with Coverage Reduction New Missed Lines %
pkg/host/internal/lib/dputils/mock/mock_dputils.go 11 80.53%
pkg/host/internal/lib/dputils/dputils.go 13 9.09%
pkg/host/internal/network/network.go 38 44.44%
<!-- Total: 62 -->
Totals Coverage Status
Change from base Build 9691456203: 0.01%
Covered Lines: 5480
Relevant Lines: 13568

💛 - Coveralls
coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9682784028

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/helper/mock/mock_helper.go 0 10 0.0%
pkg/host/internal/network/network.go 0 26 0.0%
<!-- Total: 25 64 39.06% -->
Files with Coverage Reduction New Missed Lines %
pkg/host/internal/lib/dputils/mock/mock_dputils.go 11 80.53%
pkg/host/internal/lib/dputils/dputils.go 13 9.09%
pkg/host/internal/network/network.go 38 44.44%
<!-- Total: 62 -->
Totals Coverage Status
Change from base Build 9667839529: 0.01%
Covered Lines: 5480
Relevant Lines: 13568

💛 - Coveralls
SchSeba commented 4 months ago

/hold cancel

SchSeba commented 4 months ago

I switched the implementation to call the tryGetInterfaceName in the new function that way we don't need any new function in the sriov network device plugin.

github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9760276703

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/sriov/sriov.go 7 8 87.5%
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/host/internal/network/network.go 13 19 68.42%
pkg/helper/mock/mock_helper.go 0 10 0.0%
<!-- Total: 41 61 67.21% -->
Files with Coverage Reduction New Missed Lines %
controllers/drain_controller.go 1 68.06%
pkg/host/internal/sriov/sriov.go 1 55.4%
controllers/generic_network_controller.go 5 74.53%
<!-- Total: 7 -->
Totals Coverage Status
Change from base Build 9745388570: 0.3%
Covered Lines: 5582
Relevant Lines: 13635

💛 - Coveralls
github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9761609394

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/sriov/sriov.go 7 8 87.5%
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/host/internal/network/network.go 13 19 68.42%
pkg/helper/mock/mock_helper.go 0 11 0.0%
<!-- Total: 42 63 66.67% -->
Files with Coverage Reduction New Missed Lines %
pkg/host/internal/sriov/sriov.go 1 55.4%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 9745388570: 0.4%
Covered Lines: 5589
Relevant Lines: 13637

💛 - Coveralls
github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9766049420

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/sriov/sriov.go 7 8 87.5%
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/host/internal/network/network.go 13 19 68.42%
pkg/helper/mock/mock_helper.go 0 11 0.0%
<!-- Total: 42 63 66.67% -->
Files with Coverage Reduction New Missed Lines %
pkg/host/internal/sriov/sriov.go 1 55.4%
controllers/generic_network_controller.go 5 74.53%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 9745388570: 0.3%
Covered Lines: 5584
Relevant Lines: 13637

💛 - Coveralls
github-actions[bot] commented 4 months ago

Thanks for your PR, To run vendors CIs use one of:

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9774215462

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/sriov/sriov.go 7 8 87.5%
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/host/internal/network/network.go 13 19 68.42%
pkg/helper/mock/mock_helper.go 0 11 0.0%
<!-- Total: 42 63 66.67% -->
Files with Coverage Reduction New Missed Lines %
controllers/drain_controller.go 1 68.06%
pkg/host/internal/sriov/sriov.go 1 55.4%
controllers/generic_network_controller.go 5 74.53%
<!-- Total: 7 -->
Totals Coverage Status
Change from base Build 9774112825: 0.3%
Covered Lines: 5583
Relevant Lines: 13637

💛 - Coveralls
SchSeba commented 4 months ago

done with all the function names :P

can you please take another look?