ntt-sic / masakari

[UNMAINTAINED]
https://wiki.openstack.org/wiki/Masakari
Apache License 2.0
41 stars 25 forks source link

Feature/sqlalchemy #22

Closed sampathP closed 8 years ago

sampathP commented 8 years ago

sqlalchemy implementation for masakari.

Error handling done through sqlalchemy lib and masakari only catch exceptions. add decorators for session rollback, commit, deadlock handling.

sampathP commented 8 years ago

Conflicts are resolved. Merge button does not show up... :imp: But still can merge via CLI .. :bulb:

muroi commented 8 years ago

The PR looks good to me. If possible, could you check my inline comment?

sampathP commented 8 years ago

@muroi Thank you for review. Please find the fix in previous patch

muroi commented 8 years ago

Thank you for quick updating! I want to merge it, but I hit some following issues when I deploy this with masakari-deploy. Do you have any work for these?

  1. masakari-controller/db directory is not put under /opt/masakari/ by deb installer
  2. I was required to additionally install sqlalchemy_utils ($ sudo pip install sqlalchemy_utils) even after I installed openstack

Additionally, I hit unexpected error during testing. Sorry, I should've tested it and comment it first.

  1. in recovering instance failure, I received the log in masakari-controller.log and instance is not rescued. <_MainThread(MainThread, started 140390764574528)> must be string, not None at File "/opt/masakari/controller/masakari_util.py", line 205, in insert_notification_list_db
  2. in recovering host failure, I received the log in masakari-controller.log and instance is not rescued. <Thread(Thread-4, started 139735165568768)> 'unicode' object has no attribute 'rollback' at File "/opt/masakari/controller/masakari_starter.py", line 163, in _create_vm_list_db_for_failed_host( notification_uuid)
sampathP commented 8 years ago

Sorry for the inconvenience.

muroi commented 8 years ago

Thank you @sampathP . I hit same bug you resolved by last commit. Eventually, the PR works well. So I merged it manually since there is a merge conflict.

Close #5