Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Fix an issue in db connect function #1211

Merged
merged 2 commits into from May 3, 2017
Merged

Conversation

shaominchen
Copy link
Contributor

@shaominchen shaominchen commented May 2, 2017

sqlite3.connect() function will throw an exception instead of returning none if running into any errors. So we should try/catch the error instead of checking for none result.

Fix #1144

Test result:

  1. Set up MultiNode on 2 ESXs
  2. Open the DB on ESX1
  3. Run "vmdkops_admin vmgroup ls" on ESX2

Error from step 3:
[root@sc-rdops-vm09-dhcp-28-64:/var/log/vmware] vmdkops_admin vmgroup ls
Internal Error(DB connection error at /etc/vmware/vmdkops/auth-db)

Error from vmdk_ops.log:
05/03/17 00:15:41 91741 [MainThread] [ERROR ] Failed to connect to DB (/etc/vmware/vmdkops/auth-db): unable to open database file

@shaominchen
Copy link
Contributor Author

Note: I'm still waiting for my new testbed to be ready to verify the fix.

@msterin
Copy link
Contributor

msterin commented May 2, 2017

LGTM pending usual stuff:
(1) test: need a cut-n-paste example of triggering the exception (2) need CI pass when CI is back

Copy link
Contributor

@msterin msterin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (contingent)

@shaominchen shaominchen self-assigned this May 2, 2017
@shuklanirdesh82
Copy link
Contributor

@shaominchen Please rebase your branch with master to get .drone.sec changes.


try:
self.conn = sqlite3.connect(self.db_path)
except sqlite3.Error as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can log the content of object e as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will log the error.

pshahzeb
pshahzeb previously approved these changes May 2, 2017
Copy link
Contributor

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (CI pass)
Log the content of exception (only if it is helpful)

Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed changes looks good to me.

Can you please paste the error message (by executing the step mentioned the issue #1144) for the reference?

Copy link
Contributor

@shuklanirdesh82 shuklanirdesh82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for pasting the sample output.

Copy link
Contributor

@pshahzeb pshahzeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pshahzeb
Copy link
Contributor

pshahzeb commented May 3, 2017

Not this PR; but I feel all the errors reported to user/admin shouldn't be marked as "internal error". They should be clear as to what error or type of error is.

@shuklanirdesh82 shuklanirdesh82 merged commit bd18fe3 into master May 3, 2017
@shuklanirdesh82 shuklanirdesh82 deleted the issue_1144.shchen branch May 3, 2017 07:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants