Add support for BF2 in connectX mode #353

SchSeba · 2022-08-22T12:01:06Z

Fix the issue introduced in https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/240/files#r808624002

Allow to use BF in connectX mode also for OCP platform.
This is needed until we support the systemd configuration

Signed-off-by: Sebastian Sch [email protected]

github-actions · 2022-08-22T12:01:22Z

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.

SchSeba · 2022-08-22T12:01:40Z

/cc @adrianchiris @e0ne @zshi-redhat @bn222 @zshi-redhat @zeeke

please have a look

github-actions · 2022-08-22T12:04:09Z

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 · 2022-08-22T12:07:41Z

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 · 2022-08-22T12:08:50Z

Pull Request Test Coverage Report for Build 3097548519

16 of 117 (13.68%) changed or added relevant lines in 5 files are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.06%) to 24.977%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/plugins/mellanox/mellanox_plugin.go	0	2	0.0%
pkg/utils/switchdev.go	4	6	66.67%
pkg/plugins/generic/generic_plugin.go	0	5	0.0%
pkg/utils/utils.go	12	33	36.36%
pkg/utils/utils_mlx.go	0	71	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/utils/utils.go	2	7.86%
controllers/sriovnetwork_controller.go	5	69.0%

Totals
Change from base Build 3097501597:	-0.06%
Covered Lines:	1871
Relevant Lines:	7491

💛 - Coveralls

zeeke · 2022-08-22T16:03:51Z

pkg/plugins/generic/generic_plugin.go

+	// Create a map with all the PFs we will need to configure
+	// we need to create it here before we access the host file system using the chroot function
+	// because the SkipConfigVf needs the mstconfig package that exist only inside the sriov-config-daemon file system
+	vfsToConfig := map[string]bool{}


please, rename this variable (and parameter) to vfsToSkip or something like that, as its entries seem to be true when the Virtual Functions must not be configured.

Another solution may be to make utils.SyncNodeState(...) receive ifSpecs []sriovnetworkv1.Interface, ifStatuses []sriovnetworkv1.InterfaceExt and filter out the to-skip interfcaes before calling it.

wizhaoredhat · 2022-08-22T20:57:31Z

pkg/utils/utils_mlx.go

+		return bluefield2Conntexd, nil
+	}
+
+	return -1, fmt.Errorf("MellanoxBlueFieldMode(): unknow card status for %s", PciAddress)


SP: unknown

wizhaoredhat · 2022-08-22T21:00:23Z

pkg/utils/utils_mlx.go

+		return 0, fmt.Errorf("failed to find %s in the mstconfig output command", internalCPUOffloadEngine)
+	}
+
+	// check for DPU


Just a comment: These registers aren't available with some versions of mstflint.

INTERNAL_CPU_MODEL should also be 1 as well. This is true for both DPU mode and NIC mode. Maybe we should add a check here just to be safe.

@wizhaoredhat if for both it will be 1 why do we need to check it?

It is possible that the NIC was incorrectly set to Separated Host Mode which will have a value of zero.

sounds good done

github-actions · 2022-09-01T10:21:26Z

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.

e0ne

I still need to test this PR. With this change we introduce more platform-specific and NIC-specific code which is used across the operator instead of in s specific plugins

e0ne · 2022-09-06T11:11:12Z

pkg/utils/utils.go

+			return false, nil
+		}
+
+		glog.V(2).Infof("SkipConfigVf(): skip config VF for BF2 device on DPU mode")


Neet to confirm that such use-case is not needed

what is the use case you refer ?

e0ne · 2022-09-06T11:14:21Z

pkg/plugins/generic/generic_plugin.go

+	// Create a map with all the PFs we will need to configure
+	// we need to create it here before we access the host file system using the chroot function
+	// because the SkipConfigVf needs the mstconfig package that exist only inside the sriov-config-daemon file system
+	vfsToSkip := map[string]bool{}


nit: it would be good to move this code into the separate function

pkg/plugins/generic/generic_plugin.go

github-actions · 2022-09-07T10:02:07Z

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.

SchSeba · 2022-09-07T10:03:58Z

I still need to test this PR. With this change we introduce more platform-specific and NIC-specific code which is used across the operator instead of in s specific plugins

I agree but I was not able to find a different solution, but I am optimistic that the rework I am doing will make stuff simpler

wizhaoredhat · 2022-09-07T13:28:07Z

pkg/utils/utils_mlx.go

+		glog.V(2).Infof("MellanoxBlueFieldMode():card %s in ConnectX mode", PciAddress)
+		return bluefield2Conntexd, nil
+	}
+


Just a suggestion. I believe it would help with debugging if who have some logs here (in the case of failure) what the actual mstconfig is.

adrianchiris

still need to give this one a deeper look.

one thing in mind is creating an interface to provide NIC information (e,g if its DPU or non DPU)
then keeping the vendor specific logic in plugin.

pkg/utils/utils_mlx.go

