Should use object interfaces instead of DBAPI interfaces

Bug #1314732 reported by Chris Behrens
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ironic
Fix Released
Medium
Lucas Alvares Gomes

Bug Description

There's a mix of using the ironic/objects interfaces to retrieve objects and using dbapi to retrieve objects. This should all be fixed to use the objects interfaces where possible. The DBAPI interfaces should really be hidden underneath objects.

The interfaces for using objects needs to be made consistent as well. This means making sure the object stores 'context' when it is instantiated. And removing 'context' from being used on .refresh() and .save() calls.

To fix this all, things need to be done in a certain order:

1) Make sure appropriate methods exist in our objects that match current dbapi behavior.
2) Switch code to use the object interfaces. Pass the context on the calls that instantiate the object.
3) Remove the 'objectify' decorator from DB API methods. Right now, since dbapi is returning objects, the object code is sometimes converting an object to an object (basically a no-op). Since DBAPI methods don't get the 'context', we also can't
reasonably form the object, storing the context, if we're doing it at this layer.
4) Remove 'context' from .refresh() and .save().

aeva black (tenbrae)
Changed in ironic:
status: New → In Progress
importance: Undecided → Medium
assignee: nobody → Chris Behrens (cbehrens)
Revision history for this message
Openstack Gerrit (openstack-gerrit) wrote : Fix merged to ironic (master)

Reviewed: https://review.openstack.org/84823
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=7ecca8868a63237485e1b0ca25278d526e1324d2
Submitter: Jenkins
Branch: master

commit 7ecca8868a63237485e1b0ca25278d526e1324d2
Author: Chris Behrens <email address hidden>
Date: Wed Apr 2 02:56:55 2014 -0700

    Add create() and destroy() to Node

    What it says. Also make the code use them vs direct calls to dbapi. This
    also removes passing context to node.refresh() and node.save() since
    context is cached on the Node object on create.

    Partial-Bug: 1314732

    Change-Id: Id3fafe1eae70954095ed94dec86e4583b23c5af8

Revision history for this message
Dmitry Tantsur (divius) wrote :

Hi Chris! What's the progress on this task? I see one patch was landed, do you have more? If yes, please link them here. Otherwise, please consider leaving a comment on what is left to do and unassigning yourself. Thank you!

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to ironic (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to ironic (master)

Reviewed: https://review.openstack.org/108959
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=e2e89126efa1ed910f269e2798e7b8b63adaf841
Submitter: Jenkins
Branch: master

commit e2e89126efa1ed910f269e2798e7b8b63adaf841
Author: Lucas Alvares Gomes <email address hidden>
Date: Wed Jul 23 12:14:21 2014 +0100

    Clean up calls to get_chassis()

    The same work have already been done by Nodes (see 2192f2848e), so right
    now the dbapi's are inconsistent across different objects. This patch
    fixes it for Chassis.

    This patch removes all direct calls to dbapi's get_chassis(). They
    are replaced either by Chassis.get(), Chassis.get_by_id(), or
    Chassis.get_by_uuid() calls.

    Additional detail about the changes:

    1) Break DBAPI get_chassis() into get_chassis_by_id() and get_chassis_by_uuid().
    Let's be explicit about which type we're passing. This allows
    Chassis.get_by_uuid() to work exactly how its named. Also, do not
    return an object from the DB API calls. The Chassis object itself will
    do the conversion.

    2) Adds Chassis.get_by_id() to compliment the current
    Chassis.get_by_uuid() for places you are querying by the ID.

    3) Adds Chassis.get() so that you can still have a single method to use
    to pass either and ID or a UUID.

    4) Make all of the Chassis.get* calls set the 'context' so that you
    can later just chassis.save() without passing it. Doc strings have been
    updated to clarify what 'context' is for.

    Co-Authored-By: Chris Behrens <email address hidden>
    Related-Bug: #1314732
    Change-Id: I5162c2843e15e538c1dc50dd5ac775298f8f0f9d

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/109252
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=b22507913ab741a46b67ccad14ecd35df14dad3e
Submitter: Jenkins
Branch: master

