Skip config if policy not applied yet #287

rollandf · 2022-04-26T13:31:32Z

There is a stage when the SriovNetworkNodeState is initializing
where the spec is empty because the SriovNetworkNodePolicyReconciler
did not yet applied the policies.

It can cause a non required action by the plugins,
that will try to apply the empty spec by resetting the NIC for example.

The config daemon will not run the plugins if the generation is 1
and the Spec.Interfaces is empty.

Solves issue #283

For e2e tests, the wait timeout to get to initial Sync state has been
increased.
This change is needed as now the config daemon will not apply on "empty"
spec until the SriovNetworkNodePolicyReconciler will iterate on the Interfaces.
The reconcile loop interval is 5 minutes, so the test timeout needed to be increased.

Signed-off-by: Fred Rolland frolland@nvidia.com

github-actions · 2022-04-26T13:31: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.

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

github-actions · 2022-04-26T14:04:45Z

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

LGTM

adrianchiris · 2022-04-28T06:52:15Z

/test-all

abdallahyas · 2022-04-28T07:27:29Z

/test-all

e0ne · 2022-04-28T12:17:25Z

/test-all

pkg/daemon/daemon.go

adrianchiris · 2022-05-08T09:30:18Z

one minor nit, otherwise LGTM

adrianchiris · 2022-05-08T10:52:43Z

/test-all

github-actions · 2022-05-08T10:57: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.

abdallahyas · 2022-05-08T11:41:02Z

@adrianchiris @rollandf i am not sure if it is related, but in last CI failure, the sriovnetworknodestate was missing the syncStatus field, see sriovnetworknodestate,
we also see


I0508 11:08:50.751581    1394 writer.go:163] setNodeStateStatus(): syncStatus: , lastSyncError: 
I0508 11:08:56.009989    1394 daemon.go:311] get item: 1
I0508 11:08:56.010049    1394 daemon.go:401] nodeStateSyncHandler(): new generation is 1
I0508 11:08:56.023458    1394 daemon.go:424] nodeStateSyncHandler(): Interface policy spec not yet set
I0508 11:08:56.023491    1394 daemon.go:346] Successfully synced
I0508 11:08:56.023509    1394 daemon.go:309] worker queue size: 0
I0508 11:08:56.116046    1394 daemon.go:284] Run(): period refresh
I0508 11:08:56.121430    1394 daemon.go:985] tryCreateSwitchdevUdevRule()
I0508 11:08:56.121583    1394 daemon.go:1043] tryCreateNMUdevRule()
I0508 11:09:11.010630    1394 daemon.go:311] get item: 1
I0508 11:09:11.010672    1394 daemon.go:401] nodeStateSyncHandler(): new generation is 1
I0508 11:09:11.019890    1394 daemon.go:424] nodeStateSyncHandler(): Interface policy spec not yet set
I0508 11:09:11.019917    1394 daemon.go:346] Successfully synced
I0508 11:09:11.019928    1394 daemon.go:309] worker queue size: 0

in the config daemon logs, even though the VFs were configured, see kind worker interfaces

abdallahyas · 2022-05-09T06:21:52Z

/test-all

abdallahyas · 2022-05-09T06:50:02Z

/test-all

github-actions · 2022-05-09T08:09:11Z

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.

abdallahyas · 2022-05-09T08:10:29Z

/test-all

test/e2e/sriovoperatornodepolicy_test.go

github-actions · 2022-05-09T09:11:15Z

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-05-09T09:14:45Z

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.

rollandf · 2022-05-09T10:14:00Z

/test-all

abdallahyas · 2022-05-09T10:36:07Z

/test-all

adrianchiris · 2022-05-11T09:15:11Z

test/e2e/sriovoperatornodepolicy_test.go

@@ -32,7 +32,7 @@ var _ = Describe("Operator", func() {
 		Eventually(func() *cluster.EnabledNodes {
 			sriovInfos, _ = cluster.DiscoverSriov(clients, testNamespace)
 			return sriovInfos
-		}, timeout, interval).ShouldNot(BeNil())
+		}, Timeout*15, RetryInterval*20).ShouldNot(BeNil())


