ovs_all_ports option used by ovs cleanup has a misleading help string

[Darragh O'Reilly]

The help for ovs_all_ports says that when False, only ports that were created by Neutron are deleted.

https://github.com/openstack/neutron/blob/67b613b795416406fb4fab143b3ec9ba8657711f/neutron/conf/agent/cmd.py#L41-L46
    cfg.BoolOpt('ovs_all_ports',
                default=False,
                help=_('True to delete all ports on all the OpenvSwitch '
                       'bridges. False to delete ports created by '
                       'Neutron on integration and external network '
                       'bridges.'))

But actually ports created by Nova are also deleted. Those also have the iface-id and attached-mac fields.
https://github.com/openstack/neutron/blob/67b613b795416406fb4fab143b3ec9ba8657711f/neutron/agent/ovsdb/impl_idl.py#L80-L81

# ovs-vsctl --columns=name,external_ids --format=table list Interface
name external_ids
---------------- ---------------------------------------------------------------------------------
...
"qr-868ef235-bb" {attached-mac="fa:16:3e:bb:94:ca", iface-id="868ef235-bba4-4776-aac9-4b46a9a17def", iface-status=active}
...
"qvof43ccb24-99" {attached-mac="fa:16:3e:70:5c:b0", iface-id="f43ccb24-999e-4f9d-a9e4-07d11b9e5128", iface-status=active, vm-uuid="33c6fffa-f58b-4aab-b5ba-9960c1e02004"}

Not sure which is wrong, the help or the script. Is it supposed to delete ports that were created by nova?

[Miguel Lavalle (minsel)]

Let's look at the code: https://github.com/openstack/neutron/blob/67b613b795416406fb4fab143b3ec9ba8657711f/neutron/cmd/ovs_cleanup.py#L63-L76. 'ovs_all_ports' is used to decide what bridges are going to be cleaned-up. For each selected bridge, though, all the ports are cleaned up. And this behavior has been the same since the very beginning: https://review.opendev.org/#/c/18302/8/quantum/agent/ovs_cleanup_util.py. So the issue is the help string.

[Darragh O'Reilly (darragh-oreilly)]

If ovs_all_ports is False, only ports that have an interface with 'iface-id' and 'attached-mac' are deleted. i.e. openstack "vif" ports.

https://github.com/openstack/neutron/blob/67b613b795416406fb4fab143b3ec9ba8657711f/neutron/agent/ovsdb/impl_idl.py#L66-L68
https://github.com/openstack/neutron/blob/67b613b795416406fb4fab143b3ec9ba8657711f/neutron/agent/ovsdb/impl_idl.py#L80-L82

Also, the neutron l3 agent no longer has the option to plug router external interfaces into an external network bridge, so there should be nothing to clean on external bridges now.

https://github.com/openstack/neutron/blob/f6aef3c43452c4b8f63eecf2c7471fdedc489a14/releasenotes/notes/external_network_bridge-option-removed-bbf50fb803f04f82.yaml

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/697439

[Bence Romsics (bence-romsics)]

Unless I misread something I don't think there's a difference in which bridges get cleaned up no matter what is the value of 'ovs_all_ports'. Here:

https://github.com/openstack/neutron/blob/67b613b795416406fb4fab143b3ec9ba8657711f/neutron/cmd/ovs_cleanup.py#L65

ovs.get_bridges() lists all existing bridges, including br-int. So when ovs works properly the if-else in lines 68-71 is basically noise. The value of 'bridges' will be the same no matter if we took the if or the else. I can imagine a difference in the value of 'bridges' when the get_bridges query didn't succeed (and if an expcetion gets suppressed somewhere like in the original code linked by mlavalle). However in that case the cleanup actions likely won't work either a moment later.

[Darragh O'Reilly (darragh-oreilly)]

@Bence

I added a couple of debug statements to print the variables.

neutron.cmd.ovs_cleanup [-] ovs_bridges: set([u'br-tun', u'br-int', u'br-ex'])
neutron.cmd.ovs_cleanup [-] available_configuration_bridges: set(['br-int'])

[Darragh O'Reilly (darragh-oreilly)]

available_configuration_bridges = configuration_bridges & ovs_bridges

The & is the set intersection operator. So available_configuration_bridges can only be set(['br-int']) if br-int exists. Or set([]) if it does not exist. It can never be all bridges (ovs_bridges).

https://github.com/openstack/neutron/blob/67b613b795416406fb4fab143b3ec9ba8657711f/neutron/cmd/ovs_cleanup.py#L66

[Bence Romsics (bence-romsics)]

@Darragh: Oops, I'm sorry. I misread '&' for union. Please ignore my earlier comment.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/697439
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=4075db5fd066715fd143201590092a1cf0bdf08a
Submitter: Zuul
Branch: master

commit 4075db5fd066715fd143201590092a1cf0bdf08a
Author: Darragh O'Reilly <email address hidden>
Date: Thu Dec 5 10:35:35 2019 +0000

    Improve ovs cleanup utility help

    Fixed a couple of things for ovs_all_ports=False.
    First state that it also cleans up ports created by Nova.
    Second since [1] it only cleans up the integration bridge.

    [1] https://opendev.org/openstack/neutron/commit/b09b4460

    Change-Id: I41dda554e8cb4b4ca36515d64f17ad5bf52f3b49
    Closes-Bug: #1853582

[OpenStack Infra (hudson-openstack)]

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