Support for acquiring DDP profile using devlink #387

ipatrykx · 2021-10-14T10:53:30Z

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.

Eoghan1232 · 2021-10-14T11:21:44Z

LGTM

zshi-redhat · 2021-10-19T02:56:13Z

/cc @SchSeba

pkg/utils/ddp.go

pkg/utils/devlink.go

pkg/netdevice/pciNetDevice.go

pkg/utils/devlink.go

zshi-redhat · 2021-11-26T04:09:29Z

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

ipatrykx · 2021-11-26T14:33:55Z

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.

pkg/utils/devlink.go

coveralls · 2022-10-26T14:08:16Z

Pull Request Test Coverage Report for Build 3329880032

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

17 of 35 (48.57%) changed or added relevant lines in 4 files are covered.
20 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-1.9%) to 75.285%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/utils/testing.go	0	1	0.0%
pkg/utils/netlink_provider.go	0	4	0.0%
pkg/netdevice/pciNetDevice.go	12	17	70.59%
pkg/utils/ddp.go	5	13	38.46%

Files with Coverage Reduction	New Missed Lines	%
pkg/utils/netlink_provider.go	3	33.33%
pkg/netdevice/pciNetDevice.go	17	74.55%

Totals
Change from base Build 2733786348:	-1.9%
Covered Lines:	1587
Relevant Lines:	2108

💛 - Coveralls

ipatrykx · 2022-10-27T14:24:21Z

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.

adrianchiris · 2023-05-17T08:13:10Z

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

Eoghan1232 · 2023-05-17T08:49:14Z

@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!

Co-Authored-By: Eoghan1232 <[email protected]> Co-Authored-By: ipatrykx <[email protected]>

Eoghan1232 · 2023-05-24T13:27:10Z

@SchSeba @adrianchiris PTAL

Eoghan1232 · 2023-05-30T13:25:32Z

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.

Moving the changes into the GetDDPProfiles function Signed-off-by: Eoghan Russell <[email protected]>

Eoghan1232 · 2024-02-20T13:26:35Z

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

pkg/netdevice/pciNetDevice.go

removed gocyclo as method is no longer changed Signed-off-by: Eoghan Russell <[email protected]>

pkg/netdevice/pciNetDevice.go

pkg/utils/devlink.go

pkg/utils/ddp.go

adrianchiris · 2024-02-26T15:43:30Z

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

Signed-off-by: Eoghan Russell <[email protected]>

Eoghan1232 · 2024-02-26T17:23:45Z

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

Signed-off-by: Eoghan Russell <[email protected]>

pkg/utils/ddp.go

SchSeba · 2024-03-17T12:50:59Z

pkg/utils/devlink.go

+)
+
+// IsDevlinkDDPSupportedByPCIDevice checks if DDP of PCI device can be acquired with devlink info command
+func IsDevlinkDDPSupportedByPCIDevice(device string) bool {


what is the different between the two functions?

IsDevlinkDDPSupportedByPCIDevice:
This function is intended to check if DDP is supported specifically for PCI devices.

IsDevlinkDDPSupportedByDevice:
This function is more generic and checks if DDP is supported for any type of device.

I guess since IsDevlinkDDPSupportedByPCIDevice is just a wrapper that calls IsDevlinkDDPSupportedByDevice, we could remove IsDevlinkDDPSupportedByPCIDevice in replace of IsDevlinkDDPSupportedByDevice

WDYT?

I have removed IsDevlinkDDPSupportedByPCIDevice and used IsDevlinkDDPSupportedByDevice throughout instead

pkg/utils/devlink.go

Eoghan1232 · 2024-05-20T14:07:38Z

will update this PR in next day - also update docs to call out support for E800 series but issue with current ddptool means E700 series may not work.

Eoghan1232 · 2024-05-21T08:51:12Z

just testing my changes - will push commit after

Signed-off-by: Eoghan Russell <[email protected]>

Eoghan1232 · 2024-05-21T08:58:14Z

@SchSeba @adrianchiris PTAL

Signed-off-by: Eoghan Russell <[email protected]>

adrianchiris · 2024-05-22T12:55:07Z

pkg/utils/devlink.go

+)
+
+var (
+	netlinkDevlinkGetDeviceInfoByNameAsMap = netlink.DevlinkGetDeviceInfoByNameAsMap


we already have a netlink provider [1] shouldnt we extend it with the new method ? it already has netlink calls to devlink.

then, the rest of the functions in this file would be moved to ddp.go as they are pretty specific for ddp.

in tests (ddp_test.go) we would use a mocked version of NetlinkProvider instead of defining a fake function.

[1]

sriov-network-device-plugin/pkg/utils/netlink_provider.go

Line 28 in d59e6ab

GetDevLinkDeviceEswitchAttrs(ifName string) (*nl.DevlinkDevEswitchAttr, error)

Hi @adrianchiris yes 100% agree.

I migrated the code, but have hit an issue with the mockery.

2024/05/23 14:26:14 internal error: package "bytes" without types was imported from "github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/utils"

I am trying to figure out the reason, but this is blocking me from making the changes, as I cannot regenerate the mocked version of the NetlinkProvider Interface.

I also did a test of moving the code to a test project and trying to compile but still hit the same issues with mockery.

Has anyone come across an issue like this with mockery before?

I found this one argoproj/argo-workflows#8251
can you try to build the mockery version please?

adrianchiris · 2024-05-22T12:58:43Z

@Eoghan1232 i gave another round, left a comment to make things a bit more consistent with other places in device plugin code.

hoping to get this sorted soon so i can tag a release.

extended netlink provider updated unit tests regenerated mocked interfaces addressed comments Signed-off-by: Eoghan Russell <[email protected]>

Eoghan1232 · 2024-05-31T15:32:34Z

trying to fix lint issue.....

Eoghan1232 · 2024-05-31T15:42:35Z

@adrianchiris @SchSeba PTAL

adrianchiris

LGTM

SchSeba

LGTM

BUT please squash all the commits to reasonable ones :)

