[Windows] Reconcile host-network Pods after agent is restarted by wenyingd · Pull Request #6944 · antrea-io/antrea

wenyingd · 2025-01-20T08:18:54Z

This change is to support reconciling the host-network Pods on Windows because k8s expects to let CNI manage such Pods as long as they are not using host-process containers.

Antrea has received the CmdAdd request for such Pods when they are created, so they should be included in the Pod reconcile list after agent is restarted.

Fix: #6943

luolanzone · 2025-01-20T08:34:51Z

@wenyingd there is a golang-lint issue, please fix it.

wenyingd · 2025-01-20T08:37:04Z

@wenyingd there is a golang-lint issue, please fix it.

updated.

wenyingd · 2025-01-20T08:45:22Z

/test-all
/test-windows-all

pkg/agent/cniserver/server.go

luolanzone · 2025-01-20T08:48:13Z

pkg/agent/cniserver/server_windows.go

 }
+
+// getPodsListOptions returns the Pods running on the current Node. Note, the host-network Pods are not filtered
+// out on Windows because they are also managed by antrea as long as "spec.SecurityContext.windowsOptions.hostProcess"


what will happen if "spec.SecurityContext.windowsOptions.hostProcess" is configured and the Pod is not filtered out?

Nothing was performed since we didn't create OVS interfaces for such Pods.

Having offline sync with @tnqn , we would not perform other filters on Windows in reconcile logic except for the node name to get local Pods and resourceVersion=0 to avoid performance issue, this is to try our best to ensure the reconciled Pod set can cover those Antrea have handled.

wenyingd · 2025-01-20T10:15:31Z

/test-all
/test-windows-all

luolanzone

LGTM, one nit.

@tnqn @antoninbas can you take a look? thanks.

pkg/agent/cniserver/server_linux.go

wenyingd · 2025-01-21T03:50:47Z

/test-all
/test-windows-all

XinShuYang · 2025-01-21T05:17:30Z

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

wenyingd · 2025-01-21T05:42:39Z

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

antoninbas

could we add an e2e test for this as well?

antoninbas · 2025-01-21T19:15:27Z

pkg/agent/cniserver/server_windows_test.go


 	// Re-install Pod1 flows
-	podFlowsInstalled := make(chan string, 2)
+	expReInstalledPodCount := 3


nit: expectedReinstalledPodCount

reinstall is one word

updated.
I will add an e2e case to verify the host-network Pod traffic is working even if antrea-agent is restarted on Windows.

wenyingd · 2025-01-22T03:26:39Z

/test-windows-all

pkg/agent/cniserver/pod_configuration.go

XinShuYang · 2025-01-22T03:38:06Z

pkg/agent/cniserver/server_windows.go

 func (c *CNIConfig) getInfraContainer() string {
 	return getInfraContainer(c.ContainerId, c.Netns)
 }
+


wenyingd · 2025-01-22T05:34:21Z

/test-windows-e2e

wenyingd · 2025-01-22T05:48:05Z

/test-windows-e2e

wenyingd · 2025-01-22T05:51:01Z

/test-windows-all

wenyingd · 2025-01-22T09:49:30Z

/test-windows-e2e

antoninbas · 2025-01-22T21:49:50Z

test/e2e/connectivity_test.go

+	assert.NoErrorf(t, err, "The Openflow entries should be consistent after Antrea agent restarts")
+
+	data.runPingMesh(t, podInfos, toolboxContainerName, true)
+	time.Sleep(10 * time.Minute)


I think you had this for debugging purposes, that's a very long sleep :)

Yes, I will remove it after finishing the debug on the new e2e case.

antoninbas · 2025-01-22T21:54:59Z

test/e2e/connectivity_test.go

