Parallel NICs configuration #497

e0ne · 2023-09-05T10:24:11Z

No description provided.

github-actions · 2023-09-05T10:24:24Z

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 · 2023-09-19T11:59:40Z

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 · 2023-09-19T13:58:29Z

UPDATE: environment variables exist in pod after node reboot

github-actions · 2023-09-20T18:25:56Z

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 · 2023-09-20T18:37: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.

coveralls · 2023-09-20T18:44:55Z

Pull Request Test Coverage Report for Build 7916806140

Details

-170 of 173 (1.73%) changed or added relevant lines in 4 files are covered.
12 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.3%) to 29.846%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cmd/sriov-network-config-daemon/start.go	1	3	33.33%
controllers/sriovoperatorconfig_controller.go	2	4	50.0%
api/v1/zz_generated.deepcopy.go	0	7	0.0%
pkg/host/internal/sriov/sriov.go	0	159	0.0%

Files with Coverage Reduction	New Missed Lines	%
api/v1/zz_generated.deepcopy.go	2	42.96%
controllers/sriovnetwork_controller.go	2	70.68%
api/v1/helper.go	3	43.73%
pkg/host/internal/sriov/sriov.go	5	7.06%

Totals
Change from base Build 7875814354:	-0.3%
Covered Lines:	3546
Relevant Lines:	11881

💛 - Coveralls

github-actions · 2023-09-25T16:55:46Z

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 · 2023-10-18T13:56: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 · 2023-10-18T14:55: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 · 2023-10-18T15:18: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.

github-actions · 2023-10-18T15:51: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 · 2023-10-18T16:38:17Z

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.

api/v1/helper.go

zeeke

Left a few comments

zeeke · 2023-10-31T16:12:16Z

pkg/utils/utils.go

+	if !parallelNicConfig {
+		return ConfigSriovInterfaces(newState.Spec.Interfaces, newState.Status.Interfaces, pfsToConfig)
+	}
+	return ConfigSriovInterfacesInParallel(newState.Spec.Interfaces, newState.Status.Interfaces, pfsToConfig)


nit: use positve condition for readability

Suggested change

if !parallelNicConfig {

return ConfigSriovInterfaces(newState.Spec.Interfaces, newState.Status.Interfaces, pfsToConfig)

}

return ConfigSriovInterfacesInParallel(newState.Spec.Interfaces, newState.Status.Interfaces, pfsToConfig)

if parallelNicConfig {

return ConfigSriovInterfacesInParallel(newState.Spec.Interfaces, newState.Status.Interfaces, pfsToConfig)

}

return ConfigSriovInterfaces(newState.Spec.Interfaces, newState.Status.Interfaces, pfsToConfig)

zeeke · 2023-10-31T16:14:23Z

pkg/utils/utils.go

+							result = resetErr
+						}
+					}
+					wg.Done()


Please, put it in a defer statement at the beginning of the function

zeeke · 2023-10-31T16:23:03Z

pkg/utils/utils.go

+				go func(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetworkv1.InterfaceExt) {
+					if err := configSriovDevice(iface, ifaceStatus); err != nil {
+						glog.Errorf("ConfigSriovInterfacesInParallel(): fail to configure sriov interface %s: %v. resetting interface.", iface.PciAddress, err)
+						result = err


What about using errgroup.Group to handle this synchronization? I think it would be useful, as we are dealing with errors here.

using result is problematic, multiple goroutines may update this at the same time

we need to re-visit our API to include muliple results in SriovNetworkNodeState

zeeke · 2023-10-31T16:26:19Z

pkg/utils/utils.go

+						}
+					}
+					wg.Done()
+				}(&iface, &ifaceStatus)


It's not a good idea to use pointers here, as the variable address is always the same. Please, take a look at this modified example of WaitGroup: https://go.dev/play/p/8CuE98kz-Eg. If you try running it, you'll get the same print every time:

