Avoid mixed usage of old and new transaction styles

Bug #1744829 reported by Lujin Luo on 2018-01-23
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
neutron
High
Lujin Luo

Bug Description

The newly merged (distributed) Port Binding OVO integration patch [1] is mixing the old nested transaction style used by OVO with the new engine facade transactions. According to what we learnt in https://review.openstack.org/#/c/529169 and https://review.openstack.org/#/c/532343, this shouldn't be done.

A patch to change the new engine facade transactions back to old nested transaction style is required.

[1] https://review.openstack.org/#/c/407868

Lujin Luo (luo-lujin) on 2018-01-23
Changed in neutron:
assignee: nobody → Lujin Luo (luo-lujin)
Miguel Lavalle (minsel) on 2018-01-23
Changed in neutron:
importance: Undecided → High
status: New → Confirmed
Miguel Lavalle (minsel) wrote :

Lujin Luo,

Thanks for filing this bug. Per our conversation in IRC of moments ago, while we need to fix the specific buggy code introduced by https://review.openstack.org/#/c/407868, we also need to think about some follow up questions:

1) Is it better for our users a ML2 plugin with OVO for port bindings and old engine facade than the one we had before with new engine facade and old style DB code?

2) In the longer term, does this mean that every time we adopt OVO in a piece of code, we are going to undo the good work done in adopting the new engine facade?

Both OVO and the new engine facade are good things that we certainly want to adopt in Neutron. We are painting ourselves into a corner where we have to choose between one or the other. I pinged Matt Riedemann earlier today and asked him about Nova's adoption of new engine facade and OVO. It is complete and they co-exist nicely. This event should prompt us to think how we can get Neutron in the same position. I think this should be a topic that we discuss in Dublin and put together a solid plan to move ahead with both OVO and new engine facade

Changed in neutron:
status: Confirmed → In Progress
Slawek Kaplonski (slaweq) wrote :

One (maybe silly) question: why OVO can't use new enginefacade also? Why we need this nested transactions inside OVO methods?

Slawek Kaplonski (slaweq) wrote :

@Miguel: I added this topic to Rocky planning etherpad: https://etherpad.openstack.org/p/neutron-ptg-rocky to not forget it :)

There is no particular reason for base OVO layer to use those old style transactions. It just never got to the point where someone posted a patch to switch it that would pass the gate. (I believe Anna had some drafts but they never went to git.)

Change abandoned by Lujin Luo (<email address hidden>) on branch: master
Review: https://review.openstack.org/536679
Reason: As we are reverting the patch in https://review.openstack.org/#/c/536913/ we no longer need this temporary fix.

Miguel Lavalle (minsel) on 2018-01-26
Changed in neutron:
milestone: none → queens-rc1

Reviewed: https://review.openstack.org/536913
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=906eda44d2c230be99fc5c61daa0f359ab46a3ad
Submitter: Zuul
Branch: master

commit 906eda44d2c230be99fc5c61daa0f359ab46a3ad
Author: Ihar Hrachyshka <email address hidden>
Date: Tue Jan 23 17:46:40 2018 +0000

    Revert "Integration of (Distributed) Port Binding OVO"

    This reverts commit febeaf5d4016157d0cca41023e64b91e550453e8.

    This patch broke postgres tempest jobs, as well as introduced potential
    race conditions in database layer because of mixed usage of old and new
    engine facades.

    Related-Bug: #1744829
    Change-Id: Ic142ae7faf4e5f10cbdf761d7e6f3d442e94a3eb

Miguel Lavalle (minsel) on 2018-02-01
Changed in neutron:
status: In Progress → Fix Committed
Thomas Morin (tmmorin-orange) wrote :

The revert introduces a regression in networking-bagpipe/networking-bgpvpn:
https://bugs.launchpad.net/networking-bagpipe/+bug/1746996

Changed in neutron:
status: Fix Committed → Fix Released
tags: added: neutron-proactive-backport-potential
tags: removed: neutron-proactive-backport-potential
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers