openvswitch native bridge implementation redefines delete_flows with a different signature

[Thomas Morin]

The delete_flows method [1] in the native implementation of OVSBridge takes different parameters (including ryu objects) compared to with parent class method [2].

As a result an agent extension reusing such a bridge will fail to call delete_flows depending on which flavor (native or ovs-ofctl) the agent is configured for.

[1] https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ofswitch.py#L102

[2] https://github.com/openstack/neutron/blob/master/neutron/agent/common/ovs_lib.py#L301

[YAMAMOTO Takashi (yamamoto)]

ovs-ofctl driver also override delete_flows method.
it's intended to be reasonably compatible with ryu version.

do you have an example of delete_flows usage which doesn't work?
isn't it enough to add entries here?
https://github.com/openstack/neutron/blob/master/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/ovs_ofctl/ofswitch.py#L25

[Thomas Morin (tmmorin-orange)]

Thanks for the explanation Takashi.

An example is here:
https://review.openstack.org/#/c/377668/2/networking_bagpipe/agent/bagpipe_bgp_agent.py
(line 833)

Indeed the following fields referred to in the match aren't defined in keywords:
- proto
- arp_sha
- arp_spa

We have another patch being cooked, that needs 'arp_op, 'arp_tpa' and 'dl_vlan' as well.

To avoid surprises/issues for other extensions, perhaps it would be nice to have extensive coverage of all the possible fields (but I know this is a significant work...).

[Thomas Morin (tmmorin-orange)]

An issue I see is on the 'table' field.

2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server File "/opt/stack/neutron/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ofswitch.py", line 108, in delete_flows
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server match = self._match(ofp, ofpp, match, **match_kwargs)
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server File "/opt/stack/neutron/neutron/plugins/ml2/drivers/openvswitch/agent/openflow/native/ofswitch.py", line 100, in _match
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server return ofpp.OFPMatch(**match_kwargs)
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server File "/usr/local/lib/python2.7/dist-packages/ryu/ofproto/ofproto_v1_3_parser.py", line 847, in __init__
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server in kwargs.items()]
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server File "/usr/local/lib/python2.7/dist-packages/ryu/ofproto/oxx_fields.py", line 63, in _from_user
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server (num, t) = _get_field_info_by_name(oxx, name_to_field, name)
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server File "/usr/local/lib/python2.7/dist-packages/ryu/ofproto/oxx_fields.py", line 53, in _get_field_info_by_name
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server raise KeyError('unknown %s field: %s' % (oxx.upper(), name))
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server KeyError: 'unknown OXM field: table'
2016-09-29 09:34:59.214 TRACE oslo_messaging.rpc.server

OpenFlowSwitchMixin.delete_flows expects the table to be specified with 'table_id', while the ovs-ofctl implementation would have 'table' in the kwargs.

[Thomas Morin (tmmorin-orange)]

In my matches I have:
- 'dl_vlan=42', which would need to be translated into "vlan_vid=vlan | ofp.OFPVID_PRESENT"
- "proto='arp'" which would need to be translated into "eth_type=ether_types.ETH_TYPE_ARP".

So, it seems that adding keywords will not be sufficient, and that mapping values would also be required.

But I'm not sure how reasonable is the idea of building a complete translation machinery from ovs-ofctl syntax into ryu syntax...

[YAMAMOTO Takashi (yamamoto)]

both implementations can handle 'table_id'.
but you're right, it isn't realistic to support everything at the layer.
it's one of reasons why we chose higher-level abstractions like "provision_local_vlan".

ideally extensions should not deal with raw flows, which are implementation details of ovs-agent.
(in other words, the extension framework should provide enough machinery.)
if you need an immediate solution, i guess we can rename methods to avoid shadowing ovs_lib methods.

how do you think?

[Thomas Morin (tmmorin-orange)]

Renaming the methods seems to me as the best short-term approach.

In our specific case here, our L2 agent extension needs to inject an ARP responder flow, but with a higher priority than ARP responder flows pushed by the base agent ARP responder code. We could add a priority parameter to install_arp_responder, but that would have to be I guess for Ocata cycle. Another issue is that delete_arp_responder does not restrict flow deletion to the flows having the extension cookie (see OVSCookieBridge), so if our extension call delete_arp_responder on the native bridge, that would not only delete our extension specific ARP entry, but any possible corresponding entry setup by the base bridge. This is an issue than can be fixed as well, but again, probably not for Newton.

