(fix) database is locked during remove #828

gallettilance · 2021-11-16T14:34:27Z

Signed-off-by: Lance Galletti galletti@bu.edu

Description of the change:

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci · 2021-11-16T14:34:31Z

@gallettilance: This pull request references Bugzilla bug 2023522, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2023522: (fix) database is locked during remove

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gallettilance · 2021-11-16T14:35:17Z

/bugzilla refresh

openshift-ci · 2021-11-16T14:35:24Z

@gallettilance: This pull request references Bugzilla bug 2023522, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-11-16T14:35:25Z

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: jianzhangbjz.

Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@gallettilance: This pull request references Bugzilla bug 2023522, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)

bug target release (4.10.0) matches configured target release for branch (4.10.0)

bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gallettilance · 2021-11-16T14:37:22Z

pkg/sqlite/load.go


 	packagePropertiesQuery := `select distinct operatorbundle_name, value from properties where type = ?`
-	rows, err = s.db.Query(packagePropertiesQuery, registry.PackageType)
+	pRows, err := tx.Query(packagePropertiesQuery, registry.PackageType)


Note for reviewers: this is technically the root cause for this particular bug. Everything else seemed like good changes to make at the same time to prevent future such errors.

codecov · 2021-11-16T14:40:03Z

Codecov Report

Merging #828 (6658e6b) into master (7a789fe) will increase coverage by 0.03%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #828      +/-   ##
==========================================
+ Coverage   51.46%   51.50%   +0.03%     
==========================================
  Files         103      103              
  Lines        9032     9062      +30     
==========================================
+ Hits         4648     4667      +19     
- Misses       3510     3516       +6     
- Partials      874      879       +5

Impacted Files	Coverage Δ
pkg/sqlite/load.go	`43.41% <77.77%> (+0.16%)`	⬆️
pkg/registry/query.go	`60.41% <0.00%> (-0.53%)`	⬇️
pkg/registry/populator.go	`63.15% <0.00%> (-0.03%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a789fe...6658e6b. Read the comment docs.

anik120

/lgtm

Worked for me locally.

Ideally we'd like to have some test cases for rmStrandedBundles. I'm guessing that could have helped catch concurrent db access. This change is simple enough though and makes sense to me by itself.

openshift-ci · 2021-11-17T17:52:06Z

@gallettilance: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

(fix) database is locked during remove

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

anik120 · 2021-11-17T17:53:02Z

@gallettilance also, we're not using bugzilla numbers in our upstream projects anymore.

Having proper commit messages and "description of the change" is the default way of providing all the context that's needed to review this PR.

This change ensures that connections to the database are properly closed and that no new connection is started when another has not been closed. Signed-off-by: Lance Galletti <galletti@bu.edu>

ankitathomas

/lgtm

dinhxuanvu

/approve

openshift-ci · 2021-11-17T22:31:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, ankitathomas, dinhxuanvu, gallettilance

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dinhxuanvu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Nov 16, 2021

openshift-ci bot requested review from dinhxuanvu and joelanford November 16, 2021 14:34

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Nov 16, 2021

gallettilance commented Nov 16, 2021

View reviewed changes

anik120 approved these changes Nov 17, 2021

View reviewed changes

openshift-ci bot assigned anik120 Nov 17, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

anik120 changed the title ~~Bug 2023522: (fix) database is locked during remove~~ (fix) database is locked during remove Nov 17, 2021

openshift-ci bot removed bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Nov 17, 2021

(fix) database is locked during removal

6658e6b

This change ensures that connections to the database are properly closed and that no new connection is started when another has not been closed. Signed-off-by: Lance Galletti <galletti@bu.edu>

gallettilance force-pushed the db-locked branch from 67d321e to 6658e6b Compare November 17, 2021 20:56

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

ankitathomas approved these changes Nov 17, 2021

View reviewed changes

openshift-ci bot assigned ankitathomas Nov 17, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

dinhxuanvu approved these changes Nov 17, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2021

openshift-merge-robot merged commit 28aa654 into operator-framework:master Nov 17, 2021

(fix) database is locked during remove #828

(fix) database is locked during remove #828

Uh oh!

Conversation

gallettilance commented Nov 16, 2021

Uh oh!

openshift-ci bot commented Nov 16, 2021

Uh oh!

gallettilance commented Nov 16, 2021

Uh oh!

openshift-ci bot commented Nov 16, 2021

Uh oh!

openshift-ci bot commented Nov 16, 2021

Uh oh!

gallettilance Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anik120 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 17, 2021

Uh oh!

anik120 commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankitathomas left a comment

Choose a reason for hiding this comment

Uh oh!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Nov 16, 2021 •

edited

Loading

anik120 left a comment •

edited

Loading

anik120 commented Nov 17, 2021 •

edited

Loading