adrianchiris · 2024-06-02T13:41:19Z

Squashed to single commit, edited commit msg to contain brief description.

ipatrykx changed the title ~~Added support for acquiring DDP profile using devlink~~ Support for acquiring DDP profile using devlink Oct 14, 2021

Eoghan1232 approved these changes Oct 14, 2021

View reviewed changes

zshi-redhat reviewed Oct 19, 2021

View reviewed changes

pkg/utils/ddp.go Show resolved Hide resolved

pkg/utils/devlink.go Outdated Show resolved Hide resolved

pkg/utils/devlink.go Outdated Show resolved Hide resolved

pkg/netdevice/pciNetDevice.go Outdated Show resolved Hide resolved

pkg/netdevice/pciNetDevice.go Outdated Show resolved Hide resolved

adrianchiris mentioned this pull request Oct 19, 2021

Release sriov-network-device-plugin v3.4.0 #385

Closed

adrianchiris reviewed Nov 2, 2021

View reviewed changes

pkg/utils/devlink.go Outdated Show resolved Hide resolved

adrianchiris mentioned this pull request Nov 10, 2021

Fix detection of Mellanox VFs in switchdev mode #395

Merged

zshi-redhat mentioned this pull request Nov 26, 2021

Use Devlink when possible for NewPciNetDevice #399

Open

ipatrykx changed the base branch from master to alpine_image_broken November 30, 2021 15:47

ipatrykx changed the base branch from alpine_image_broken to master November 30, 2021 15:47

e0ne reviewed Dec 2, 2021

View reviewed changes

pkg/utils/devlink.go Outdated Show resolved Hide resolved

adrianchiris mentioned this pull request Mar 1, 2022

Devlink Error message #413

Merged

ipatrykx force-pushed the devlink-support branch 2 times, most recently from cbfaa77 to 1ea4df3 Compare October 26, 2022 13:54

ipatrykx force-pushed the devlink-support branch 2 times, most recently from 48535fc to 9161854 Compare October 27, 2022 13:55

SchSeba mentioned this pull request Jan 9, 2023

Updating alpine version to fix docker image build #460

Closed

Eoghan1232 mentioned this pull request May 23, 2023

Release 3.6.0 #486

Closed

devlink support to acquire DDP profile name on supported hardware

9091c2f

Co-Authored-By: Eoghan1232 <[email protected]> Co-Authored-By: ipatrykx <[email protected]>

Eoghan1232 force-pushed the devlink-support branch from 9161854 to 9091c2f Compare May 24, 2023 13:24

SchSeba mentioned this pull request Dec 20, 2023

DDP Tool failing to get ddp profile for devices #516

Closed

update PR

bc45ca6

Moving the changes into the GetDDPProfiles function Signed-off-by: Eoghan Russell <[email protected]>

Eoghan1232 reviewed Feb 20, 2024

View reviewed changes

pkg/netdevice/pciNetDevice.go Show resolved Hide resolved

adrianchiris reviewed Feb 26, 2024

View reviewed changes

pkg/netdevice/pciNetDevice.go Outdated Show resolved Hide resolved

Addressing comments

d30a3c6

removed gocyclo as method is no longer changed Signed-off-by: Eoghan Russell <[email protected]>

adrianchiris reviewed Feb 26, 2024

View reviewed changes

pkg/netdevice/pciNetDevice.go Show resolved Hide resolved

pkg/utils/devlink.go Outdated Show resolved Hide resolved

pkg/utils/devlink.go Outdated Show resolved Hide resolved

pkg/utils/ddp.go Show resolved Hide resolved

pkg/utils/ddp.go Outdated Show resolved Hide resolved

Addressing comments

6050f33

Signed-off-by: Eoghan Russell <[email protected]>

Fix IF statement

8a2d8c6

Signed-off-by: Eoghan Russell <[email protected]>

SchSeba reviewed Mar 17, 2024

View reviewed changes

Applying updates

2c3628d

Signed-off-by: Eoghan Russell <[email protected]>

Removing redundant function

13ec6f9

Signed-off-by: Eoghan Russell <[email protected]>

adrianchiris reviewed May 22, 2024

View reviewed changes

Eoghan Russell added 2 commits May 31, 2024 15:21

updates

e205cdb

extended netlink provider updated unit tests regenerated mocked interfaces addressed comments Signed-off-by: Eoghan Russell <[email protected]>

fixing imports

b018ba3

Eoghan1232 force-pushed the devlink-support branch from a2a85a8 to b018ba3 Compare May 31, 2024 15:26

Eoghan Russell added 2 commits May 31, 2024 16:27

Merge branch 'master' into devlink-support

c29e684

fixing lint error

4b2eace

fixing lint issue

c74f66e

adrianchiris approved these changes Jun 2, 2024

View reviewed changes

SchSeba approved these changes Jun 2, 2024

View reviewed changes

adrianchiris merged commit bd0a56d into k8snetworkplumbingwg:master Jun 2, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for acquiring DDP profile using devlink #387

Support for acquiring DDP profile using devlink #387

ipatrykx commented Oct 14, 2021 •

edited

Loading

Eoghan1232 commented Oct 14, 2021 •

edited

Loading

zshi-redhat commented Oct 19, 2021

zshi-redhat commented Nov 26, 2021

ipatrykx commented Nov 26, 2021

coveralls commented Oct 26, 2022 •

edited

Loading

ipatrykx commented Oct 27, 2022 •

edited

Loading

adrianchiris commented May 17, 2023

Eoghan1232 commented May 17, 2023

Eoghan1232 commented May 24, 2023 •

edited

Loading

Eoghan1232 commented May 30, 2023

Eoghan1232 commented Feb 20, 2024

adrianchiris commented Feb 26, 2024

Eoghan1232 commented Feb 26, 2024

SchSeba Mar 17, 2024

Eoghan1232 May 21, 2024

Eoghan1232 May 21, 2024

Eoghan1232 commented May 20, 2024

Eoghan1232 commented May 21, 2024

Eoghan1232 commented May 21, 2024

adrianchiris May 22, 2024 •

edited

Loading

Eoghan1232 May 23, 2024

SchSeba May 23, 2024

adrianchiris commented May 22, 2024

Eoghan1232 commented May 31, 2024

Eoghan1232 commented May 31, 2024

adrianchiris left a comment

SchSeba left a comment

adrianchiris commented Jun 2, 2024

Support for acquiring DDP profile using devlink #387

Support for acquiring DDP profile using devlink #387

Conversation

ipatrykx commented Oct 14, 2021 • edited Loading

Eoghan1232 commented Oct 14, 2021 • edited Loading

zshi-redhat commented Oct 19, 2021

zshi-redhat commented Nov 26, 2021

ipatrykx commented Nov 26, 2021

coveralls commented Oct 26, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3329880032

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

ipatrykx commented Oct 27, 2022 • edited Loading

adrianchiris commented May 17, 2023

Eoghan1232 commented May 17, 2023

Eoghan1232 commented May 24, 2023 • edited Loading

Eoghan1232 commented May 30, 2023

Eoghan1232 commented Feb 20, 2024

adrianchiris commented Feb 26, 2024

Eoghan1232 commented Feb 26, 2024

SchSeba Mar 17, 2024

Choose a reason for hiding this comment

Eoghan1232 May 21, 2024

Choose a reason for hiding this comment

Eoghan1232 May 21, 2024

Choose a reason for hiding this comment

Eoghan1232 commented May 20, 2024

Eoghan1232 commented May 21, 2024

Eoghan1232 commented May 21, 2024

adrianchiris May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Eoghan1232 May 23, 2024

Choose a reason for hiding this comment

SchSeba May 23, 2024

Choose a reason for hiding this comment

adrianchiris commented May 22, 2024

Eoghan1232 commented May 31, 2024

Eoghan1232 commented May 31, 2024

adrianchiris left a comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

adrianchiris commented Jun 2, 2024

ipatrykx commented Oct 14, 2021 •

edited

Loading

Eoghan1232 commented Oct 14, 2021 •

edited

Loading

coveralls commented Oct 26, 2022 •

edited

Loading

ipatrykx commented Oct 27, 2022 •

edited

Loading

Eoghan1232 commented May 24, 2023 •

edited

Loading

adrianchiris May 22, 2024 •

edited

Loading