API exposes SQL queries to the client

[Max Lobur]

Steps to reproduce - creating node with duplicate uuid:
1. Create the first node specifying some uuid
curl -i -X POST -H 'X-Auth-Token: <your token>'Content-Type: application/json' -H 'Accept: application/json' -H -d '{"driver": "fake", "uuid": "2359bca1-3ca1-4913-92c2-4d40438e01e5"}' http://192.168.122.224:6385/v1/nodesHTTP/1.0

2. Create the second node specifying the same uuid. Response will be
{"error_message": "{\"debuginfo\": null, \"faultcode\": \"Server\", \"faultstring\": \"(IntegrityError) (1062, \\\"Duplicate entry '2359bca1-3ca1-4913-92c2-4d40438e01e5' for key 'node_uuid_ux'\\\") 'INSERT INTO nodes (created_at, updated_at, uuid, instance_uuid, chassis_id, power_state, target_power_state, provision_state, target_provision_state, last_error, properties, driver, driver_info, reservation, maintenance, extra) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s)' (datetime.datetime(2014, 2, 7, 15, 14, 12, 730006), None, '2359bca1-3ca1-4913-92c2-4d40438e01e5', None, None, None, None, None, None, None, '\\\"{}\\\"', 'fake', '\\\"{}\\\"', None, 0, '\\\"{}\\\"')\"}"}

Response code 500, which should be 409

The hook intended to cut exception traceback cannot handle this.
https://github.com/openstack/ironic/blob/master/ironic/api/hooks.py#L139-L141
Because fault string in this case is not a traceback and does not contain python traceback artifacts.

We need either convert all such DB exceptions to IronicException, or create one more hook. The first one is more preferable to me.

[Josh Gachnang (joshnang)]

So part of the issue is that the exception being raised by node.save() is a generic ironic.openstack.common.db.exception.DBError, rather than DBDuplicateEntry.

The other part is that while we have debug on, we probably still want the error messages, right?

I think the solution is going to be wrapping all the .save() methods in try/excepts in sqlalchemy/api (where this is happening), and raising an IronicException with a message based on whether debug is on or not. Some of the .save() calls catch some instances, but I think it should be catching the broader DBError to avoid leaking details. For example, if a DBConnectionError pops up in a call with only DuplicateError caught, it may leak some details as well.

[OpenStack Infra (hudson-openstack)]

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

[Josh Gachnang (joshnang)]

This still needs some additional tests for the new exceptions, and I'd like to figure out why DBError is being raised instead of DuplicateEntry.

[OpenStack Infra (hudson-openstack)]

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

[aeva black (tenbrae)]

No new revisions in the last two weeks, and both proposed patch sets have been abandoned. I've confirmed the bug is still present, so I am unassigning and setting status back to Triaged.

Josh, if you're going to continue working on it, please let me know and re-open the relevant reviews.

[Dmitry Tantsur (divius)]

Hi Josh,

You've been assigned for more than a month for now, could you give a status update on this bug? Are you still working on https://review.openstack.org/#/c/73121/ ?

[Josh Gachnang (joshnang)]

Last time I committed, I had it working (IMO) to what Deva had requested, but it adds and replaces some exceptions, which breaks Tempest. I haven't had much time to learn how Tempest works and fix those tests (and now it probably is broken with merged changes). I'm happy to let someone else fix it, as I'll be focused on making the agent driver rock solid for the foreseeable future.

[Dmitry Tantsur (divius)]

Thanks for update, Josh! I set status to triaged, so that people know they can jump in.

[Viktor Serhieiev (vsergeyev)]

Hello All.

I suppose ,that database structure in Ironic is not commercial information, so we actually can provide SQL query with debug information. Also seems to be, that current solution (I mean patch https://review.openstack.org/#/c/73121/ ) remove all additional debug info (like traceback and SQL query) so we will be not in able to get it easily with debug=True.

Also I can see in patch, that we set code 409 only for DBDuplicateEntry exception. So maybe it'swill be better, if we will catch this exception in db-api methods and re-raise it more gracefully - like NodeAlreadyExist for situation in bug description.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

commit db509e1fda804986ac87c78a344531fcf2ceab87
Author: Dmitry Tantsur <email address hidden>
Date: Wed Jun 25 14:03:31 2014 +0200

    Raise appropriate errors on duplicate Node, Port and Chassis creation

    This patch introduces NodeAlreadyExists, ChassisAlreadyExists,
    PortAlreadyExists and InstanceAssociated exceptions for
    appropriate cases.

    Partial-Bug: #1277555

    Change-Id: Iec2a7cd09022b6a12afacec9b9b51f8197817ded

[Dmitry Tantsur (divius)]

This needs reinvestigation, as we migrated to olso.db

[aeva black (tenbrae)]

I can not reproduce this, and believe the aforementioned patches and/or the oslo.db migration have already addressed it. As such, I'm closing this bug. Please re-open if necessary.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Josh Gachnang (<email address hidden>) on branch: master
Review: https://review.openstack.org/73121
Reason: Bug fixed in oslo.db.