commit b22507913ab741a46b67ccad14ecd35df14dad3e
Author: Lucas Alvares Gomes <email address hidden>
Date: Thu Jul 24 11:56:47 2014 +0100

    Clean up calls to get_port()

    The same work have already been done by Nodes (see 2192f2848e), so right
    now the dbapi's are inconsistent across different objects. This patch
    fixes it for Port.

    This patch removes all direct calls to dbapi's get_port(). They are
    replaced either by Port.get(), Port.get_by_id(), Port.get_by_uuid()
    or Port.get_by_address() calls.

    Additional detail about the changes:

    1) Break DBAPI get_port() into get_port_by_id(), get_port_by_uuid(),
    and get_port_by_address(). Let's be explicit about which type we're
    passing. This allows Port.get_by_uuid() to work exactly how its
    named. Also, do not return an object from the DB API calls. The Chassis
    object itself will do the conversion.

    2) Adds Port.get_by_id() and Port.get_by_address to compliment the
    current Port.get_by_uuid() for places you are querying by the ID.

    3) Adds Port.get() so that you can still have a single method to use to
    pass either and ID, UUID or MAC address.

    4) Make all of the Port.get* calls set the 'context' so that you can
    later just port.save() without passing it. Doc strings have been updated
    to clarify what 'context' is for.

    Co-Authored-By: Chris Behrens <email address hidden>
    Related-Bug: #1314732
    Change-Id: I39d9a7acc12f0f3b7e06abd038537cd2759fb088

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to ironic (master)

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

Changed in ironic:
assignee: Chris Behrens (cbehrens) → Lucas Alvares Gomes (lucasagomes)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ironic (master)

Reviewed: https://review.openstack.org/112290
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=459919faa3b76c7763ce0d83de90f4931b3b91d5
Submitter: Jenkins
Branch: master

commit 459919faa3b76c7763ce0d83de90f4931b3b91d5
Author: Lucas Alvares Gomes <email address hidden>
Date: Wed Aug 6 11:52:54 2014 +0100

    Add create() and destroy() to Chassis object

    What it says. Also make the code and tests use them vs direct calls
    to dbapi.

    Partial-Bug: 1314732
    Change-Id: Iabd62baba09e2f8ea94ed3aab417b6a63ea14ec4

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/112323
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=7711b57029bf655eb152d021d128341516ecc7b5
Submitter: Jenkins
Branch: master

commit 7711b57029bf655eb152d021d128341516ecc7b5
Author: Lucas Alvares Gomes <email address hidden>
Date: Wed Aug 6 16:08:29 2014 +0100

    Add create() and destroy() to Port object

    What it says. Also make the code and tests use them vs direct calls
    to dbapi.

    Partial-Bug: 1314732
    Change-Id: Ic85b63a02722dd9657d1f2d2f1ad86fd8b78a061

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/112595
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=b1c7f00ca1834c82e50935fc5f5e076269208b51
Submitter: Jenkins
Branch: master

