Enforce router admin_state_up=False before distributed update

[Matt Welch]

Until now, the API has allowed changing the distributed attribute of a router in the same request with changing its admin-state-up attribute to False. This does not seem to cause a problem when upgrading a router from centralized to distributed, but downgrading a distributed router to centralized can cause an issue.

When the distributed attribute is set to false, then a notification should be sent to the node hosting the centralized router or all of the nodes hosting the distributed router. If the distributed attribute is set to false, the notification will only to one node, resulting in all the other instances of the distributed router getting stuck configured as distributed. This can affect communication to VM instances until the L3 agent realizes the router is no longer set to distributed and cleans up the node. This delay can be as long as the length of the agent audit interval.

Rather than remembering the old state of the router, we have chosen to enforce that the user update the admin-state-up attribute to false before changing the distributed attribute. Effectively, routers will need to be in admin-state-up=False to modify the distributed attribute. Once disabled, the user can modify the distributed attribute and admin state in a single request.

Before this change the following was possible:

    neutron router-update tenant2-router --distributed false --admin-state-up false
    neutron router-update tenant2-router --admin-state-up true

After this change the following is required:

    neutron router-update tenant2-router --admin-state-up false
    neutron router-update tenant2-router --distributed false --admin-state-up true

Horizon workflows may need a similar restriction.

*Version*
OpenStack installed via devstack, git hash d1fe5ad507c6dcb6955d66fab0b6bc9fb59a80f2
Neutron git hash 77a8b020c3da77fd0329ae349a2cab952e4bc37a

[Boden R (boden)]

Have you spoken to anyone on the neutron drivers team regards this change?
IMHO it seems borderline RFE and appears to change the API behavior.

Before going too far with the work on this one, we should probably have someone from the drivers team confirm it.

[Miguel Lavalle (minsel)]

Matt and I had a brief irc chat about this RFE. IWe will get to it soon. Thanks

[LIU Yulong (dragon889)]

Fix starts here:
https://review.openstack.org/#/c/625134/

[Miguel Lavalle (minsel)]

Looking at the code (https://github.com/openstack/neutron/blob/master/neutron/db/l3_dvr_db.py#L86) it seems that what the submitter proposes is the original intent. Since it has already been marked as RFE, let's discuss it in the drivers meeting just to be on the safe side

[Miguel Lavalle (minsel)]

This submission was discussed today by the drivers team. We agreed that it is not a RFE. It is a bug, since the proposed behavior was what the code was originally intended to do. Having said that, the current API behavior has been living in the wild for a long time. As a consequence, if we are going to change it to what is should have been, we need to make it discoverable. Thus the fix should include a shim API extension. An example of how this has to be implemented in neutron-lib and Neutron can be found here https://github.com/openstack/neutron-lib/blob/master/neutron_lib/api/definitions/empty_string_filtering.py and here https://github.com/openstack/neutron/blob/master/neutron/extensions/empty_string_filtering.py.

We also want to see scenario tests added to https://github.com/openstack/neutron-tempest-plugin/blob/master/neutron_tempest_plugin/scenario/test_migration.py

[Matt Welch (mattw4)]

Thank you to the Neutron drivers team for the input.
I agree with the conclusion of writing a shim api extension.

I did some digging into the empty_string_filtering extension and, other than the files linked above, I've found a method "is_empty_string_supported()" in neutron/api/api_common.py.
That method shows that we can checking if the extension alias is present in extensions.PluginAwareExtensionManager.get_instance().extensions.
Is this the standard method that I should use to discover extensions or should we also have a configuration bit that enables/disables a particular extension?

Finally, in the proposed fix of "_validate_router_migration()", should I include a switch that preserves the old behavior in the case that the "enforce_router_admin_state_down" shim extension is disabled?

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/634509
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=adc6a7087f0b7aa07730e25a9b946157bbf98c53
Submitter: Zuul
Branch: master

commit adc6a7087f0b7aa07730e25a9b946157bbf98c53
Author: Matt Welch <email address hidden>
Date: Fri Feb 1 14:44:05 2019 -0800

    Add shim extension admin_state_down_before_update

    Shim extension for neutron patch:
    https://review.openstack.org/#/c/625134/
    Adds releasenote.
    Adds api-ref update.

    Related-Bug: #1811166

    Change-Id: Ie1167a9c7fc5dcb88a6955085937572805e41fd7
    Signed-off-by: Matt Welch <email address hidden>

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/625134
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=00b6460df2748cbf0bfa8fccdaad6a7356c15388
Submitter: Zuul
Branch: master

commit 00b6460df2748cbf0bfa8fccdaad6a7356c15388
Author: Matt Welch <email address hidden>
Date: Thu Dec 13 15:52:34 2018 -0800

    Enforce router admin state before distributed

    Enforce that a user updates the admin state of a router before modifying
    the distributed state. The API currently allows setting admin state to
    false concurrently with changing the distributed state.
    This is fine for a transition of centralized->distributed, but the
    distributed->centralized transition could leave other nodes configured
    as distributed until an audit is performed.

    Commit adds shim api extension which should be replaced by neutron-lib
    shim extension once https://review.openstack.org/#/c/634509/ is merged.
    New method 'is_admin_state_down_necessary' checks that shim extension
    is loaded.

    Set extension as standard by adding to _supported_extension_aliases in
    neutron/services/l3_router/l3_router_plugin.py

    Closes-Bug: #1811166
    Co-Authored-By: Allain Legacy <email address hidden>
    Co-Authored-By: Enyinna Ochulor <email address hidden>
    Change-Id: Ie624aeb3f3aeb4db176d2ca0b22020208d4b408a
    Signed-off-by: Matt Welch <email address hidden>

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/744901

[OpenStack Infra (hudson-openstack)]

Change abandoned by Slawek Kaplonski (<email address hidden>) on branch: master
Review: https://review.opendev.org/744901
Reason: This review is > 4 weeks without comment, and failed Zuul jobs 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.opendev.org/744901
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=531d337db5fec668437beba48f65134d44b255db
Submitter: Zuul
Branch: master

commit 531d337db5fec668437beba48f65134d44b255db
Author: elajkat <email address hidden>
Date: Wed Aug 5 14:57:19 2020 +0200

    Remove left over code for admin_state_down_before_update

    The extension in neutron-lib was released in 1.29.0 so it is time to
    use the extension from neutron-lib.

    Change-Id: Id2bd872646feede7179affe8c7d124b4530afc9d
    Related-Bug: #1811166