+// This case only run on Windows. The Pods in the test includes both host-network Pod and none host-network Pod,
+// because CNI on Windows is also responsible for the host-network Pod's networking as long as it is not using
+// host-process containers.
+func testWindowsPodConnectivityAfterAntreaRestart(t *testing.T, data *TestData) {


I guess one issue with the test as currently written is that if there is a failure, it's not clear which one of the 2 Pods may be responsible (assuming only 1 Pod is broken after restart).
Maybe we could have 1 Linux Pod as the client, and then use it to ping each Windows Pod independently? This way we could split this test into 2 subtests, one for the regular Pod and one for the hostNetwork Pod. Would that make sense?

Make sense, updated.

test/e2e/connectivity_test.go

wenyingd · 2025-01-23T02:56:38Z

/test-windows-all

wenyingd · 2025-01-23T04:16:50Z

/test-windows-all

wenyingd · 2025-01-23T06:59:12Z

/test-all

wenyingd · 2025-01-23T07:01:00Z

The new added e2e is succeeded, the failed case in windows-e2e is TestFQDNCacheMinTTL by issue #6891

See: http://10.164.243.223/view/Windows/job/antrea-windows-e2e-for-pull-request/89

XinShuYang

LGTM

antoninbas

couple nits, otherwise LGTM

antoninbas · 2025-01-24T22:21:14Z

test/e2e/connectivity_test.go

+	if err := data.createToolboxPodOnNode(clientPod.Name, clientPod.Namespace, clientPod.NodeName, false); err != nil {
+		t.Fatalf("Error when creating Pod '%s': %v", clientPod.Name, err)
+	}
+	defer deletePodWrapper(t, data, clientPod.Namespace, clientPod.Name)


feel free to omit this, especially for a simple Linux Pod like this one, the Pod will be automatically deleted when the test Namespace is deleted at the end of the test.

I would keep this code since personally I prefer to clean up the new resources created dedicated for the case after the test.

antoninbas · 2025-01-24T22:21:58Z

test/e2e/connectivity_test.go

+	data.runPingMesh(t, testPodInfos, toolboxContainerName, true)
+
+	// Count the OVS flows.
+	oriOVSFlows := data.dumpOVSFlows(t, winWorkerNode)


spell out original
originalOVSFlows or initialOVSFlows

Sure, updated accordingly.

antoninbas · 2025-01-24T22:23:10Z

test/e2e/connectivity_test.go

 	}
 }

+// verifyWindowsPodConnectivity checks the Pods' connectivity after antrea-agent is restarted on Windows.


checks Pod connectivity

antoninbas · 2025-01-24T22:24:17Z

test/e2e/connectivity_test.go

 }

+// verifyWindowsPodConnectivity checks the Pods' connectivity after antrea-agent is restarted on Windows.
+// The Pods in the test includes both host-network Pod and the generic Pod, because CNI on Windows is also


We test both the generic Pod case and the host-network Pod case, because ...

This change is to support reconciling the host-network Pods on Windows because k8s expects to let CNI manage such Pods as long as they are not using host-process containers. Antrea has received the CmdAdd request for such Pods when they are created, so they should be included in the Pod reconcile list after agent is restarted. Signed-off-by: Wenying Dong <wenyingd@vmware.com>

wenyingd · 2025-02-03T09:18:01Z

/test-all
/test-windows-all

antoninbas

LGTM

antoninbas · 2025-02-03T22:28:59Z

@wenyingd will you backport this?

wenyingd · 2025-02-05T01:29:44Z

@wenyingd will you backport this?

Sure, I will back port it to release-2.1 and release-2.2.

This change is to support reconciling the host-network Pods on Windows because k8s expects to let CNI manage such Pods as long as they are not using host-process containers. Antrea received the CmdAdd request for such Pods when they were created, so they should be included in the Pod reconcile list after agent is restarted. Signed-off-by: Wenying Dong <wenyingd@vmware.com>

wenyingd requested a review from tnqn January 20, 2025 08:18

wenyingd force-pushed the windows_reconcile_hostnetwork branch from 547d964 to da952ef Compare January 20, 2025 08:36

wenyingd requested a review from luolanzone January 20, 2025 08:37

luolanzone added the area/OS/windows Issues or PRs related to the Windows operating system. label Jan 20, 2025

luolanzone reviewed Jan 20, 2025

View reviewed changes

wenyingd force-pushed the windows_reconcile_hostnetwork branch from da952ef to 065af4a Compare January 20, 2025 08:55

wenyingd requested review from antoninbas and luolanzone January 20, 2025 09:40

luolanzone reviewed Jan 20, 2025

View reviewed changes

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

wenyingd force-pushed the windows_reconcile_hostnetwork branch from 065af4a to 1193dc7 Compare January 21, 2025 02:25

