OVSCookieBridge not applying extension cookie to all flow operations

[Thomas Morin]

The OVSCookieBridge class does properly the job of applying the extension-specific cookie when calls are made to add_flow/mod_flow/delete_flows.

However if methods that call self.add/mod/delete_flow() are called on an OVSCookieBridge instance, the call will be passed to the underlying bridge and the implementation in the underlying bridge will call its own add_flow method that is not cookie aware, and the extension-specific cookie will not be applied.

Better with an example:

Assuming:
- a = ovs_ofctl.br_tun.OVSTunnelBridge(...)
- c = OVSCookieBridge(a)

Then calling c.install_arp_responder(...) will result in :
- c.bridge.install_arp_responder(...)
- -> a.install_arp_responder(...)
- -> ovs_ofctl.br_tun.OVSTunnelBridge.install_arp_responder(a, ...)
- ...
- -> a.add_flow( ... ) [1]

And the cookie will not be applied because the cookie-aware add_flow method is in c, not in a.

This topic was discussed on IRC [2]

A second, related, issue is that OVSBridge subclasses touching flows with methods other than add/mod/delete_flow are not taking cookie into account at all. This is the case for instance of openflow.br_tun.OVSTunnelBridge.install_arp_responder, e.g. [3].

[1] https://github.com/openstack/neutron/blob/c44dc073fb9abd91fdcc6713b7191872f8130227/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/br_tun.py#L197

[2] http://eavesdrop.openstack.org/irclogs/%23openstack-neutron/%23openstack-neutron.2016-03-11.log.html#t2016-03-11T09:11:54

[3] https://github.com/openstack/neutron/blob/c44dc073fb9abd91fdcc6713b7191872f8130227/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/br_tun.py#L220

[Sreekumar S (sreesiv)]

Needs refactoring and fixes. I've gone through the discussion on IRC between tmorin, ihrachys and ajo.
I will work on this.

[Thomas Morin (tmmorin-orange)]

Thanks.

You can have a look at https://review.openstack.org/#/c/322048/
This is a change we brought in networking-bgpvpn to really get the wanted behavior (which I hadn't got right yet at the time of the IRC discussion you mention).

[Sreekumar S (sreesiv)]

OK, now I see the point!
Once the cookie stuff works properly, we can revert https://review.openstack.org/#/c/322048/

Got to admit, that one's real voodoo! :-)

[Thomas Morin (tmmorin-orange)]

@sreesiv: yes, and... yes :)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/326637

[Sreekumar S (sreesiv)]

@tmorin, will someone in your team be willing to pick this up? It'll be nice if someone can address/justify @Yamamoto's comments, and own it from there on.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/425756

[OpenStack Infra (hudson-openstack)]

Change abandoned by Sreekumar S (<email address hidden>) on branch: master
Review: https://review.openstack.org/326637
Reason: Patch not worth the effort. Alternative fix suggested in another patch set.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/437952
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=d761d26225a8fc63e512fc26b864a46c0956e19a
Submitter: Jenkins
Branch: master

commit d761d26225a8fc63e512fc26b864a46c0956e19a
Author: Thomas Morin <email address hidden>
Date: Thu Feb 23 17:44:38 2017 -0500

    delete_flows shall only touch flows with the bridge cookie

    With this change delete_flows will only remove flows matching the default
    cookie of the bridge.

    The uninstall_flows implementation in the native bridge is also modified
    to touch only the flows with the bridge cookie.

    To still allow deletion of all cookies, cookie=COOKIE_ANY is introduced
    as a special value, and used in the agent code in the places where the
    intent is indeed to clean all flows whatever their cookie is.

    Partial-Bug: #1557620
    Change-Id: Idd0531cedda87224531cb8fb6a912ccd0f1554d5

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/425756
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=106f6507db7db61f3fb6f55ebf6b6d9405d66079
Submitter: Jenkins
Branch: master

commit 106f6507db7db61f3fb6f55ebf6b6d9405d66079
Author: Thomas Morin <email address hidden>
Date: Thu Jan 26 15:19:54 2017 +0100

    Refactor OVSCookieBridge: always use bridge cookie

    Instead of having OVSCookieBridge as a passthrough class that does not
    provide the intended behavior (see bug 1557620), this change implements
    a cookie bridge as a patched copy of the underlying bridge:
    - the underlying bridge is copied
    - the copy is given an extension-specific cookie

    The 'extension bridge should only touch its flows' effect is obtained
    by a separate change (Idd0531cedda87224531cb8fb6a912ccd0f1554d5).

    The two problems in the bug are addressed:
    - the extension-specific cookie is now applied even for calls to
      methods other than add/delete/mod_flows
    - the extension-specific cookie is now applied in the case of the
      native/ryu implementation

    This commit also re-enable the use of uninstall_flows in the QoS OVS driver,
    which had to be disabled in Idd0531cedda87224531cb8fb6a912ccd0f1554d5, but
    can now be re-enabled with this bug addressed.

    This change complements the unit tests to confirm that the bug is
    fixed.

    Change-Id: I55835a34d8fba7a139dce93f99cbff54584d695c
    Closes-Bug: #1557620
    Needed-By: I8570441a0b8d5ee3ad7f88e07affac2f1b782021

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron 11.0.0.0b1 development milestone.