Fix bugs when Controller reconnecting to Switch #31

wenyingd · 2022-07-22T10:32:50Z

Maintain a variable for Switch connection, and close the previous
one when reconnecting to Switch. This can ensure only one unix
socket is used between Controller and Switch.
Add timeout when sending message to Switch or waiting for message
from Switch, to avoid Controller entering blocking state in a long time.

Signed-off-by: wenyingd [email protected]

wenyingd · 2022-07-22T14:04:07Z

ofctrl/fgraphSwitch.go

@@ -54,13 +54,6 @@ func (self *OFSwitch) initFgraph() error {
 	normalLookup.outputType = "normal"
 	normalLookup.portNo = openflow13.P_NORMAL
 	self.normalLookup = normalLookup
-
-	// Clear all existing flood lists
-	groupMod := openflow13.NewGroupMod()


No flooding is not configured by default, so this cleanup operation is meaningless. A separate patch will be used to clean up the other legacy code.

wenyingd · 2022-07-22T14:05:30Z

ofctrl/ofSwitch.go

-	self.changeStatus(true)
-
-	// Send new feature request
-	self.Send(openflow13.NewFeaturesRequest())


FeatureRequest is sent in the connection when Controller receives "Hello" message from Switch, so this request is not needed.

wenyingd · 2022-07-22T14:06:25Z

ofctrl/ofSwitch.go

-	// Send new feature request
-	self.Send(openflow13.NewFeaturesRequest())
-
-	self.Send(openflow13.NewEchoRequest())


Echo request is sent periodically in function switchConnected in another goroutine.

wenyingd · 2022-07-22T14:08:09Z

ofctrl/ofctrl.go

+				s := NewSwitch(stream, m.DPID, c.app, reConnChan, c.id)
+				if err := s.switchConnected(); err != nil {
+					log.Errorf("failed to initialize OpenFlow switch %s: %v", m.DPID, err)
+					s.switchDisconnected(false)


Reconnection event is sent to Controller when this function returns, so do not need to send this event inside switchDisconnected

tnqn · 2022-08-08T13:57:33Z

ofctrl/fgraphSwitch.go

-	// Clear all existing flood lists
-	groupMod := openflow15.NewGroupMod()
-	groupMod.GroupId = openflow15.OFPG_ALL
-	groupMod.Command = openflow15.OFPGC_DELETE
-	groupMod.Type = openflow15.GT_ALL
-	return self.Send(groupMod)


Is it safe to remove this? I remember agent relies on this or similar code to clean up previous installed groups so reinstalling groups won't conflict? @antoninbas

You're correct. And there is a comment in the Antrea Agent code about this: https://github.com/antrea-io/antrea/blob/454df5dd383eac93aedab4df6f5b0e4b478e46db/pkg/agent/agent.go#L465-L470

I don't know if things have changed with Openflow 1.5. If this code is not needed any more, the comment in the Antrea Agent code should be updated as well.

IIRC, this is quite easy to test. You just need to kill the antrea-agent process / container and let it restart. The idea is that the Agent should restart, without OVS restarting. So you don't want to delete the antrea-agent Pod. If there is any error during Agent initialization after the restart, then we either need this code or more complex logic in the Antrea Agent.

I don't think the group modification message is used, since it is used to delete the groups with type "all" only. I could have a try with Antrea Agent.

I didn't see errors in Antrea logs in agent restart case. But I think a better way is to remove the existing groups before replaying groups in the reconnections in Antrea Agent code. So I add this part in another change in antrea repo.

Maybe the groups are always deleted in case of reconnection, and there is nothing new here?

I checked the existing code in antrea, and I didn't find related logic to delete the existing groups in re-connect case. Before the flexible pipeine logic, I don't remember we have the logic to replay groups.

But it opens up the following questions:

is it the same for flows? When I added the cookie mechanism to delete stale flows 2+ years ago, I believe it was to solve an actual problem

Flow is different. There is a special field "cookie" in every flow, and we use several bits in the cookie to identify round number. Every time antrea is re-connected to OVS, we update the round number, then replay the flows with new cookie ID (including the new round number), and then we delete the flows with filter of the last round. Since the flows are override with the new cookie ID if they are replayed, the deletion operation only removes the flows not needed. But the cookie is not existing in group modification message.

what about meters? When we added meters for packet-in rate limiting, we had to add code to the Antrea agent to delete meters in case of restart to avoid the OFPMMFC_METER_EXISTS error: Packet-in rate limiting with OF Meter antrea#2215 (comment). Are meters treated differently from flows / groups, that seems strange.

I didn't see meter replay operations in antrea existing code.

we actually don't want flows / groups to be deleted on antrea-agent restart, to minimize traffic disruption as it can take some time to restore all flows. Can this behavior be modified by changing the fail-mode of the OVS bridge from standalone to secure?

In my understanding, standalone mode means the openflow entries are deleted once the OF controller is disconnected, and secure mode means the existing flows are deleted afterOVS is re-connected to OF Controller. The difference is if the existing flows are kept during the period when OVS is disconnected from Controller. I am not sure if the logic is also suitable on the groups and meters. But anyway, the flows are empty when antrea agent is reconnected to OVS, and this is a different behavior from the current logic.

@wenyingd I'm reading the Openflow spec and I think that the group type is ignored for a OFPGC_DELETE operation. So setting it to ALL should not matter. This code should be deleting all groups of all types.

Did you test without this code, with a restart of the antrea-agent container?

Let me try.

Even if the operation to delete existing groups is needed, I don't think is a good practice to leave this behavior in ofnet. Shall we move it to antrea? Otherwise, we have to always remember that ofnet has this pre-configuration. @antoninbas @tnqn

@wenyingd I'm reading the Openflow spec and I think that the group type is ignored for a OFPGC_DELETE operation. So setting it to ALL should not matter. This code should be deleting all groups of all types.
Did you test without this code, with a restart of the antrea-agent container?

Let me try.

@antoninbas You are correct, if I remove this code ( deleteing all groups in agent restart only case), I would got this error when replaying groups:

Received OpenFlow1.5 error: OFPGMFC_GROUP_EXISTS on message OFPT_GROUP_MOD

Moving to agent makes sense to me. Just wanted to make sure everything works as expected.

ofctrl/ofSwitch.go

ofctrl/ofTlvMap.go

ofctrl/ofctrl.go

tnqn · 2022-08-08T14:36:23Z

ofctrl/ofSwitch.go

-	self.changeStatus(true)
-
-	// Send new feature request
-	self.Send(openflow15.NewFeaturesRequest())


Is it removing a useless request the lib sent before?

Yes, NewFeaturesRequest is sent in Controller (ofctrl.go) when receiving "hello" from Switch.

Update: I removed this duplicated SwitchFeatures request, and sent the SwitchConfig and Controller setting message in function switchConnected, since this function is called when Controller received a SwitchFeature reply message. The change removes a duplicated message, but other configurations are the same.

tnqn · 2022-08-08T14:37:10Z

ofctrl/ofSwitch.go

-	// Send new feature request
-	self.Send(openflow15.NewFeaturesRequest())
-
-	self.Send(openflow15.NewEchoRequest())


Is it expected to remove the first echo request?

Yes, there is another goroutine to send echo request periodically.

tnqn

LGTM

tnqn · 2022-08-16T12:40:00Z

Same comment as https://github.com/antrea-io/antrea/pull/4091/files#r946697624

1. Maintain a variable for Switch connection, and close the previous one when reconnecting to Switch. This can ensure only one unix socket is used between Controller and Switch. 2. Add timeout when sending message to Switch or waiting for message from Switch, to avoid Controller entering blocking state in a long time. 3. Remove a duplicated SwitchFeatures request, and send SwitchConfig and Controller setting message when switch is connected. 4. Resolve OpenFlow version misconfiguration bugs in Hello message, and PortModification message. Signed-off-by: wenyingd <[email protected]>

wenyingd · 2022-08-17T03:42:09Z

Same comment as https://github.com/antrea-io/antrea/pull/4091/files#r946697624

I revert the change of removing group deletion in this patch, and move the call into function switchConnected, so that we can trigger a reconnect if this cleanup is failed since it is the initial logic for a new connection. And a separate patch would be used to optimize this part.

tnqn

LGTM

wenyingd requested a review from tnqn July 22, 2022 14:02

wenyingd commented Jul 22, 2022

View reviewed changes

wenyingd force-pushed the reconnect_issue branch from 1dfe5d5 to 46e26e2 Compare July 25, 2022 02:24

wenyingd requested a review from ashish-varma July 27, 2022 00:39

wenyingd force-pushed the reconnect_issue branch from 46e26e2 to e322bf6 Compare August 8, 2022 08:16

tnqn reviewed Aug 8, 2022

View reviewed changes

wenyingd force-pushed the reconnect_issue branch 4 times, most recently from 838a654 to 3efb2b0 Compare August 9, 2022 07:14

wenyingd requested a review from tnqn August 9, 2022 07:17

tnqn previously approved these changes Aug 16, 2022

View reviewed changes

wenyingd dismissed tnqn’s stale review via cb45146 August 17, 2022 03:14

wenyingd force-pushed the reconnect_issue branch from 3efb2b0 to cb45146 Compare August 17, 2022 03:14

tnqn approved these changes Aug 17, 2022

View reviewed changes

wenyingd merged commit 716e8cc into antrea-io:main Aug 17, 2022

wenyingd mentioned this pull request Aug 17, 2022

Resolve reconnect issue between Agent and OVS antrea-io/antrea#4091

Merged

wenyingd mentioned this pull request Dec 7, 2022

Automated cherry pick of #4126: Add OFSwitch connection check to Agent's liveness probes antrea-io/antrea#4447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs when Controller reconnecting to Switch #31

Fix bugs when Controller reconnecting to Switch #31

wenyingd commented Jul 22, 2022

wenyingd Jul 22, 2022

wenyingd Jul 22, 2022

wenyingd Jul 22, 2022

wenyingd Jul 22, 2022

tnqn Aug 8, 2022

antoninbas Aug 8, 2022

antoninbas Aug 8, 2022

wenyingd Aug 9, 2022 •

edited

Loading

wenyingd Aug 9, 2022

wenyingd Aug 16, 2022 •

edited

Loading

wenyingd Aug 16, 2022

wenyingd Aug 16, 2022

wenyingd Aug 16, 2022 •

edited

Loading

tnqn Aug 16, 2022

tnqn Aug 8, 2022

wenyingd Aug 9, 2022 •

edited

Loading

tnqn Aug 8, 2022

wenyingd Aug 9, 2022

tnqn left a comment

tnqn commented Aug 16, 2022

wenyingd commented Aug 17, 2022 •

edited

Loading

tnqn left a comment

Fix bugs when Controller reconnecting to Switch #31

Fix bugs when Controller reconnecting to Switch #31

Conversation

wenyingd commented Jul 22, 2022

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 Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd Aug 9, 2022 • edited Loading

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

tnqn commented Aug 16, 2022

wenyingd commented Aug 17, 2022 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

wenyingd Aug 9, 2022 •

edited

Loading

wenyingd Aug 16, 2022 •

edited

Loading

wenyingd Aug 16, 2022 •

edited

Loading

wenyingd Aug 9, 2022 •

edited

Loading

wenyingd commented Aug 17, 2022 •

edited

Loading