[SecondaryNetwork] Attach OVS uplink after removing flow-restore-wait flag #6504

wenyingd · 2024-07-04T11:03:45Z

Physcial network interface is attached on the OVS bridge with SecondaryNetwork feature enabled. Antrea uses a global configuration flow-restore-wait='true' to ensure that OVS OpenFlow entries can start working after the dependencies are ready. A connectivity issue exists if a setup uses the Node NIC as secondary network interface and connects the NIC to OVS bridge before removing the flow-restore-wait option.

This change ensures agent attaches the physical network interface to the secondary OVS bridge after the global flow-restore-wait option is removed.

Fix: #6448

cmd/antrea-agent/agent.go

pkg/agent/secondarynetwork/init_linux.go

pkg/agent/secondarynetwork/podwatch/controller.go

wenyingd · 2024-07-05T05:41:14Z

/test-all

tnqn

LGTM, minor comments, will defer to @jianjuns for final approval

tnqn · 2024-07-05T09:26:47Z

pkg/agent/secondarynetwork/init_windows.go

 	agentconfig "antrea.io/antrea/pkg/config/agent"
 	"antrea.io/antrea/pkg/util/channel"
 )

-func Initialize(
+type Controller struct {
+	PodController *podwatch.PodController


why adding this?

We need to call Controller.PodController.Run() in cmd/antrea-agent/agent.go after exposing PodController as public. If I don't add this property in init_windows.go, it returns compile error on Windows.

It's a bit strange to call a struct's member's method from the struct's consumer, I think you should keep the Controller's Run method.

When I suggested to make podController public, I mean making it public so pkg/agent/secondary can use call the struct's method directly, instead of having to relying on another interface. I don't mean to calling podController's method directly in pkg/cmd directly.

I see, I would update.

tnqn · 2024-07-05T09:28:40Z

cmd/antrea-agent/agent.go

@@ -864,6 +865,14 @@ func run(o *Options) error {
 			return fmt.Errorf("failed to connect uplink to OVS bridge: %w", err)
 		}
 	}
+	// secondaryNetworkController Initialize must be run after FlowRestoreComplete.
+	if features.DefaultFeatureGate.Enabled(features.SecondaryNetwork) {
+		defer secondaryNetworkController.RestoreHostInterfaceConfiguration()


nit: maybe name it secondaryNetworkController.Restore() to hide details from the program entry and to be consistent with the other functions: Initialize, Run.

Sure, updated.

tnqn

LGTM

jianjuns

I think you should revise the commit message "Attach OVS uplink for after FlowRestoreComplete".

pkg/agent/secondarynetwork/init_linux.go

cmd/antrea-agent/agent.go

wenyingd · 2024-07-08T02:08:36Z

/test-all

wenyingd · 2024-07-08T07:20:00Z

/test-latest-all

jianjuns · 2024-07-08T18:43:43Z

pkg/agent/secondarynetwork/init_linux.go

+	podController   *podwatch.PodController
+}
+
+func NewController(


Could we move this one to init.go? It seems have no OS specific code?

According to the original logic, Windows does not supported SecondaryNetwork feature, so we would expect to return error on Windows when calling function NewController. If we move to init.go, we can't satisfy this precheck on Windows.

I can move the struct definition of Controller to init.go, and prefer to leave different functions of NewController in init_linux.go and init_windows.go.

What is your thought @jianjuns ?

update: I tried to move the struct of Controller to init.go, then I would hit golangci-lint failures, because the properties of ovsBridgeClient and secNetConfig are not used in init.go. So I moved it back.

If we want not Linux feature code to be in common files, then there are many more code / funcs to move to _linux.go. For example, all secondary network should be moved to _linux.go then.

So, I feel it does not help just to put NewController() to init_linux.go. For consideration of supporting Windows in future, it is also good make common code compliable on both Linux and Windows?

So your suggestion is we modify the functions in secondaryNetwork module more common in this bug fix? If this is the purpose, I feel most code in the current init_linux.go can move to init.go, the OS specific code are only for calls "util.PrepareHostInterfaceConnection" in Initialize and "RestoreHostInterfaceConfiguration" in Restore.

If we follow this direction, I would move most of the init_linux.go to init.go, and leave the two calls in _linux.go and _windows.go, is this what you want? @jianjuns

Humm.. I was just suggesting moving NewController() to init.go, if it does not have any Linux dependency. Moving Initialize() and Restore() requires more code changes and probably more dummy funcs for Windows, and so not worthwhile.

got it, I will update accordingly.

jianjuns · 2024-07-08T18:45:26Z

pkg/agent/secondarynetwork/init_linux.go

-	if len(secNetConfig.OVSBridges) != 0 && len(secNetConfig.OVSBridges[0].PhysicalInterfaces) == 1 {
-		util.RestoreHostInterfaceConfiguration(secNetConfig.OVSBridges[0].BridgeName, secNetConfig.OVSBridges[0].PhysicalInterfaces[0])
+// Run starts the Pod controller for secondary networks.
+func (c *Controller) Run(stopCh <-chan struct{}) {


Do we need a separate Run() func? Why not run the controller in Initialize()?

If we must keep it, it can be moved to init.go.

Do we need a separate Run() func? Why not run the controller in Initialize()?

From the function content perspective, "Initialize" is a one-time function, which is to initialize the secondary OVS bridge configurations including attaching the uplink to OVS bridge, and Run() is a long-run function which runs the PodController until the a signal receives by stopCh. So I would prefer to separate them into two functions.

I can move the Run function to init.go

Ok. We can follow the philosophy you suggested.

… flag Physcial network interface is attached on the OVS bridge with SecondaryNetwork feature enabled. Antrea uses a global configuration flow-restore-wait='true' to ensure that OVS OpenFlow entries can start working after the dependencies are ready. A connectivity issue exists if a setup uses the Node NIC as secondary network interface and connects the NIC to OVS bridge before removing the flow-restore-wait option. This change ensures agent attaches the physical network interface to the secondary OVS bridge after the global flow-restore-wait option is removed. Signed-off-by: Wenying Dong <[email protected]>

wenyingd · 2024-07-10T07:16:00Z

/test-all

wenyingd mentioned this pull request Jul 4, 2024

SecondaryNetwork breaks network connection of compute nodes #6448

Closed

wenyingd requested review from jianjuns and tnqn July 4, 2024 11:06

wenyingd force-pushed the issue_6448 branch from f94f391 to f925234 Compare July 4, 2024 11:17

tnqn reviewed Jul 4, 2024

View reviewed changes

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

pkg/agent/secondarynetwork/init_linux.go Outdated Show resolved Hide resolved

pkg/agent/secondarynetwork/podwatch/controller.go Outdated Show resolved Hide resolved

wenyingd force-pushed the issue_6448 branch 2 times, most recently from 10c00aa to 331a31c Compare July 5, 2024 02:09

tnqn added this to the Antrea v2.1 release milestone Jul 5, 2024

tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Jul 5, 2024

tnqn reviewed Jul 5, 2024

View reviewed changes

wenyingd force-pushed the issue_6448 branch 2 times, most recently from dde8a35 to 4c26b7a Compare July 5, 2024 14:54

tnqn previously approved these changes Jul 5, 2024

View reviewed changes

jianjuns reviewed Jul 5, 2024

View reviewed changes

pkg/agent/secondarynetwork/init_linux.go Outdated Show resolved Hide resolved

cmd/antrea-agent/agent.go Show resolved Hide resolved

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

wenyingd changed the title ~~[SecondaryNetwork] Attach OVS uplink for after FlowRestoreComplete~~ [SecondaryNetwork] Attach OVS uplink after FlowRestoreComplete Jul 8, 2024

wenyingd changed the title ~~[SecondaryNetwork] Attach OVS uplink after FlowRestoreComplete~~ [SecondaryNetwork] Attach OVS uplink after removing flow-restore-wait flag Jul 8, 2024

wenyingd dismissed tnqn’s stale review via fe84079 July 8, 2024 02:08

wenyingd force-pushed the issue_6448 branch from 4c26b7a to fe84079 Compare July 8, 2024 02:08

jianjuns reviewed Jul 8, 2024

View reviewed changes

wenyingd force-pushed the issue_6448 branch 3 times, most recently from 6bd1a4c to ad15933 Compare July 10, 2024 05:47

wenyingd force-pushed the issue_6448 branch from ad15933 to caec9c1 Compare July 10, 2024 06:23

jianjuns approved these changes Jul 10, 2024

View reviewed changes

jianjuns merged commit f4ef620 into antrea-io:main Jul 10, 2024
53 of 58 checks passed

antoninbas mentioned this pull request Aug 22, 2024

SecondaryNetwork breaks ServiceExternalIP feature #6623

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecondaryNetwork] Attach OVS uplink after removing flow-restore-wait flag #6504

[SecondaryNetwork] Attach OVS uplink after removing flow-restore-wait flag #6504

wenyingd commented Jul 4, 2024 •

edited

Loading

wenyingd commented Jul 5, 2024

tnqn left a comment

tnqn Jul 5, 2024

wenyingd Jul 5, 2024

tnqn Jul 5, 2024 •

edited

Loading

wenyingd Jul 5, 2024

tnqn Jul 5, 2024

wenyingd Jul 5, 2024

tnqn left a comment

jianjuns left a comment

wenyingd commented Jul 8, 2024

wenyingd commented Jul 8, 2024

jianjuns Jul 8, 2024

wenyingd Jul 9, 2024

wenyingd Jul 9, 2024

jianjuns Jul 9, 2024

wenyingd Jul 10, 2024 •

edited

Loading

jianjuns Jul 10, 2024

wenyingd Jul 10, 2024

jianjuns Jul 8, 2024

jianjuns Jul 8, 2024

wenyingd Jul 9, 2024

jianjuns Jul 9, 2024

wenyingd commented Jul 10, 2024

[SecondaryNetwork] Attach OVS uplink after removing flow-restore-wait flag #6504

[SecondaryNetwork] Attach OVS uplink after removing flow-restore-wait flag #6504

Conversation

wenyingd commented Jul 4, 2024 • edited Loading

wenyingd commented Jul 5, 2024

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn Jul 5, 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

tnqn left a comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

wenyingd commented Jul 8, 2024

wenyingd commented Jul 8, 2024

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 Jul 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd commented Jul 10, 2024

wenyingd commented Jul 4, 2024 •

edited

Loading

tnqn Jul 5, 2024 •

edited

Loading

wenyingd Jul 10, 2024 •

edited

Loading