pkg/utils/switchdev.go

adrianchiris · 2022-09-08T06:52:27Z

pkg/utils/utils.go

+			return false, nil
+		}
+
+		glog.V(2).Infof("SkipConfigVf(): skip config VF for BF2 device on DPU mode")


what is the use case you refer ?

pkg/utils/utils.go

pkg/plugins/mellanox/mellanox_plugin.go

pkg/utils/utils_mlx.go

SchSeba · 2022-09-12T17:10:37Z

@adrianchiris about your comment

what is the use case you refer ?

what do you mean?

SchSeba · 2022-09-12T17:19:39Z

@bn222 done

github-actions · 2022-09-12T17:19: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.

github-actions · 2022-09-12T17:21:55Z

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.

e0ne · 2022-09-12T17:46:28Z

/test-all

pkg/utils/utils.go

github-actions · 2022-09-15T10:08:43Z

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.

e0ne · 2022-09-16T13:34:07Z

/test-all

SchSeba · 2022-09-18T11:28:51Z

@e0ne @bn222 can you make another round of review so we can merge this PR please :)

Signed-off-by: Sebastian Sch <[email protected]>

…riov-network-operator/pull/240/files#r808624002 Allow to use BF in connectX mode also for OCP platform. This is needed until we support the systemd configuration Signed-off-by: Sebastian Sch [email protected]

github-actions · 2022-09-21T11:48:42Z

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.

e0ne

Verified with vanilla k8s 1.25

bn222 · 2022-09-28T13:20:35Z

lgtm

SchSeba · 2022-09-28T16:24:28Z

Thanks for the review folks I am going to merge this PR

github-actions bot requested review from adrianchiris, bn222, e0ne and zshi-redhat August 22, 2022 12:01

SchSeba force-pushed the support_BF2_connectx branch from 84aa891 to 1c010a2 Compare August 22, 2022 12:03

SchSeba force-pushed the support_BF2_connectx branch from 1c010a2 to c12eef3 Compare August 22, 2022 12:07

zeeke reviewed Aug 22, 2022

View reviewed changes

wizhaoredhat reviewed Aug 22, 2022

View reviewed changes

SchSeba force-pushed the support_BF2_connectx branch from c12eef3 to 9a35de5 Compare September 1, 2022 10:21

e0ne reviewed Sep 6, 2022

View reviewed changes

SchSeba force-pushed the support_BF2_connectx branch from 9a35de5 to 9b5717d Compare September 7, 2022 10:01

wizhaoredhat reviewed Sep 7, 2022

View reviewed changes

adrianchiris reviewed Sep 8, 2022

View reviewed changes

bn222 reviewed Sep 12, 2022

View reviewed changes

pkg/plugins/mellanox/mellanox_plugin.go Show resolved Hide resolved

bn222 reviewed Sep 12, 2022

View reviewed changes

pkg/utils/utils_mlx.go Show resolved Hide resolved

SchSeba force-pushed the support_BF2_connectx branch from 9b5717d to 29997e3 Compare September 12, 2022 17:19

SchSeba force-pushed the support_BF2_connectx branch from 29997e3 to 374d929 Compare September 12, 2022 17:21

bn222 reviewed Sep 13, 2022

View reviewed changes

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

SchSeba force-pushed the support_BF2_connectx branch from 374d929 to e3492d1 Compare September 15, 2022 10:08

SchSeba added 2 commits September 21, 2022 14:47

move mellanox mstconfig functions to the utils_mlx.go

34b3fc3

Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the support_BF2_connectx branch from e3492d1 to 42eb7e9 Compare September 21, 2022 11:48

SchSeba mentioned this pull request Sep 21, 2022

Adding support for Octeon DPU family #355

Closed

e0ne approved these changes Sep 22, 2022

View reviewed changes

SchSeba merged commit c630e3e into k8snetworkplumbingwg:master Sep 28, 2022

Add support for BF2 in connectX mode #353

Add support for BF2 in connectX mode #353

Conversation

SchSeba commented Aug 22, 2022

github-actions bot commented Aug 22, 2022

SchSeba commented Aug 22, 2022

github-actions bot commented Aug 22, 2022

github-actions bot commented Aug 22, 2022

coveralls commented Aug 22, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3097548519

💛 - Coveralls

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 Sep 1, 2022

e0ne 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

github-actions bot commented Sep 7, 2022

SchSeba commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented Sep 12, 2022

SchSeba commented Sep 12, 2022

github-actions bot commented Sep 12, 2022

github-actions bot commented Sep 12, 2022

e0ne commented Sep 12, 2022

github-actions bot commented Sep 15, 2022

e0ne commented Sep 16, 2022

SchSeba commented Sep 18, 2022

github-actions bot commented Sep 21, 2022

e0ne left a comment

Choose a reason for hiding this comment

bn222 commented Sep 28, 2022

SchSeba commented Sep 28, 2022

coveralls commented Aug 22, 2022 •

edited

Loading