http://www.example.com/ http://www.example.com/ http://www.example.com/ Program exited.

thanks, fixed

github-actions · 2023-11-06T07:06:53Z

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 · 2023-11-06T07:57:24Z

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.

bindata/manifests/daemon/daemonset.yaml

config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml

adrianchiris · 2023-11-07T15:59:47Z

pkg/utils/utils.go

+				go func(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetworkv1.InterfaceExt) {
+					if err := configSriovDevice(iface, ifaceStatus); err != nil {
+						glog.Errorf("ConfigSriovInterfacesInParallel(): fail to configure sriov interface %s: %v. resetting interface.", iface.PciAddress, err)
+						result = err


using result is problematic, multiple goroutines may update this at the same time

pkg/utils/utils.go

github-actions · 2024-02-05T10:38: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-05T10:39: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.

github-actions · 2024-02-05T10:52:00Z

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-06T14:20: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.

adrianchiris

overall looks good @e0ne !
added some comments, nothing major.

api/v1/zz_generated.deepcopy.go

adrianchiris · 2024-02-07T14:47:35Z

controllers/sriovoperatorconfig_controller.go

@@ -187,6 +187,10 @@ func (r *SriovOperatorConfigReconciler) syncConfigDaemonSet(ctx context.Context,
 	} else {
 		data.Data["UsedSystemdMode"] = false
 	}
+	data.Data["ParallelNicConfig"] = false
+	if parallelConfig, ok := dc.Spec.FeatureGates["parallelNicConfig"]; ok {


can we have "parallelNicConfig" as a constant in const package ?

pkg/host/internal/sriov/sriov.go

adrianchiris · 2024-02-07T15:05:12Z

pkg/host/internal/sriov/sriov.go

+	return nil
+}
+
+func checkNeedUpdate(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetworkv1.InterfaceExt, storeManager store.ManagerInterface) (bool, error) {


can you add docstring for added methods ?

also from the name "checkNeedUpdate" i expected that if true is returned then we need to update, but its actually the opposite... but i see also sriovnetworkv1.NeedToUpdateSriov has same meaning...

pkg/host/internal/sriov/sriov.go

github-actions · 2024-02-09T13:28:33Z

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.

FeatureGates is a map to enable experimental features.

github-actions · 2024-02-12T09:31:48Z

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-12T11:44:34Z

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.

pkg/host/internal/sriov/sriov.go

github-actions · 2024-02-13T12:42:49Z

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

LGTM !

Thx @e0ne :)

adrianchiris · 2024-02-15T08:16:27Z

@zeeke could take a look ?

zeeke

Left a few minor comments

pkg/host/internal/sriov/sriov.go

github-actions · 2024-02-15T13:38:00Z

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.

zeeke

LGTM. merging

Changes to CustomResourceDefinition have been introduced by - k8snetworkplumbingwg/sriov-network-operator#497 - k8snetworkplumbingwg/sriov-network-operator#569 Signed-off-by: Andrea Panattoni <[email protected]>

e0ne requested a review from SchSeba September 5, 2023 10:24

e0ne force-pushed the parallel-nic-config branch from 42bf306 to ca9ace5 Compare September 19, 2023 11:59

e0ne force-pushed the parallel-nic-config branch from ca9ace5 to 37a73d8 Compare September 20, 2023 18:25

e0ne force-pushed the parallel-nic-config branch from 37a73d8 to e2b7dd4 Compare September 20, 2023 18:36

e0ne force-pushed the parallel-nic-config branch from e2b7dd4 to c753939 Compare September 25, 2023 16:55

e0ne changed the title ~~Parallel NICs configuraion~~ Parallel NICs configuration Oct 17, 2023

e0ne force-pushed the parallel-nic-config branch from c753939 to 851bc86 Compare October 18, 2023 13:56

e0ne force-pushed the parallel-nic-config branch from 851bc86 to 59acc63 Compare October 18, 2023 14:55

e0ne force-pushed the parallel-nic-config branch from 59acc63 to 11dba38 Compare October 18, 2023 15:17

e0ne force-pushed the parallel-nic-config branch from 11dba38 to e039dc5 Compare October 18, 2023 15:50

e0ne force-pushed the parallel-nic-config branch from e039dc5 to 9ca38c4 Compare October 18, 2023 16:38

adrianchiris reviewed Oct 19, 2023

View reviewed changes

api/v1/helper.go Outdated Show resolved Hide resolved

zeeke reviewed Oct 31, 2023

View reviewed changes

e0ne force-pushed the parallel-nic-config branch from 9ca38c4 to 5cc64b9 Compare November 6, 2023 07:06

e0ne force-pushed the parallel-nic-config branch from 5cc64b9 to 1e08d75 Compare November 6, 2023 07:57

adrianchiris reviewed Nov 7, 2023

View reviewed changes

e0ne force-pushed the parallel-nic-config branch from 0f10346 to 5be6a80 Compare February 5, 2024 10:38

e0ne force-pushed the parallel-nic-config branch from 5be6a80 to 79347dd Compare February 5, 2024 10:39

e0ne force-pushed the parallel-nic-config branch from 79347dd to 6ff8524 Compare February 5, 2024 10:51

e0ne force-pushed the parallel-nic-config branch from 6ff8524 to f6bfd53 Compare February 6, 2024 14:19

adrianchiris reviewed Feb 7, 2024

View reviewed changes

e0ne force-pushed the parallel-nic-config branch from f6bfd53 to aafe487 Compare February 9, 2024 13:28

Add FeatureGates field into SriovOperatorConfig CRD

801845b

FeatureGates is a map to enable experimental features.

e0ne force-pushed the parallel-nic-config branch from aafe487 to 01eb91f Compare February 12, 2024 09:31

e0ne force-pushed the parallel-nic-config branch from 01eb91f to 8132982 Compare February 12, 2024 11:44

adrianchiris reviewed Feb 13, 2024

View reviewed changes

pkg/host/internal/sriov/sriov.go Outdated Show resolved Hide resolved

e0ne force-pushed the parallel-nic-config branch from 8132982 to 1e4308a Compare February 13, 2024 12:42

adrianchiris approved these changes Feb 14, 2024

View reviewed changes

zeeke reviewed Feb 15, 2024

View reviewed changes

pkg/host/internal/sriov/sriov.go Outdated Show resolved Hide resolved

pkg/host/internal/sriov/sriov.go Outdated Show resolved Hide resolved

Parallel NICs configuration

0aead49

e0ne force-pushed the parallel-nic-config branch from 1e4308a to 0aead49 Compare February 15, 2024 13:37

zeeke approved these changes Feb 15, 2024

View reviewed changes

zeeke merged commit 3852228 into k8snetworkplumbingwg:master Feb 15, 2024
12 checks passed

zeeke mentioned this pull request Feb 22, 2024

OCPBUGS-25986: Downstream sync 2024-02-20 openshift/sriov-network-operator#887

Merged

Parallel NICs configuration #497

Parallel NICs configuration #497

Conversation

e0ne commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 19, 2023

e0ne commented Sep 19, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

coveralls commented Sep 20, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7916806140

Details

💛 - Coveralls

github-actions bot commented Sep 25, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

github-actions bot commented Oct 18, 2023

zeeke 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

github-actions bot commented Nov 6, 2023

github-actions bot commented Nov 6, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 6, 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

github-actions bot commented Feb 9, 2024

github-actions bot commented Feb 12, 2024

github-actions bot commented Feb 12, 2024

github-actions bot commented Feb 13, 2024

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris commented Feb 15, 2024

zeeke left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 15, 2024

zeeke left a comment

Choose a reason for hiding this comment

coveralls commented Sep 20, 2023 •

edited

Loading