Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle vswitch reconnect gracefully (vxlan + bridge fwd mode) #101

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kahou82
Copy link
Member

@kahou82 kahou82 commented Nov 22, 2017

With vxlan bridge fwd mode, vxlanBridge will start the ovs switch object
in two of the vlan flooding objects (localFlood and allFlood). The switch object
is being populated when the first vxlan is assigned.

When user restart ovs switch, ofnet handle the reconnect which pass down the notification
to vxlanBridge object but it doesn't refresh the existing vlans to save the
new switch object. This causes netplugin stuck to talk to an unbuffered channel.

This patchset is to refresh vxlan entry to store new switch object
when ofnet handle ovs switch reconnection

Signed-off-by: Kahou Lei [email protected]

With vxlan bridge fwd mode, vxlanBridge will start the ovs switch object
in two of the vlan flooding objects (localFlood and allFlood). The switch object
is being populated when the first vxlan is assigned.

When user restart ovs switch, ofnet handle the reconnect which pass down the notification
to vxlanBridge object but it doesn't refresh the existing vlans to save the
new switch object. This causes netplugin stuck to talk to an unbuffered channel.

This patchset is to refresh vxlan entry to store new switch object
when ofnet handle ovs switch reconnection

Signed-off-by: Kahou Lei <[email protected]>
if len(self.vlanDb) != 0 {
for vlanId, vlan := range self.vlanDb {
log.Debugf(" Updating vlan %s switch object", vlanId)
var err error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since none of the variables are used outside of the if statements immediately following them, I would get rid of this error variable and have the rest like this (also added error logging):

if vlan.localFlood, err := self.ofSwitch.NewFlood(); err != nil {
	log.Errorf("Unable to assign new switch to vlan %s local flood: %v", vlanId, err)
	return
}

if vlan.allFlood, err := self.ofSwitch.NewFlood(); err != nil {
	log.Errorf("Unable to assign new switch to vlan %s all flood: %v", vlanId, err)
	return
}

Also, do we want to break instead of return? Do we want to exit the loop at all or try to continue processing the rest of the entries? continue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind about the inline assignment thing, you can't do that when assigning to struct members lol

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I was think to use os.Exit().

@unclejack
Copy link
Contributor

build PR

vxlanBridge.go Outdated
@@ -141,12 +141,12 @@ func (self *Vxlan) SwitchConnected(sw *ofctrl.OFSwitch) {
var err error
vlan.localFlood, err = self.ofSwitch.NewFlood()
if err != nil {
log.Errorf("Unable to assign new switch to vlan %s local flood", vlanId)
log.Errorf("Unable to assign new switch to vlan %v local flood", vlanId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logrus.Fatalf() does the same logging and os.exist

@unclejack
Copy link
Contributor

build PR

@dseevr
Copy link
Contributor

dseevr commented Nov 27, 2017

build pr

@unclejack
Copy link
Contributor

Is this good to go?

@dseevr
Copy link
Contributor

dseevr commented Nov 29, 2017

@kahou82 feel free to squash merge if this is all set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants