[Windows] CNI Server installs OpenFlow entries by PortStatus #6763

wenyingd · 2024-10-23T02:00:03Z

This change has introduced a worker in podConfigurator to listen for the OpenFlow PortStatus message when a new OpenFlow port is allocated in OVS. After receiving the message, antrea-agent Windows will install Pod related OpenFlow entries. If the OpenFlow port is not allocated within 30s after the CmdAdd request is responded, an event with type "NetworkNotReady" is added on the Pod; Whenever the Pod networking forwarding rules are installed, an event with type "NetworkIsReady" is added.

Fix: #6721

pkg/agent/cniserver/pod_configuration_windows.go

pkg/agent/cniserver/pod_configuration.go

pkg/agent/cniserver/server_windows_test.go

pkg/agent/cniserver/pod_configuration_windows.go

antoninbas · 2024-10-24T22:56:08Z

pkg/agent/cniserver/pod_configuration_windows.go

+	// unReadyInterfaces is a map to store the OVS ports which is waiting for the PortStatus from OpenFlow switch.
+	// The key in the map is the OVS port name, and its value is unReadyPodInfo.
+	// It is used only on Windows now.
+	unReadyInterfaces sync.Map


But addUnReadyPodInterface is called from a separate goroutine, no? You cannot have a map be updated from different goroutines.

pkg/agent/cniserver/pod_configuration_windows.go

antoninbas · 2024-10-24T22:57:25Z

pkg/agent/cniserver/pod_configuration_windows.go