That said, overall, having L2 agenextensions rely on install_... higher level methods would be better, but it also means having a lot of extensions-specific install_... in the bridge code.

Perhaps a nicer way would be to allow L2 agent extensions to inject install_... methods in the two different bridge variants via stevedore entrypoint ?

[Thomas Morin (tmmorin-orange)]

One last thing: could the local method renaming be part of Newton release ?

[OpenStack Infra (hudson-openstack)]

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

[Ihar Hrachyshka (ihar-hrachyshka)]

I don't think anything will go into Newton final at this point, but we can backport later, depending on how safe a patch would look like.

[Ihar Hrachyshka (ihar-hrachyshka)]

But note that your Newton compatible code would probably need to be compatible with initial Newton release in addition to any next releases that would include the backport.

[Thomas Morin (tmmorin-orange)]

I have a patch currently cooking and planned for inclusion in our newton release, that works around the issue with an ugly:

     ovs_agent_extension_api.OVSCookieBridge.delete_flows(self.tun_br, ...)

The above will still work once the issue is resolved...

[OpenStack Infra (hudson-openstack)]

Change abandoned by Armando Migliaccio (<email address hidden>) on branch: master
Review: https://review.openstack.org/380329
Reason: This review is > 4 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

[OpenStack Infra (hudson-openstack)]

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

commit 2c54cc22ef8f9f0eb29eb0a7f8372dee515d4e25
Author: Thomas Morin <email address hidden>
Date: Fri Sep 30 16:07:20 2016 +0200

    OpenFlowSwitchMixin: do not override delete_flows

    With this change the delete_flow variant implementation
    of OpenFlowSwitchMixin, which was overriding the parent implementation
    from ovs_lib in an incompatible way using the native ryu implementation,
    is renamed into uninstall_flows.

    As discussed in bug 1628455, the approach consisting in
    extending the _keyword dict to convert ovs-ofctl rules into ryu
    parameters does not seem practical.

    This change also updates calls to delete_flows so that, when
    enabled, the native interface will be used. Similar calls outside neutron
    repo need to be updated as well, which will be done in separate changes.

    Change-Id: I90ff1055d367609694eef975c7d084e4cd7a2cf4
    Closes-Bug: 1628455
    Needed-By: Idd2315565cc9c88319984d83487148bf498e91ab

[OpenStack Infra (hudson-openstack)]

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

[Saverio Proto (zioproto)]

I am hitting this bug during my Newton to Ocata migration. Is there any Ocata patch that fixes this bug, or it is fixed only for Pike and later ? thanks

[Thomas Morin (tmmorin-orange)]

Pike only AFAICT.

To my knowledge this was happening, in Pike before the fix, with an agent extension provided in networking-bagpipe. But I'm not aware of the issue appearing with other agent extensions. Out of curiosity: with which agent extension are you hitting this bug ? (stack trace?)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/ocata
Review: https://review.openstack.org/600793

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/600793
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=245b45d81c493fb747da3fcfd8a3ec80b77036c0
Submitter: Zuul
Branch: stable/ocata

commit 245b45d81c493fb747da3fcfd8a3ec80b77036c0
Author: Thomas Morin <email address hidden>
Date: Fri Sep 30 16:07:20 2016 +0200

    OpenFlowSwitchMixin: do not override delete_flows

    With this change the delete_flow variant implementation
    of OpenFlowSwitchMixin, which was overriding the parent implementation
    from ovs_lib in an incompatible way using the native ryu implementation,
    is renamed into uninstall_flows.

    As discussed in bug 1628455, the approach consisting in
    extending the _keyword dict to convert ovs-ofctl rules into ryu
    parameters does not seem practical.

    This change also updates calls to delete_flows so that, when
    enabled, the native interface will be used. Similar calls outside neutron
    repo need to be updated as well, which will be done in separate changes.

    Change-Id: I90ff1055d367609694eef975c7d084e4cd7a2cf4
    Closes-Bug: 1628455
    Needed-By: Idd2315565cc9c88319984d83487148bf498e91ab
    (cherry picked from commit 2c54cc22ef8f9f0eb29eb0a7f8372dee515d4e25)

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron ocata-eol release.