sriov-network-device-plugin icon indicating copy to clipboard operation
sriov-network-device-plugin copied to clipboard

Support for acquiring DDP profile using devlink

Open ipatrykx opened this issue 4 years ago • 21 comments
trafficstars

This PR introduces support for devlink commands to be used to acquire DDP profile name on supported hardware. First, the devlink will be used, and if it is not supported then it will fallback to DDPTool as it was before.

Some of the devlink related code will be possibly moved to vishvananda/netlink library if the PR there will be accepted.

Later this can be used to add support for DDP profiles in k8snetworkplumbingwg/sriov-network-operator.

ipatrykx avatar Oct 14 '21 10:10 ipatrykx

LGTM

Eoghan1232 avatar Oct 14 '21 11:10 Eoghan1232

/cc @SchSeba

zshi-redhat avatar Oct 19 '21 02:10 zshi-redhat

@ipatrykx Would you mind updating this PR? I assume some of the util functions have been merged in netlink go library. Thank you!

zshi-redhat avatar Nov 26 '21 04:11 zshi-redhat

Hi @zshi-redhat , sure, sorry for the delay on that. I am right now on a tight schedule, but will try to do this until Tuesday.

ipatrykx avatar Nov 26 '21 14:11 ipatrykx

Pull Request Test Coverage Report for Build 8053076208

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • -21 of 63 (66.67%) changed or added relevant lines in 3 files are covered.
  • 133 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.8%) to 74.83%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/utils/ddp.go 0 8 0.0%
pkg/netdevice/pciNetDevice.go 7 20 35.0%
<!-- Total: 42 63
Files with Coverage Reduction New Missed Lines %
pkg/devices/gen_net.go 2 86.25%
pkg/netdevice/netDeviceProvider.go 3 85.84%
pkg/utils/sriovnet_provider.go 9 22.22%
pkg/factory/factory.go 11 87.92%
pkg/resources/server.go 19 80.51%
pkg/netdevice/pciNetDevice.go 29 53.13%
pkg/utils/utils.go 60 80.7%
<!-- Total: 133
Totals Coverage Status
Change from base Build 7377056051: -0.8%
Covered Lines: 2087
Relevant Lines: 2789

💛 - Coveralls

coveralls avatar Oct 26 '22 14:10 coveralls

Hi @zshi-redhat , sure, sorry for the delay on that. I am right now on a tight schedule, but will try to do this until Tuesday.

@zshi-redhat I know, I've said I'll deliver it until Tuesday, but Thursday isn't bad as well, isn't it? 🥳

Anyway, I've rebased this PR on top of the current master, fixed some issues and added unit tests. Hope we can get it merged soon.

ipatrykx avatar Oct 27 '22 14:10 ipatrykx

@Eoghan1232 @ipatrykx is there a plan to re-kindle this work ?

adrianchiris avatar May 17 '23 08:05 adrianchiris

@Eoghan1232 @ipatrykx is there a plan to re-kindle this work ?

Hi @adrianchiris , yes of course. Looks like just to resolve a merge conflict, will get to it this week :) Then let's get it merged!

Eoghan1232 avatar May 17 '23 08:05 Eoghan1232

@SchSeba @adrianchiris PTAL

Eoghan1232 avatar May 24 '23 13:05 Eoghan1232

As discussed in community, a test is needed to verify if the DDP Tool can be removed from the Dockerfile, as this PR should remove the dependency.

Eoghan1232 avatar May 30 '23 13:05 Eoghan1232

will fix lint issues in a moment.

Eoghan1232 avatar Jan 16 '24 14:01 Eoghan1232

I am currently testing this PR locally and will also provide update soon

Eoghan1232 avatar Jan 16 '24 14:01 Eoghan1232

@SchSeba @adrianchiris PTAL

Eoghan1232 avatar Jan 18 '24 15:01 Eoghan1232

I've updated the PR and validated it now works. I've also fixed lint issue.

I can squash the PR at the end if there are no comments/changes needed.

Eoghan1232 avatar Jan 18 '24 15:01 Eoghan1232

@SchSeba @adrianchiris I've updated this PR. PTAL

Eoghan1232 avatar Feb 20 '24 13:02 Eoghan1232

at some point we would need to switch to interfaces here like we did with sriov-network-operator. not related to this PR.

adrianchiris avatar Feb 26 '24 15:02 adrianchiris

@adrianchiris thanks for the feedback! I've updated the PR, PTAL

Eoghan1232 avatar Feb 26 '24 17:02 Eoghan1232