[RFE] Use the new enginefacade from oslo_db

Bug #1520719 reported by Henry Gessau on 2015-11-28
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
neutron
Wishlist
Ann Taraday

Bug Description

[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

Henry Gessau (gessau) on 2015-11-28
tags: added: db
Changed in neutron:
importance: Undecided → Low
tags: added: low-hanging-fruit
Hong Hui Xiao (xiaohhui) on 2015-11-30
Changed in neutron:
assignee: nobody → Hong Hui Xiao (xiaohhui)
Changed in neutron:
status: New → Confirmed
Hong Hui Xiao (xiaohhui) wrote :

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) wrote :

Any progress?

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.

summary: - OsloDBDeprecationWarning: EngineFacade is deprecated
+ Use the new enginefacade from oslo_db
description: updated
tags: added: rfe
removed: low-hanging-fruit
summary: - Use the new enginefacade from oslo_db
+ [RFE]Use the new enginefacade from oslo_db
Changed in neutron:
status: Confirmed → New
Hong Hui Xiao (xiaohhui) wrote :

remove myself as assignee to follow the RFE policy

Changed in neutron:
assignee: Hong Hui Xiao (xiaohhui) → nobody
Henry Gessau (gessau) on 2015-12-22
Changed in neutron:
status: New → Confirmed
importance: Low → Wishlist
Henry Gessau (gessau) wrote :

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) wrote :

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

Changed in neutron:
assignee: nobody → Hong Hui Xiao (xiaohhui)
Hong Hui Xiao (xiaohhui) wrote :

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

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?

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) wrote :

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.

Any updates?

Hong Hui Xiao (xiaohhui) wrote :

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

Henry Gessau (gessau) wrote :

I will check with zzzeek and report back here.

Sean M. Collins (scollins) wrote :

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

@Henry: do you have an update?

Henry Gessau (gessau) wrote :

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) wrote :

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

Henry Gessau (gessau) on 2016-03-24
Changed in neutron:
assignee: Hong Hui Xiao (xiaohhui) → Henry Gessau (gessau)
Henry Gessau (gessau) on 2016-03-24
summary: - [RFE]Use the new enginefacade from oslo_db
+ [RFE] Use the new enginefacade from oslo_db

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.

Changed in neutron:
status: Confirmed → Triaged
Henry Gessau (gessau) wrote :
Changed in neutron:
assignee: Henry Gessau (gessau) → Ann Kamyshnikova (akamyshnikova)

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.

tags: added: deprecation rfe-approved
removed: rfe
Changed in neutron:
milestone: none → newton-1
Changed in neutron:
status: Triaged → In Progress
Henry Gessau (gessau) wrote :

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.

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

Changed in neutron:
milestone: newton-1 → newton-2

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

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

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

Changed in neutron:
milestone: newton-2 → newton-3
Henry Gessau (gessau) wrote :
Download full text (4.0 KiB)

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...

Read more...

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

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

Changed in neutron:
milestone: newton-3 → newton-rc1
Changed in neutron:
milestone: newton-rc1 → ocata-1

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

Changed in neutron:
milestone: ocata-1 → ocata-2
Changed in neutron:
milestone: ocata-2 → ocata-3
Changed in neutron:
milestone: ocata-3 → ocata-rc1
milestone: ocata-rc1 → pike-1
Changed in neutron:
milestone: pike-1 → pike-2
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers