[Backport][bug/1393925] Race condition adding a security group rule when another is in-progress

[Alexander Ignatov]

This patch is to back-port fix for bug: https://bugs.launchpad.net/neutron/+bug/1393925

Original description:
================

I've come across a race condition where I sometimes see a security group rule is never added to iptables, if the OVS agent is in the middle of applying another security group rule when the RPC arrives.

Here's an example scenario:

nova boot --flavor 1 --image $nova_image dev_server1
sleep 4
neutron security-group-rule-create --direction ingress --protocol tcp --port_range_min 1111 --port_range_max 1111 default
neutron security-group-rule-create --direction ingress --protocol tcp --port_range_min 1112 --port_range_max 1112 default

Wait for VM to complete booting, then check iptables:

$ sudo iptables-save | grep 111
-A neutron-openvswi-i741ff910-1 -p tcp -m tcp --dport 1111 -j RETURN

The second rule is missing, and will only get added if you either add another rule, or restart the agent.

My config is just devstack, running with the latest openstack bits as of today. OVS agent w/vxlan and DVR enabled, nothing fancy.

I've been able to track this down to the following code (i'll attach the complete log as a file due to line wraps):

OVS agent receives RPC to setup port
    Port info is gathered for devices and filters for security groups are created
        Iptables "apply" is called
        New security group rule is added, triggering RPC message
        RPC received, and agent seems to add device to list that needs refresh

            Security group rule updated on remote: [u'5f0f5036-d14c-4b57-a855-ed39deaea256'] security_groups_rule_updated
            Security group rule updated [u'5f0f5036-d14c-4b57-a855-ed39deaea256']
            Adding [u'741ff910-12ba-4c1e-9dc9-38f7cbde0dc4'] devices to the list of devices for which firewall needs to be refreshed _security_group_updated

        Iptables "apply" is finished

rpc_loop() in OVS agent does not notice there is more work to do on next loop, so rule never gets added

At this point I'm thinking it could be that self.devices_to_refilter is modified in both _security_group_updated() and setup_port_filters() without any lock/semaphore, but the log doesn't explicity implicate it (perhaps we trust the timestamps too much?).

I will continue to investigate, but if someone has an "aha!" moment after reading this far please add a note.

A colleague here has also been able to duplicate this on his own devstack install, so it wasn't my fat-fingering that caused it.

[OSCI Robot (oscirobot)]

RPM package neutron has been built for project openstack/neutron
Package version == 2014.2, package release == fuel6.0.mira11.git.9a1eb5d.a299407

Changeset: https://review.fuel-infra.org/1160
project: openstack/neutron
branch: openstack-ci/fuel-6.0/2014.2
author: Alexander Ignatov
committer: Alexander Ignatov
subject: Fix a race condition adding a security group rule
status: patchset-created

Files placed on repository:
openstack-neutron-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-bigswitch-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-brocade-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-cisco-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-hyperv-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-ibm-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-linuxbridge-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-mellanox-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-metaplugin-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-metering-agent-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-midonet-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-ml2-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-nec-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-nuage-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-ofagent-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-oneconvergence-nvsd-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-opencontrail-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-openvswitch-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-plumgrid-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-ryu-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-vmware-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
openstack-neutron-vpn-agent-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm
python-neutron-2014.2-fuel6.0.mira11.git.9a1eb5d.a299407.noarch.rpm

NOTE: Changeset is not merged, created temporary package repository.
RPM repository URL: http://osci-obs.vm.mirantis.net:82/centos-fuel-6.0-stable-1160/centos

[OSCI Robot (oscirobot)]

DEB package neutron has been built for project openstack/neutron
Package version == 2014.2, package release == fuel6.0~mira10+git.9a1eb5d.a299407

Changeset: https://review.fuel-infra.org/1160
project: openstack/neutron
branch: openstack-ci/fuel-6.0/2014.2
author: Alexander Ignatov
committer: Alexander Ignatov
subject: Fix a race condition adding a security group rule
status: patchset-created

Files placed on repository:
neutron-common_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-dhcp-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-l3-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-lbaas-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-metadata-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-metering-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-bigswitch-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-bigswitch_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-brocade_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-cisco_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-hyperv_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-ibm-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-ibm_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-linuxbridge-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-linuxbridge_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-metaplugin_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-metering-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-midonet_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-ml2_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-mlnx-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-mlnx_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-nec-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-nec_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-nicira_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-oneconvergence-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-oneconvergence_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-openflow-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-openvswitch-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-openvswitch_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-plumgrid_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-ryu-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-ryu_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-vmware_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-plugin-vpn-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-server_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
neutron-vpn-agent_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb
python-neutron_2014.2-fuel6.0~mira10+git.9a1eb5d.a299407_all.deb

NOTE: Changeset is not merged, created ...