+	}()
+}
+
+func (m *podIfaceMonitor) updatePodFlows(ifName string, ofPort int32) error {


that's fair, I was trying to reduce the number of dependencies that need to be injected into the pod monitor

we can leave it as a future change

pkg/agent/cniserver/pod_configuration.go

pkg/agent/cniserver/pod_monitor.go

pkg/agent/cniserver/pod_configuration_windows.go

pkg/agent/cniserver/pod_monitor.go

pkg/agent/cniserver/pod_monitor_windows.go

pkg/agent/cniserver/pod_monitor.go

XinShuYang · 2024-10-29T03:44:15Z

pkg/agent/cniserver/pod_monitor.go

+	go wait.Until(m.worker, time.Second, stopCh)
+
+	go wait.Until(func() {
+		ticker := time.Tick(retryInterval)


Using time.Tick without stopping the ticker can cause memory leaks. Can we replace it with time.NewTicker, and make sure to defer ticker.Stop()?

The ticker is removed in the latest change as Antonin suggested.

pkg/agent/cniserver/pod_monitor.go

wenyingd · 2024-10-29T07:59:40Z

/test-windows-all

wenyingd · 2024-10-29T09:29:23Z

/test-windows-networkpolicy
/test-windows-e2e

XinShuYang · 2024-10-29T09:38:18Z

/test-windows-e2e

pkg/agent/cniserver/pod_configuration_windows.go

pkg/agent/cniserver/pod_monitor.go

wenyingd · 2024-11-05T10:53:10Z

/test-all

tnqn

LGTM after the remaining comments on tests are addressed

pkg/agent/cniserver/pod_configuration.go

pkg/agent/cniserver/pod_configuration_linux.go

tnqn · 2024-11-05T14:01:46Z

pkg/agent/cniserver/pod_configuration_test.go

+	kubeClient := fakeclientset.NewClientset()
+	for _, obj := range objects {
+		if obj != nil {
+			kubeClient.Tracker().Add(obj)
+		}
+	}


it doesn't seem updated

pkg/agent/cniserver/pod_configuration_test.go

pkg/agent/cniserver/pod_configuration_windows.go

pkg/agent/cniserver/server.go

pkg/agent/cniserver/server_windows_test.go

pkg/agent/cniserver/pod_configuration_test.go

pkg/agent/cniserver/server_windows_test.go

…ge is received This change has introduced a worker in `podConfigurator` to listen for the OpenFlow PortStatus message when a new OpenFlow port is allocated in OVS. After receiving the message, antrea-agent Windows will install Pod related OpenFlow entries. If the OpenFlow port is not allocated within 30s after the CmdAdd request is responded, an event with type "NetworkNotReady" is added on the Pod; Whenever the Pod networking forwarding rules are installed, an event with type "NetworkIsReady" is added. Signed-off-by: Wenying Dong <[email protected]>

tnqn

LGTM

wenyingd · 2024-11-06T06:23:48Z

/test-all

wenyingd · 2024-11-06T10:39:30Z

@antoninbas Could you help take another look at this change?

antoninbas

Only very minor comments, so I will go ahead and merge this

antoninbas · 2024-11-06T21:26:45Z

pkg/agent/cniserver/pod_configuration_test.go

+			if tc.installOpenFlowErr != nil {
+				require.Equal(t, 1, configurator.unreadyPortQueue.Len())
+				key, _ := configurator.unreadyPortQueue.Get()
+				assert.Equal(t, key, podIfName)


nit: for future reference, the expected value should come first in assert.Equal

antoninbas · 2024-11-06T21:28:43Z

pkg/agent/cniserver/pod_configuration_test.go

+			var gotEvent string
+			select {
+			case gotEvent = <-testClients.recorder.Events:
+			default:
+			}


a comment indicating that recording events happens synchronously and hence a timeout is not necessary would have been nice here

antoninbas · 2024-11-06T21:32:13Z

pkg/agent/cniserver/pod_configuration_windows.go

 		}
 	}
 }
+
+// initPortStatusMonitor has subscribed a channel to listen for the OpenFlow PortStatus message, and it also


Suggested change

// initPortStatusMonitor has subscribed a channel to listen for the OpenFlow PortStatus message, and it also

// initPortStatusMonitor subscribes a channel to listen for the OpenFlow PortStatus message, and it also

antoninbas · 2024-11-06T21:35:25Z

I think we need to run Windows tests again

antoninbas · 2024-11-06T21:35:31Z

/test-windows-all

wenyingd · 2024-11-07T03:02:47Z

@antoninbas Windows CI testbed is down because of the limited resource, it may need another 1d to redeploy the setup and run tests. I have verified the change 3 days ago on Windows testbed, and the later change is mostly about the test code. So maybe we can bypass the re-run?

windows-e2e/windows-networkpolicy had passed, windows-conformance was failed because the dependent image was not able to download.

antoninbas · 2024-11-07T04:05:02Z

@wenyingd yes we can merge without the testing in that case, but we will need to run the Windows CI jobs with this change prior to the release. cc @luolanzone

XinShuYang · 2024-11-08T02:34:30Z

@wenyingd yes we can merge without the testing in that case, but we will need to run the Windows CI jobs with this change prior to the release. cc @luolanzone

The Windows tests have passed with the latest Antrea code. Due to resource limitations, we are unable to deploy the Windows testbed before the 2.2 release. After discussion, we plan to skip Windows tests before this release, and there will be no additional Windows-related patches in release 2.2.

wenyingd requested review from antoninbas, tnqn and XinShuYang October 23, 2024 02:00

wenyingd force-pushed the async_ofport branch 2 times, most recently from 370e578 to 4fcbe6f Compare October 23, 2024 03:45

antoninbas reviewed Oct 23, 2024

View reviewed changes

wenyingd force-pushed the async_ofport branch from 4fcbe6f to 9ee313d Compare October 24, 2024 11:29

antoninbas reviewed Oct 24, 2024

View reviewed changes

tnqn reviewed Oct 25, 2024

View reviewed changes

pkg/agent/cniserver/pod_configuration.go Outdated Show resolved Hide resolved

pkg/agent/cniserver/pod_configuration.go Outdated Show resolved Hide resolved

wenyingd force-pushed the async_ofport branch 2 times, most recently from 7ed399f to cb09094 Compare October 25, 2024 12:34

antoninbas reviewed Oct 25, 2024

View reviewed changes

wenyingd force-pushed the async_ofport branch from cb09094 to 5c1fd59 Compare October 28, 2024 11:10

antoninbas reviewed Oct 28, 2024

View reviewed changes

XinShuYang reviewed Oct 29, 2024

View reviewed changes

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

XinShuYang reviewed Oct 29, 2024

View reviewed changes

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

wenyingd force-pushed the async_ofport branch from 5c1fd59 to b384c73 Compare October 29, 2024 04:16

wenyingd requested a review from antoninbas October 29, 2024 06:07

tnqn reviewed Oct 29, 2024

View reviewed changes

pkg/agent/cniserver/pod_configuration_windows.go Show resolved Hide resolved

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

XinShuYang reviewed Oct 30, 2024

View reviewed changes

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

XinShuYang reviewed Oct 30, 2024

View reviewed changes

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

XinShuYang reviewed Oct 30, 2024

View reviewed changes

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

XinShuYang reviewed Oct 30, 2024

View reviewed changes

pkg/agent/cniserver/pod_monitor.go Outdated Show resolved Hide resolved

wenyingd force-pushed the async_ofport branch 3 times, most recently from 2bc767d to d9ea32b Compare October 30, 2024 10:50

wenyingd force-pushed the async_ofport branch 2 times, most recently from c81059a to 3d917bf Compare November 5, 2024 03:16

wenyingd requested review from antoninbas and tnqn November 5, 2024 03:18

wenyingd force-pushed the async_ofport branch 3 times, most recently from 162600d to 6f59f5e Compare November 5, 2024 10:45

tnqn reviewed Nov 5, 2024

View reviewed changes

wenyingd force-pushed the async_ofport branch from 6f59f5e to 136785b Compare November 6, 2024 02:13

wenyingd requested a review from tnqn November 6, 2024 02:15

wenyingd force-pushed the async_ofport branch from 136785b to 41c13f8 Compare November 6, 2024 02:52

tnqn reviewed Nov 6, 2024

View reviewed changes

wenyingd force-pushed the async_ofport branch from 41c13f8 to dee4e5f Compare November 6, 2024 04:00

wenyingd force-pushed the async_ofport branch from dee4e5f to 73e93a9 Compare November 6, 2024 04:02

wenyingd requested a review from tnqn November 6, 2024 05:31

tnqn approved these changes Nov 6, 2024

View reviewed changes

antoninbas approved these changes Nov 6, 2024

View reviewed changes

antoninbas merged commit 85e437c into antrea-io:main Nov 7, 2024
57 checks passed

antoninbas added area/OS/windows Issues or PRs related to the Windows operating system. kind/bug Categorizes issue or PR as related to a bug. action/release-note Indicates a PR that should be included in release notes. labels Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] CNI Server installs OpenFlow entries by PortStatus #6763

