[RFE] Use the new enginefacade from oslo_db

[Henry Gessau]

[Existing problem]
The oslo.db.sqlalchemy.session.EngineFacade is only a factory, and not a facade. This will make the code inconsistent, low performance, and potential bug. The details of its problems are described in [1].
Most intuitive issue is that oslo will report warning at [2]

[Proposal]
Use the new oslo_db.sqlalchemy.enginefacade to replace the current EngineFacade.

[Benefits]
A clean, less problem, higher performance db transaction.

[What is the enhancement?]
- Update neutron/db/api.py to use oslo method to create the singleton of LegacyEngineFacade.
- Update the current db transaction with new enginefacade. To avoid merge nightmare, this will be done in small pieces.
- Clean up the usage of LegacyEngineFacade

[Related information]
- [1] Oslo db spec: http://specs.openstack.org/openstack/oslo-specs/specs/kilo/make-enginefacade-a-facade.html
- [2] https://github.com/openstack/oslo.db/blob/60af1042b8123d67ed7c80d1a1720865a4255ad5/oslo_db/sqlalchemy/enginefacade.py#L956
- [3] Nova related spec: http://specs.openstack.org/openstack/nova-specs/specs/liberty/approved/oslo_db-enginefacade.html

[Hong Hui Xiao (xiaohhui)]

I found another code[1] that uses EngineFacade. Will address it too.

[1] https://github.com/openstack/neutron/blob/master/neutron/db/api.py#L44

[Henry Gessau (gessau)]

Any progress?

[Hong Hui Xiao (xiaohhui)]

The following link will be reference for this bug, will start work on it next week.

http://specs.openstack.org/openstack/oslo-specs/specs/kilo/make-enginefacade-a-facade.html

https://blueprints.launchpad.net/oslo.db/+spec/make-enginefacade-a-facade

[Hong Hui Xiao (xiaohhui)]

http://specs.openstack.org/openstack/nova-specs/specs/liberty/approved/oslo_db-enginefacade.html

[Hong Hui Xiao (xiaohhui)]

Hi Henry.

    I have updated the bug based on investigation. According to the scope and the work amount, I removed the tag "low-hanging-fruit" and add tag "rfe".

    If you think it should be another bug, I would recover this bug and create a new rfe bug to address the issue.

[Hong Hui Xiao (xiaohhui)]

remove myself as assignee to follow the RFE policy

[Henry Gessau (gessau)]

Thanks Hong Hui.

With the oslo.db BP/spec already written, I don't think we need to write a spec for Neutron.

Now I need to figure out how soon we can/should get this implemented.

[Hong Hui Xiao (xiaohhui)]

m-2 is not realistic， I think we can tag it at m-3 at first.

[Hong Hui Xiao (xiaohhui)]

I would take nova's implementation as reference, so many patches are expected for this rfe. Is it appropriate to record them all in this bug？

https://blueprints.launchpad.net/nova/+spec/new-oslodb-enginefacade

[Armando Migliaccio (armando-migliaccio)]

After browsing through what looks like Nova's switch to this I can't help but think: what a nightmare!

Do we know when the deprecation will lead to the removal of the legacy engine facade? Is it possible just to switch to the new facade but keep code changes to a minimum?

[Armando Migliaccio (armando-migliaccio)]

I think that as anything oslo related, we should make an effort to adopt the new library functions, like this one; that said I would like to advice for keeping the coding effort to a minimum. I wonder if zzzeek could suggest the best way forward.

[Hong Hui Xiao (xiaohhui)]

There are 5 alternatives mentioned in [1] to migrate to the new enginefacade. The author recommends the 1st approach, which is used by nova. But I am glad to see other comments about how this should be done.

[Armando Migliaccio (armando-migliaccio)]

Any updates?

[Hong Hui Xiao (xiaohhui)]

No updates from me, still waiting for others' comment.

[Henry Gessau (gessau)]

I will check with zzzeek and report back here.

[Sean M. Collins (scollins)]

Honestly I tried to use enginefacade in a new project (neutron-classifier) and the documentation was so short and sparse that I wasn't successful. It needs better dev docs

http://docs.openstack.org/developer/oslo.db/usage.html#session-handling

[Armando Migliaccio (armando-migliaccio)]