antoninbas reviewed Jan 21, 2025

View reviewed changes

wenyingd force-pushed the windows_reconcile_hostnetwork branch from 1193dc7 to f6c3466 Compare January 22, 2025 03:26

XinShuYang reviewed Jan 22, 2025

View reviewed changes

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

XinShuYang reviewed Jan 22, 2025

View reviewed changes

wenyingd force-pushed the windows_reconcile_hostnetwork branch from f6c3466 to c3d9724 Compare January 22, 2025 05:34

wenyingd force-pushed the windows_reconcile_hostnetwork branch from c3d9724 to 127aaf4 Compare January 22, 2025 09:47

antoninbas reviewed Jan 22, 2025

View reviewed changes

wenyingd force-pushed the windows_reconcile_hostnetwork branch from 127aaf4 to 1cf8dcf Compare January 23, 2025 02:19

wenyingd force-pushed the windows_reconcile_hostnetwork branch from 1cf8dcf to 6234815 Compare January 23, 2025 02:30

wenyingd force-pushed the windows_reconcile_hostnetwork branch from 6234815 to 471d2a7 Compare January 23, 2025 04:16

wenyingd requested review from XinShuYang and antoninbas January 23, 2025 06:59

XinShuYang previously approved these changes Jan 24, 2025

View reviewed changes

antoninbas reviewed Jan 24, 2025

View reviewed changes

wenyingd dismissed XinShuYang’s stale review via a8a7ef7 February 3, 2025 09:17

wenyingd force-pushed the windows_reconcile_hostnetwork branch from 471d2a7 to a8a7ef7 Compare February 3, 2025 09:17

antoninbas approved these changes Feb 3, 2025

View reviewed changes

antoninbas merged commit 71c4290 into antrea-io:main Feb 3, 2025
52 of 53 checks passed

antoninbas added kind/bug Categorizes issue or PR as related to a bug. action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Feb 3, 2025

wenyingd mentioned this pull request Feb 5, 2025

Automated cherry pick of #6944: Reconcile host-network Pods after agent is #6969

Merged

wenyingd mentioned this pull request Feb 5, 2025

Automated cherry pick of #6944: Reconcile host-network Pods after agent is #6970

Merged

XinShuYang mentioned this pull request Feb 17, 2025

Antrea agent crashed due to missing spec.hostNetwork label #7002

Closed

+| Pod Type                         | Spec.HostNetwork | windowsOptions.hostProcess | Kubelet CMDADD  | OVS interface needed? | Reconcile needed?          |
+| 1. Normal Pod (non-HostNetwork)  | false            | false / not set            | Yes             | Yes                   | Yes                        |
+| 2. Windows HostNetwork Pod       | true             | false / not set            | Yes             | Yes                   | Yes                        |
+| 3. Windows HostProcess Pod       | (any)            | true                       | No              | No                    | No (no real network config) |

Conversation

wenyingd commented Jan 20, 2025

Uh oh!

luolanzone commented Jan 20, 2025

Uh oh!

wenyingd commented Jan 20, 2025

Uh oh!

wenyingd commented Jan 20, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd commented Jan 20, 2025

Uh oh!

luolanzone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wenyingd commented Jan 21, 2025

Uh oh!

XinShuYang commented Jan 21, 2025

Uh oh!

wenyingd commented Jan 21, 2025

Uh oh!

antoninbas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd commented Jan 22, 2025

Uh oh!

Uh oh!

XinShuYang Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd commented Jan 22, 2025

Uh oh!

wenyingd commented Jan 22, 2025

Uh oh!

wenyingd commented Jan 22, 2025

Uh oh!

wenyingd commented Jan 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wenyingd commented Jan 23, 2025

Uh oh!

wenyingd commented Jan 23, 2025

Uh oh!

wenyingd commented Jan 23, 2025

Uh oh!

wenyingd commented Jan 23, 2025

Uh oh!

XinShuYang left a comment

Choose a reason for hiding this comment

Uh oh!

antoninbas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wenyingd Jan 22, 2025 •

edited

Loading

XinShuYang Jan 22, 2025 •

edited

Loading

wenyingd Jan 23, 2025 •

edited

Loading