commit b1c7f00ca1834c82e50935fc5f5e076269208b51
Author: Lucas Alvares Gomes <email address hidden>
Date: Thu Aug 7 15:30:51 2014 +0100

    Remove direct calls to dbapi's get_node_by_instance

    Small patch to patch remove the direct calls to dbapi's
    get_node_by_instance. It's replaced by Node.get_by_instance_uuid().

    Partial-Bug: #1314732
    Change-Id: Idfcab25edbc950ce8093b4e0987be5a885828530

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to ironic (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ironic (master)

Reviewed: https://review.openstack.org/113877
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=19ea0327ddbe281e939f314b9444508db5d21e53
Submitter: Jenkins
Branch: master

commit 19ea0327ddbe281e939f314b9444508db5d21e53
Author: Lucas Alvares Gomes <email address hidden>
Date: Tue Aug 12 17:01:54 2014 +0100

    Add list() to Chassis, Node, Port objects

    What it says and remove the direct calls to dbapi and switch code to
    use the object interfaces.

    Partial-Bug: 1314732
    Change-Id: I250d7c82cc4d048b60e5f360aca26f593f77b04a

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/113968
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=488f5a19c3a2cc99706011d9751af296e88c87fe
Submitter: Jenkins
Branch: master

commit 488f5a19c3a2cc99706011d9751af296e88c87fe
Author: Lucas Alvares Gomes <email address hidden>
Date: Wed Aug 13 17:30:56 2014 +0100

    Remove objectify decorator from dbapi.update_* methods

    This patch removes the objectify decorator from the update_chassis,
    update_port and update_node dbapi's methods. The problem with the
    objectify decorator is that dbapi methods doesn't get the context,
    so when forming the RPC object using the decorator we can't store the
    context within the object (which is what we want to do to have consistent
    object interfaces).

    Also there are no parts of the code that is expecting the returned RPC
    object from the dbapi.update_*() methods so we can just simply get ride
    of the decorator.

    Partial-Bug: 1314732
    Change-Id: Ib5ac15100214b351607e1c17aa663fa4c123bc7a

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to ironic (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Changed in ironic:
assignee: Lucas Alvares Gomes (lucasagomes) → Angus Thomas (athomas-c)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ironic (master)

Reviewed: https://review.openstack.org/114595
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=74805aaec0c00176523d7452fa5b6fdbf3cc9cce
Submitter: Jenkins
Branch: master

commit 74805aaec0c00176523d7452fa5b6fdbf3cc9cce
Author: Angus Thomas <email address hidden>
Date: Fri Aug 15 17:33:58 2014 +0100

    Removes get_port_by_vif

    VIF is not indexed by the databse, it lives inside the extra attribute
    which is a data blob, so the method is not used.

    Change-Id: I11ce35bae1aa81c38c8ebc87251148ae65f3a30e
    Partial-bug: 1314732

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/114242
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=9a9752e50a38e741a849b36cb4e7c882d599774f
Submitter: Jenkins
Branch: master

commit 9a9752e50a38e741a849b36cb4e7c882d599774f
Author: Lucas Alvares Gomes <email address hidden>
Date: Thu Aug 14 13:53:22 2014 +0100

    Remove objectify decorator from dbapi's {get, register}_conductor()

    This patch removes the objectify decorator from the get_conductor and
    register_conductor dbapi's methods.

    For get_condutor() method there's an objects interface to it so this patch
    make the code/tests use the object interface vs direct calls to dbapi.

    The problem with the objectify decorator is that dbapi methods
    doesn't get the context, so when forming the RPC object using the
    decorator we can't store the context within the object (which is what
    we want to do to have consistent object interfaces).

    Partial-Bug: 1314732
    Change-Id: I1a955a59435b35277f3bbff2c02c07dd1a4c1d66

Changed in ironic:
assignee: Angus Thomas (athomas-c) → Lucas Alvares Gomes (lucasagomes)
Changed in ironic:
milestone: none → juno-rc1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to ironic (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ironic (master)

Reviewed: https://review.openstack.org/120422
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=2856ef26d2f5711c6ef4efec38d5839966748eb4
Submitter: Jenkins
Branch: master

commit 2856ef26d2f5711c6ef4efec38d5839966748eb4
Author: Lucas Alvares Gomes <email address hidden>
Date: Wed Sep 10 14:50:20 2014 +0100

    Add list_by_node_id() to Port object

    What it says. Also make the code and tests use them vs direct calls
    to dbapi.

    Change-Id: I5bfa6ec93be0d6f7460d5b1c45a057cc7ab3bc9b
    Partial-Bug: #1314732

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/120488
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=f575ae7ea3f690b81404a1b37caa84d1217b265b
Submitter: Jenkins
Branch: master

commit f575ae7ea3f690b81404a1b37caa84d1217b265b
Author: Lucas Alvares Gomes <email address hidden>
Date: Wed Sep 10 17:53:45 2014 +0100

    Add reserve() and release() to Node object

    What it says. Also make the code and tests use them vs direct calls
    to dbapi.

    Change-Id: I0158be53ff683ec3569007bc6f0cee6395241062
    Partial-Bug: #1314732

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/120489
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=38182e7222218bbe548452d9f7a6a76d6c1d54b3
Submitter: Jenkins
Branch: master

commit 38182e7222218bbe548452d9f7a6a76d6c1d54b3
Author: Lucas Alvares Gomes <email address hidden>
Date: Wed Sep 10 18:38:36 2014 +0100

    Remove the objectify decorator

    The objectify decorator was used to convert DB objects into RPC objects,
    but it's not being used anymore since out objects API already has
    correspondent methods to match the dbapi ones.

    Also, since dbapi methods didn't get a context the decorator was forming
    bad RPC objects without a context stored within it.

    Change-Id: I892ab6ee2236af040c7f8effcfdfecb293e13ab6
    Partial-Bug: #1314732

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on ironic (master)

Change abandoned by Lucas Alvares Gomes (<email address hidden>) on branch: master
Review: https://review.openstack.org/120773
Reason: superseded by https://review.openstack.org/121923

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Lucas Alvares Gomes (<email address hidden>) on branch: master
Review: https://review.openstack.org/120774
Reason: superseded by https://review.openstack.org/121923

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to ironic (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ironic (master)

Reviewed: https://review.openstack.org/121923
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=654ea01741182987124f66dbabcd222d85935ccd
Submitter: Jenkins
Branch: master

commit 654ea01741182987124f66dbabcd222d85935ccd
Author: Lucas Alvares Gomes <email address hidden>
Date: Tue Sep 16 10:39:20 2014 +0100

    Make context mandatory when instantiating a RPC object

    RPC objects should have a context within it, this patch is making passing
    the context mandatory when instantiating the object and is a plumbing
    work to remove passing the context later for things like create(),
    refresh(), destroy() and save().

    Partial-Bug: #1314732
    Change-Id: If9b175fa874bcb96c77cf22d176f1111f450f796

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/122401
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=4d7c3e828adff2e68fe6c434fdad9e37440d50b4
Submitter: Jenkins
Branch: master

commit 4d7c3e828adff2e68fe6c434fdad9e37440d50b4
Author: Lucas Alvares Gomes <email address hidden>
Date: Thu Sep 18 13:33:31 2014 +0100

    Do not set the context twice when forming RPC objects

    The context is already being set once the object is instantiated so
    there's no need to set it again. This patch fixes that and update tests
    to validate if the right context has been added to the object.

    Change-Id: I7eb3d734a990eb70e4fbfce6c539b268900b5241
    Partial-Bug: #1314732

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/122441
Committed: https://git.openstack.org/cgit/openstack/ironic/commit/?id=b55f6e514851f729a998e89e4df021151f7dfa44
Submitter: Jenkins
Branch: master

commit b55f6e514851f729a998e89e4df021151f7dfa44
Author: Lucas Alvares Gomes <email address hidden>
Date: Thu Sep 18 15:48:42 2014 +0100

    Do not use the context parameter on refresh()

    Now all RPC objects already have a context embedded within them so we
    can ignore the context parameter on the refresh() method (it was the
    last method using this parameter). This patch also:

    * Update the Docstrings from the @remotable methods to let the user know
      that the context parameter is not being used. The @remotable decorator
      still requires that the method it's decorating have a context as it's
      first argument, but that's going to change in the future.

    * All code and tests were updated to not pass the context to refresh(),
      save() and destroy() methods.

    Closes-Bug: #1314732
    Change-Id: Ibb338b909d99862ae048d605e66b8831d8c2128d

Changed in ironic:
status: In Progress → Fix Committed
aeva black (tenbrae)
Changed in ironic:
milestone: juno-rc1 → none
Thierry Carrez (ttx)
Changed in ironic:
milestone: none → juno-rc1
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in ironic:
milestone: juno-rc1 → 2014.2
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.