@Henry: do you have an update?

[Henry Gessau (gessau)]

Summary of chat with zzzeek...

The purpose is to get rid of patterns like we see in nova where there are tons of get_session() calls, all nested, each one using a totally separate transaction. The job of acquiring session / transaction scope goes away, is hidden away in one place, the application needs only declare its needs per method. The spec notes that neutron already seems to have a very succinct pattern like this.

So Neutron doesn't really have the problem that nova has.

But there is still potential advantage in declaring methods as readers or writers.

Also the initial creational pattern in legacy enginefacade is expressed internally through the new API, so we might as well use the new API for the part where we first set things up.

And also use enginefacade's system to set up the "context.session" attribute.

[Henry Gessau (gessau)]

I'll start proposing small patches and make sure zzzeek reviews them.
None of this is urgent for Mitaka for Neutron.

[Armando Migliaccio (armando-migliaccio)]

I wonder whether we should get on with this now, or put a nail in the coffin once and for all and deal with the fire drill when it happens. I'd rather do the former.

[Henry Gessau (gessau)]

Ann has started, https://review.openstack.org/300017

[Armando Migliaccio (armando-migliaccio)]

Status update:

http://eavesdrop.openstack.org/meetings/neutron_drivers/2016/neutron_drivers.2016-03-31-22.00.log.html#l-150

[Armando Migliaccio (armando-migliaccio)]

I'd like to see a plan to assess whether we're suppose to expect one or a hundred patches tackling this or a thousand or a million lines affected, but other than that, I suppose we need to be proactive and ensure the deprecation is addressed.

[Henry Gessau (gessau)]

The plan:

Step 1: Start using new engine facade.
  https://review.openstack.org/300017

Step 2: Apply reader and writer decorators.
  This will involve dozens of patches.
  First the obvious read-only and write-only sessions.
  Then the more complicated mixed sessions.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/305997

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

commit aa630f2ac88bfb4a9828b432bddfed1f04e077d3
Author: Ann Kamyshnikova <email address hidden>
Date: Thu Mar 31 16:43:41 2016 +0300

    New engine facade from oslo_db: Step 1

    Start using new engine facade.
    Existing property session for Context is needed for backward
    compatibility.
    Temporary created class ContextBaseWithSession unless
    reader and writer will be used in proper places.
    Usage of lazy init for engine facade will be removed in next patch
    set.

    Partial-Bug: #1520719

    Change-Id: I4f0693789f1c928ef47c5fdd982c147d1a9a89e1

[OpenStack Infra (hudson-openstack)]

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

commit 71ccd79103fe005b83126282ab6d6778afa26fd3
Author: Ann Kamyshnikova <email address hidden>
Date: Thu Apr 14 20:27:54 2016 +0300

    Refactor test_ipam functional testing module

    Use existing testing ifrastructure to better integrate with the rest of
    Neutron tests. This change allows test_ipam module to work with future
    db engine facade changes.

    Change-Id: I6c95cce56f36dd0c97f727c337ef6af04bc13a40
    Related-Bug: 1520719

[OpenStack Infra (hudson-openstack)]

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

commit 98b6564b5abf84ccff3b8bf1d2bf36220269cd73
Author: Ann Kamyshnikova <email address hidden>
Date: Wed May 4 12:01:03 2016 +0300

    New engine facade from oslo_db: Step 2

    Get rid of lazy init for engine facade.

    Change-Id: I73b39d51f0a2e0824ba89173808bb7e00e6150ee
    Partial-Bug: #1520719

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/339783

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/340324

[Henry Gessau (gessau)]

In the drivers meeting we decided to adopt the context manager instead of the decorator over the existing code. Here is the discussion excerpt:

http://eavesdrop.openstack.org/meetings/neutron_drivers/2016/neutron_drivers.2016-08-11-22.14.log.html

22:18:30 <kevinbenton> HenryG, armax: I think it will get done a lot quicker if we use the context managers
22:18:46 <HenryG> I hope so, let's try that
22:18:47 <kevinbenton> using the decorators means the whole function has to be 'PRECOMMIT' so-to-speak
22:18:51 <armax> kevinbenton: it depends if that’s what zzzeek was proposing?
22:18:56 <kevinbenton> which means every one requires a refactor
22:19:24 <kevinbenton> armax: well the difference between the context manager and the decorator is just syntax
22:19:27 <armax> kevinbenton: yes, that’s something I questioned
22:19:33 <kevinbenton> they both use the new reader/writer thingy
22:19:44 <kevinbenton> so we still get the advantages of that
22:20:14 <HenryG> I don't know if zzzeek knew how many of our methods do things after commiting before they return
22:20:35 <kevinbenton> yeah, or did things before the transaction started
22:21:35 <kevinbenton> but really the difference between the decorator vs the context manager is just syntactic sugar
22:21:47 <kevinbenton> the important thing is switching to the new facade
22:23:42 <armax> as for the way we leverage it we can either go with what kevinbenton is suggesting
22:23:45 <armax> which seems less invasive
22:24:02 <armax> or go with what zzeeek was suggesting, which is more invasive?
22:24:40 <HenryG> it requires refactoring, yes
22:24:48 <kevinbenton> there is almost no benefit to using the decorator instead of the manager. the only thing i can see is that it makes it a little easier to reason about a function being entirely enclosed in a transaction
22:25:26 <kevinbenton> so even if we could do the decorator really quickly, i would be against it this late in the cycle due to all of the code churn it will cause
22:25:54 <HenryG> the "plan" would be 'search and replace' using the context manager
22:25:59 <armax> kevinbenton: we can’t simply apply the decorator blindly though, can we?
22:26:00 <kevinbenton> HenryG: +1
22:26:06 <kevinbenton> armax: that's my point
22:26:16 <kevinbenton> armax: decorator we can't apply without thinking and refactoring
22:26:21 <armax> kevinbenton: right, that goes back to the refactoring need, which is a mess
22:26:23 <kevinbenton> armax: context manager we pretty much can
22:26:42 <armax> do we lose any benefit if we used the context manager based approver over decorators?
22:26:52 <armax> then we can still make a mandate that new junk uses the decorator
22:26:57 <armax> can the two co-exist?
22:27:02 <kevinbenton> no different
22:27:11 <kevinbenton> they are just different entry points into the same class
22:27:14 <kevinbenton> i looked into it
22:27:16 <HenryG> Using the decorator forces a cleaner transaction design IMO
22:27:37 <armax> right, so if it’s cleaner/more readable to use the decorator then we can suggest using it during code review
22:27:45 <armax> and apply it where it’s easier to do so
22:27:51 <amuller> agreed
22:27:56 <amuller> it's two different...

[OpenStack Infra (hudson-openstack)]

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

commit ac69b228b058b4a8cf74f5c5a6b56d4f4a9a0b0d
Author: Ann Kamyshnikova <email address hidden>
Date: Mon Jul 11 14:43:35 2016 +0300

    Refactor setting OSprofiler for db calls

    This change is depends on change in oslo.db that adds option to
    set hook for osprofiler.

    Depends-On: I78bef4979c2000d05658ce17d0348cd0a10c24d9

    Related-bug: #1600739

    Related-bug: #1520719

    Change-Id: Ie971654f988c98015edc7a59cf00b27e83c0c1b7

[OpenStack Infra (hudson-openstack)]

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

commit d8d3c193b2c2b5bf814602ac4f5446e2a333a057
Author: AKamyshnikova <email address hidden>
Date: Fri Jul 8 20:33:04 2016 +0300

    Get rid of get_engine() in db/api.py

    get_engine() can be called directly with
    context_manager.get_legacy_facade().get_engine()

    Related-bug: #1520719

    Change-Id: Id38c06aee428200a061c59a984b59b81b24056e3

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/379651

[OpenStack Infra (hudson-openstack)]

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

commit 7d17f8c7c3436dd3bb8a26500f5a1e7706e4eb9e
Author: Dariusz Smigiel <email address hidden>
Date: Thu Sep 29 18:32:17 2016 +0000

    Removed get_engine() from db/api

    This function was deprecated in change
    Id38c06aee428200a061c59a984b59b81b24056e3
    get_engine() can be called directly with
    context_manager.get_legacy_facade().get_engine()

    Change-Id: I7ef45c7ced65a3a3746e616c7b796a34b3551d3c
    Related-bug: #1520719