With this change, sriovnetworknodestate will be reconciled by config daemon only after the controller set the spec (empty or other)

reason for timeout increase is for controller to resync nodestate with the actual spec and allow config daemon to reconcile.

see
#305

@rollandf question : do we need this now that we are updating sync status immediately to success on generation 1 ?

adrianchiris

LGTM

adrianchiris · 2022-06-01T07:27:31Z

@SchSeba @Eoghan1232 can we merge this ?

controllers/sriovnetworknodepolicy_controller.go

adrianchiris · 2022-06-28T10:02:40Z

/test-all

github-actions · 2022-07-04T12:30: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.

adrianchiris · 2022-07-04T12:31:02Z

/test-all

github-actions · 2022-07-10T07:54: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.

github-actions · 2022-07-10T08:04:15Z

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 · 2022-07-10T08:22:15Z

/test-all

SchSeba

just a small nit beside that LGTM

SchSeba · 2022-07-10T13:56:59Z

controllers/sriovnetworknodepolicy_controller.go

@@ -278,6 +278,11 @@ func (r *SriovNetworkNodePolicyReconciler) syncSriovNetworkNodeState(np *sriovne
 			return fmt.Errorf("failed to get SriovNetworkNodeState: %v", err)
 		}
 	} else {
+		if len(found.Status.Interfaces) == 0 {
+			logger.Info("SriovNetworkNodeState Status Interfaces are empty. Skip update of policies in spec")


nit: can you please add here "name", ns.Name

github-actions · 2022-07-10T14:18: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.

github-actions · 2022-07-11T08:05: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.

rollandf · 2022-07-12T13:10:49Z

/test-all

e0ne · 2022-07-12T19:09:11Z

/test-all

rollandf · 2022-07-13T05:56:17Z

@SchSeba Can you please take another look?

SchSeba · 2022-07-13T07:58:02Z

/lgtm

adrianchiris

final question about latest changes and i think we can merge

adrianchiris · 2022-07-13T11:59:41Z

test/e2e/sriovoperatornodepolicy_test.go

@@ -32,7 +32,7 @@ var _ = Describe("Operator", func() {
 		Eventually(func() *cluster.EnabledNodes {
 			sriovInfos, _ = cluster.DiscoverSriov(clients, testNamespace)
 			return sriovInfos
-		}, timeout, interval).ShouldNot(BeNil())
+		}, Timeout*15, RetryInterval*20).ShouldNot(BeNil())


@rollandf question : do we need this now that we are updating sync status immediately to success on generation 1 ?

There is a stage when the SriovNetworkNodeState is initializing where the spec is empty because the SriovNetworkNodePolicyReconciler did not yet applied the policies. It can cause a non required action by the plugins, that will try to apply the empty spec by resetting the NIC for example. The config daemon will not run the plugins if the generation is 1 and the Spec.Interfaces is empty. Solves issue k8snetworkplumbingwg#283 For e2e tests, the wait timeout to get to initial Sync state has been increased. This change is needed as now the config daemon will not apply on "empty" spec until the SriovNetworkNodePolicyReconciler will iterate on the Interfaces. The reconcile loop interval is 5 minutes, so the test timeout needed to be increased. Signed-off-by: Fred Rolland <frolland@nvidia.com>

github-actions · 2022-07-13T12:20: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.

adrianchiris · 2022-07-13T13:00:45Z

/test-all

adrianchiris

This has been a long one :) LGTM

e0ne requested changes Apr 26, 2022

View reviewed changes

config/crd/bases/sriovnetwork.openshift.io_sriovnetworknodestates.yaml Outdated Show resolved Hide resolved

rollandf force-pushed the nodestatepolicy branch from 16654d4 to c605ce4 Compare April 26, 2022 14:04

e0ne approved these changes Apr 27, 2022

View reviewed changes

adrianchiris reviewed May 8, 2022

View reviewed changes

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

rollandf force-pushed the nodestatepolicy branch from c605ce4 to 1373336 Compare May 8, 2022 10:57

rollandf force-pushed the nodestatepolicy branch from 1373336 to 6f893b8 Compare May 9, 2022 08:08

abdallahyas reviewed May 9, 2022

View reviewed changes

test/e2e/sriovoperatornodepolicy_test.go Outdated Show resolved Hide resolved

rollandf force-pushed the nodestatepolicy branch from 6f893b8 to 4a29c30 Compare May 9, 2022 09:11

rollandf force-pushed the nodestatepolicy branch from 4a29c30 to 89fdb17 Compare May 9, 2022 09:14

adrianchiris approved these changes May 11, 2022

View reviewed changes

adrianchiris reviewed May 11, 2022

View reviewed changes

adrianchiris approved these changes May 11, 2022

View reviewed changes

adrianchiris reviewed Jun 28, 2022

View reviewed changes

controllers/sriovnetworknodepolicy_controller.go Show resolved Hide resolved

rollandf force-pushed the nodestatepolicy branch from c2ebb2a to 6e725d2 Compare July 4, 2022 12:29

rollandf force-pushed the nodestatepolicy branch from 6e725d2 to decb44b Compare July 10, 2022 07:54

rollandf force-pushed the nodestatepolicy branch from decb44b to bb60bfd Compare July 10, 2022 08:04

adrianchiris approved these changes Jul 10, 2022

View reviewed changes

SchSeba reviewed Jul 10, 2022

View reviewed changes

rollandf force-pushed the nodestatepolicy branch from bb60bfd to 66b8535 Compare July 10, 2022 14:17

rollandf force-pushed the nodestatepolicy branch from 66b8535 to 78b25d1 Compare July 11, 2022 08:05

SchSeba approved these changes Jul 13, 2022

View reviewed changes

github-actions bot added the lgtm label Jul 13, 2022

adrianchiris reviewed Jul 13, 2022

View reviewed changes

rollandf force-pushed the nodestatepolicy branch from 78b25d1 to 47ac214 Compare July 13, 2022 12:20

adrianchiris approved these changes Jul 14, 2022

View reviewed changes

adrianchiris merged commit 9440e86 into k8snetworkplumbingwg:master Jul 14, 2022

SchSeba mentioned this pull request Dec 21, 2023

SriovNetworkNodePolicyReconciler should watch changes in SriovNetworkNodeState #305

Closed

Skip config if policy not applied yet #287

Skip config if policy not applied yet #287

Conversation

rollandf commented Apr 26, 2022 • edited Loading

github-actions bot commented Apr 26, 2022

github-actions bot commented Apr 26, 2022

e0ne left a comment

Choose a reason for hiding this comment

adrianchiris commented Apr 28, 2022

abdallahyas commented Apr 28, 2022

e0ne commented Apr 28, 2022

adrianchiris commented May 8, 2022

adrianchiris commented May 8, 2022

github-actions bot commented May 8, 2022

abdallahyas commented May 8, 2022 • edited Loading

abdallahyas commented May 9, 2022

abdallahyas commented May 9, 2022

github-actions bot commented May 9, 2022

abdallahyas commented May 9, 2022

github-actions bot commented May 9, 2022

github-actions bot commented May 9, 2022

rollandf commented May 9, 2022

abdallahyas commented May 9, 2022

adrianchiris May 11, 2022

Choose a reason for hiding this comment

adrianchiris Jul 13, 2022

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris commented Jun 1, 2022

adrianchiris commented Jun 28, 2022

github-actions bot commented Jul 4, 2022

adrianchiris commented Jul 4, 2022

github-actions bot commented Jul 10, 2022

github-actions bot commented Jul 10, 2022

adrianchiris commented Jul 10, 2022

SchSeba left a comment

Choose a reason for hiding this comment

SchSeba Jul 10, 2022

Choose a reason for hiding this comment

rollandf Jul 10, 2022

Choose a reason for hiding this comment

github-actions bot commented Jul 10, 2022

github-actions bot commented Jul 11, 2022

rollandf commented Jul 12, 2022

e0ne commented Jul 12, 2022

rollandf commented Jul 13, 2022

SchSeba commented Jul 13, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris Jul 13, 2022

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2022

adrianchiris commented Jul 13, 2022

adrianchiris left a comment

Choose a reason for hiding this comment

rollandf commented Apr 26, 2022 •

edited

Loading

abdallahyas commented May 8, 2022 •

edited

Loading