[Windows] CNI Server installs OpenFlow entries by PortStatus #6763

wenyingd commented Oct 23, 2024 •

edited

Loading

antoninbas Oct 24, 2024

antoninbas Oct 24, 2024

XinShuYang Oct 29, 2024

wenyingd Oct 29, 2024

wenyingd commented Oct 29, 2024

wenyingd commented Oct 29, 2024

XinShuYang commented Oct 29, 2024

wenyingd commented Nov 5, 2024

tnqn left a comment

tnqn Nov 5, 2024

tnqn left a comment

wenyingd commented Nov 6, 2024

wenyingd commented Nov 6, 2024

antoninbas left a comment

antoninbas Nov 6, 2024

antoninbas Nov 6, 2024

antoninbas Nov 6, 2024

antoninbas commented Nov 6, 2024

antoninbas commented Nov 6, 2024

wenyingd commented Nov 7, 2024 •

edited

Loading

antoninbas commented Nov 7, 2024

XinShuYang commented Nov 8, 2024

	// initPortStatusMonitor has subscribed a channel to listen for the OpenFlow PortStatus message, and it also
	// initPortStatusMonitor subscribes a channel to listen for the OpenFlow PortStatus message, and it also

[Windows] CNI Server installs OpenFlow entries by PortStatus #6763

[Windows] CNI Server installs OpenFlow entries by PortStatus #6763

Conversation

wenyingd commented Oct 23, 2024 • edited Loading

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

wenyingd commented Oct 29, 2024

wenyingd commented Oct 29, 2024

XinShuYang commented Oct 29, 2024

wenyingd commented Nov 5, 2024

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Nov 6, 2024

wenyingd commented Nov 6, 2024

antoninbas 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

antoninbas commented Nov 6, 2024

antoninbas commented Nov 6, 2024

wenyingd commented Nov 7, 2024 • edited Loading

antoninbas commented Nov 7, 2024

XinShuYang commented Nov 8, 2024

wenyingd commented Oct 23, 2024 •

edited

Loading

wenyingd commented Nov 7, 2024 •

edited

Loading