Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

salt: close rpm DB & yum repo/cache #1971

Merged
merged 1 commit into from
Oct 31, 2019
Merged

salt: close rpm DB & yum repo/cache #1971

merged 1 commit into from
Oct 31, 2019

Conversation

alexandre-allard
Copy link
Contributor

@alexandre-allard alexandre-allard commented Oct 30, 2019 •
edited by gdemonet
Loading

We forgot to close databases after checking the presence
of packages in repositories, it can lead to DB related errors
when trying to use yum just after calling the salt custom module

Refs: #1970

Component: salt

Context: RPM & yum DB not closed

Acceptance criteria: successful ./doit.sh vagrant_up

See: #1970

@alexandre-allard alexandre-allard requested a review from a team as a code owner October 30, 2019 13:42
@bert-e
Copy link
Contributor

bert-e commented Oct 30, 2019

Hello alexandre-allard-scality,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Oct 30, 2019

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

NicolasT
NicolasT reviewed Oct 30, 2019
View reviewed changes
salt/_modules/metalk8s_package_manager_yum.py Outdated
@@ -198,6 +198,8 @@ def check_pkg_availability(pkgs_info):
try:
ybase.install(name=name, version=version, release=release)
except yum.Errors.InstallError as exc:
ybase.closeRpmDB()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not too fond of doing it this way, because there's now one close path in an except and a later finally. There are better ways to do error handling with resource releasing.

Copy link
Contributor

Choose a reason for hiding this comment

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

implementing a python context and using with will close the object every time I guess

Copy link
Collaborator

@TeddyAndrieux TeddyAndrieux Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

As this except raise right after this do not really matter but indeed I agree it's cleaner to do one big try - finally.

Exceot if something else is raised ... true

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, you can wrap the whole for loop in try/finally

Copy link
Contributor

Choose a reason for hiding this comment

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

context manager are the way to handle this kind of things, they are the Python way of implementing RAII-like behavior.

gdemonet
gdemonet suggested changes Oct 30, 2019
View reviewed changes
salt/_modules/metalk8s_package_manager_yum.py Outdated
@@ -198,6 +198,8 @@ def check_pkg_availability(pkgs_info):
try:
ybase.install(name=name, version=version, release=release)
except yum.Errors.InstallError as exc:
ybase.closeRpmDB()
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, you can wrap the whole for loop in try/finally

@bert-e
Copy link
Contributor

bert-e commented Oct 30, 2019

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@gdemonet
Copy link
Contributor

Just tested on my machine, it fixed the issue described in #1970 for me

@alexandre-allard
salt: close rpm DB & yum repo/cache
2112ae0 
We forgot to close databases after checking the presence
of packages in repositories, it can lead to DB related errors
when trying to use yum just after calling the salt custom module

Refs: #1970
gdemonet
gdemonet approved these changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@gdemonet gdemonet left a comment

Choose a reason for hiding this comment

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

Thanks! This PR would be a good candidate for a changelog entry IMO, so let's not close the issue yet

@bert-e
Copy link
Contributor

bert-e commented Oct 30, 2019

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

@NicolasT
Copy link
Contributor

Consider adding a GNU-style CHANGELOG file in the root of the project for now in which we can track things, then later on migrate those into whichever change tracking system we put in place.

@alexandre-allard
Copy link
Contributor Author

/approve

@bert-e
Copy link
Contributor

bert-e commented Oct 31, 2019

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/2.4

The following branches will NOT be impacted:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@alexandre-allard
Copy link
Contributor Author

Creating another PR to add CHANGELOG, so we can have the fix for now.

@bert-e
Copy link
Contributor

bert-e commented Oct 31, 2019

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/2.4

The following branches have NOT changed:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3

Please check the status of the associated issue None.

Goodbye alexandre-allard-scality.

@bert-e bert-e merged commit 2112ae0 into development/2.4 Oct 31, 2019
@bert-e bert-e deleted the bugfix/1970-close-rpm-db branch October 31, 2019 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasT NicolasT NicolasT left review comments

@slaperche-scality slaperche-scality slaperche-scality left review comments

@sayf-eddine-scality sayf-eddine-scality sayf-eddine-scality left review comments

@TeddyAndrieux TeddyAndrieux TeddyAndrieux left review comments

@gdemonet gdemonet gdemonet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alexandre-allard @bert-e @gdemonet @NicolasT @slaperche-scality @sayf-eddine-scality @TeddyAndrieux