Fix to setup-vif-rules #953

Merged
merged 1 commit into from Jan 31, 2013

Conversation

Projects
None yet
6 participants
Contributor

amarao commented Dec 20, 2012

change bridge_name = "xenbr%s" % devid
to bridge_name = get_bridge_name_vswitch(vif_name)

Devid is device number for domU (f.e. vif1.15; 15 - devid) and is definitely NOT a
xenbr number (xenbr0, xenbr1, etc).

This will fix broken Multi-Tenancy feature for xenbrX and vif1.X for X !=0.

Owner

xen-git commented Dec 20, 2012

Can one of the admins verify this patch?

@ghost ghost assigned robhoes Dec 23, 2012

Owner

jonludlam commented Dec 23, 2012

Assigning to @robhoes for review

Contributor

Mattsface commented Jan 2, 2013

I just tested the patch in my lab environment. VIF level locking is working on an internal network named "xapi0".

Owner

jonludlam commented Jan 9, 2013

Thanks, @Mattsface - @robhoes , any comments?

Owner

robhoes commented Jan 14, 2013

@amarao I think the fix looks good, at least for the case where you are adding the rules/flows when a VIF is plugged. But does it also ensure that the rules/flows are removed when a VIF is unplugged or the VM is shutdown (I have seen flows being left behind even though the port was gone)? Is the bridge port for the VIF still present by the time you get into this function, so that 'ovs-vsctl iface-to-br' works, or does openvswitch remove the port as soon as the interface disappears?

Contributor

amarao commented Jan 24, 2013

AFAIK, yes.

The logic is simple: when vif is unplugged (it happens if VM is shutdowned/rebooted/migrated), setup-vif-rules called with clear command. That cause call of clear_vswitch_rules. It removes all rules for port in bridge from OVS.

I checked that patch: migration, reboot, shutdown and manual domain self-shutdown (halt command from VM) clear rules from bridge.

Owner

robhoes commented Jan 30, 2013

@amarao: I finally got a chance to try this out myself, and it looks like it indeed works as you explained. Whenever the setup-vif-rules script is called, the interface still exists, and "ovs-vsctl iface-to-br" gives the right answer.

Thanks a lot of the patch and sorry for the delay from my side! I'll merge this now.

Owner

robhoes commented Jan 30, 2013

@amarao: Oops, it seems that your patch's commit message does not have a signed-off-by line. Could you add one? After that I'll merge it. Thanks!

Contributor

mcclurmc commented Jan 30, 2013

@amarao Protip: you can automatically add a Signed-off-by line to your commit messages by doing 'git commit -s'. I can't spell 'commit' with '-s' now ;)

Fix bridge name selection for vswitch mode in scripts/setup-vif-rules
  -        bridge_name = "xenbr%s" % devid
  +        bridge_name = get_bridge_name_vswitch(vif_name)

  Devid is device number for domU (f.e. vif1.15; 15 - devid) and is definitely NOT a
  xenbr number (xenbr0, xenbr1, etc).

Signed-off-by: George Shuklin <george.shuklin@gmail.com>
Contributor

amarao commented Jan 31, 2013

Done, commit signed.

Owner

robhoes commented Jan 31, 2013

Awesome, thanks George. I'll merge this now.

robhoes added a commit that referenced this pull request Jan 31, 2013

@robhoes robhoes merged commit 47bc0e2 into xapi-project:master Jan 31, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment