[switchdev 6/x] Update netlink lib and add helper functions #633

ykulazhenkov · 2024-02-13T14:23:53Z

This PR contains following changes:

update netlink library to include latest changes
change VDPA-related functions implementations to use netlink library instead of govdpa library. This is required support additional configuration options for VDPA devices (e.g. MaxVQP parameter)
add GetDevlinkDeviceParam/SetDevlinkDeviceParam functions which will be later used to configure flow steering mode for NICs in switchdev mode.

cc @adrianchiris @SchSeba @zeeke

github-actions · 2024-02-13T14:24:06Z

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

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

coveralls · 2024-02-13T14:29:19Z

Pull Request Test Coverage Report for Build 8016849082

Details

-86 of 257 (66.54%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 34.821%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/host/internal/vdpa/vdpa.go	36	40	90.0%
pkg/host/internal/network/network.go	64	78	82.05%
pkg/host/internal/lib/netlink/netlink.go	0	15	0.0%
pkg/host/mock/mock_host.go	11	32	34.38%
pkg/helper/mock/mock_helper.go	0	32	0.0%

Totals
Change from base Build 8007791540:	0.4%
Covered Lines:	4213
Relevant Lines:	12099

💛 - Coveralls

go.mod

pkg/host/internal/lib/netlink/netlink.go

github-actions · 2024-02-22T09:57:58Z

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

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-02-22T10:15:32Z

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

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-02-22T10:31:44Z

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

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

adrianchiris

Overall looks good , left a few comments.

adrianchiris · 2024-02-22T18:25:20Z

pkg/host/internal/kernel/kernel.go

+	log.Log.V(2).Info("GetDriverByBusAndDevice(): get driver for device", "bus", bus, "device", device)
+	driver, err := getDriverByBusAndDevice(bus, device)
+	if err != nil {
+		log.Log.V(2).Info("GetDriverByBusAndDevice(): can't read device driver for device", "bus", bus, "device", device)


given its a no on the comment above,
we are going to have roughly the same information logged in this verbosity level by getDriverByBusAndDevice

do we really want this additional logs (here and below)?

I agree, there is no need for additional logs here.

adrianchiris · 2024-02-22T18:27:17Z

pkg/host/internal/kernel/kernel.go

+// device - the name of the device on the bus, e.g. 0000:85:1e.5 for PCI or vpda1 for VDPA
+func (k *kernel) GetDriverByBusAndDevice(bus, device string) (string, error) {
+	log.Log.V(2).Info("GetDriverByBusAndDevice(): get driver for device", "bus", bus, "device", device)
+	driver, err := getDriverByBusAndDevice(bus, device)


WDYT about just renaming this function and making it part of kernel struct ?

I removed extra logging from GetDriverByBusAndDevice. I would prefer to keep getDriverByBusAndDevice as a separate function

adrianchiris · 2024-02-22T18:51:11Z

pkg/host/internal/network/network.go

@@ -196,3 +200,93 @@ func (n *network) GetNetDevLinkSpeed(ifaceName string) string {

 	return fmt.Sprintf("%s Mb/s", strings.TrimSpace(string(data)))
 }
+
+// GetDeviceParam returns devlink parameter for the device as a string, if the parameter has multiple values


nit: GetDevlinkDeviceParam

adrianchiris · 2024-02-22T18:51:42Z

pkg/host/types/interfaces.go

@@ -97,6 +97,13 @@ type NetworkInterface interface {
 	GetNetDevMac(name string) string
 	// GetNetDevLinkSpeed returns the network interface link speed
 	GetNetDevLinkSpeed(name string) string
+	// GetDeviceParam returns devlink parameter for the device as a string, if the parameter has multiple values


nit: GetDevlinkDeviceParam

adrianchiris · 2024-02-22T18:53:49Z

pkg/host/internal/network/network.go

+		return "", err
+	}
+	if len(param.Values) == 0 {
+		funcLog.Error(fmt.Errorf("param %s has no value", paramName),


do we want to return here ? else code will panic if it tries to access param.Values[0]

yes, I lost return statement here. Good catch!

pkg/host/internal/network/network.go

adrianchiris · 2024-02-22T19:02:52Z

pkg/host/internal/network/network.go

+	funcLog.V(2).Info("SetDevlinkDeviceParam(): set device parameter")
+	param, err := n.netlinkLib.DevlinkGetDeviceParamByName(consts.BusPci, pciAddr, paramName)
+	if err != nil {
+		funcLog.Error(err, "SetDevlinkDeviceParam(): can get existing param data")


nit: can -> can't

adrianchiris · 2024-02-22T19:09:43Z

pkg/host/internal/network/network.go

+		return fmt.Errorf("parameter has unknown value type: %d", param.Type)
+	}
+	if err != nil {
+		err = fmt.Errorf("failed to convert value %s to the required type: %d", value, param.Type)


maybe use %T and print the type of typedValue ?

good idea, done

github-actions · 2024-02-23T08:31:30Z

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

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2024-02-23T08:46:21Z

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

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

This required to support additional configuration options and to remove need to mount /dev/ folder to the container (govdpa requires it) Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

github-actions · 2024-02-23T08:59:57Z

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

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

ykulazhenkov · 2024-02-23T09:03:14Z

@e0ne @adrianchiris I addressed your comments and rebased the PR. Please, take another look. Thx

adrianchiris

lgtm!

zeeke

LGTM

ykulazhenkov changed the title ~~[switchdev 7/x] Update netlink lib and add helper functions~~ [switchdev 6/x] Update netlink lib and add helper functions Feb 21, 2024

This was referenced Feb 22, 2024

[switchdev 7/x] Add function EnableHwTcOffload to the network pkg #628

Merged

[switchdev 8/x] Add support for switchdev configuration to sriov pkg #642

Merged

e0ne reviewed Feb 22, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/host/internal/lib/netlink/netlink.go Show resolved Hide resolved

ykulazhenkov force-pushed the pr-devlink-update branch from 2cb562f to 8682736 Compare February 22, 2024 09:57

ykulazhenkov force-pushed the pr-devlink-update branch from 8682736 to dfd1376 Compare February 22, 2024 10:15

ykulazhenkov force-pushed the pr-devlink-update branch from dfd1376 to 1f15f18 Compare February 22, 2024 10:31

adrianchiris reviewed Feb 22, 2024

View reviewed changes

ykulazhenkov force-pushed the pr-devlink-update branch from 1f15f18 to 8d90dce Compare February 23, 2024 08:31

ykulazhenkov force-pushed the pr-devlink-update branch from 8d90dce to 6bbc469 Compare February 23, 2024 08:46

ykulazhenkov added 3 commits February 23, 2024 10:47

Update netlink library

3d31324

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

Replace govdpa lib calls with netlink

7251d38

This required to support additional configuration options and to remove need to mount /dev/ folder to the container (govdpa requires it) Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

Add functions to work with devlink dev parameters

f5d2e2c

Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>

ykulazhenkov force-pushed the pr-devlink-update branch from 6bbc469 to f5d2e2c Compare February 23, 2024 08:59

ykulazhenkov requested review from e0ne and adrianchiris February 23, 2024 09:02

adrianchiris approved these changes Feb 26, 2024

View reviewed changes

zeeke approved these changes Feb 26, 2024

View reviewed changes

zeeke merged commit 0d51338 into k8snetworkplumbingwg:master Feb 26, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[switchdev 6/x] Update netlink lib and add helper functions #633

[switchdev 6/x] Update netlink lib and add helper functions #633

ykulazhenkov commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 13, 2024

coveralls commented Feb 13, 2024 •

edited

Loading

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

adrianchiris left a comment

adrianchiris Feb 22, 2024

ykulazhenkov Feb 23, 2024

adrianchiris Feb 22, 2024

ykulazhenkov Feb 23, 2024

adrianchiris Feb 22, 2024

ykulazhenkov Feb 23, 2024

adrianchiris Feb 22, 2024

ykulazhenkov Feb 23, 2024

adrianchiris Feb 22, 2024

ykulazhenkov Feb 23, 2024

adrianchiris Feb 22, 2024

ykulazhenkov Feb 23, 2024

adrianchiris Feb 22, 2024

ykulazhenkov Feb 23, 2024

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

ykulazhenkov commented Feb 23, 2024

adrianchiris left a comment

zeeke left a comment

[switchdev 6/x] Update netlink lib and add helper functions #633

[switchdev 6/x] Update netlink lib and add helper functions #633

Conversation

ykulazhenkov commented Feb 13, 2024 • edited Loading

github-actions bot commented Feb 13, 2024

coveralls commented Feb 13, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8016849082

Details

💛 - Coveralls

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

adrianchiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

ykulazhenkov commented Feb 23, 2024

adrianchiris left a comment

Choose a reason for hiding this comment

zeeke left a comment

Choose a reason for hiding this comment

ykulazhenkov commented Feb 13, 2024 •

edited

Loading

coveralls commented Feb 13, 2024 •

edited

Loading