Skip to content

(PUP-1775) Acquire and block for Yum's lock to prevent corruption#2473

Closed
domcleal wants to merge 2 commits intopuppetlabs:masterfrom
domcleal:tickets/master/pup-1775-yum-lock
Closed

(PUP-1775) Acquire and block for Yum's lock to prevent corruption#2473
domcleal wants to merge 2 commits intopuppetlabs:masterfrom
domcleal:tickets/master/pup-1775-yum-lock

Conversation

@domcleal
Copy link
Contributor

Yum tools always attempt to acquire a global lock to prevent database
corruption by blocking. This change adds lock acquisition to the yumhelper
which is called by the yum package provider, looping until either a timeout
is reached or the lock acquisition was successful.

Replaces #2387

Yum tools always attempt to acquire a global lock to prevent database
corruption by blocking.  This change adds lock acquisition to the yumhelper
which is called by the yum package provider, looping until either a timeout
is reached or the lock acquisition was successful.
@kylog
Copy link

kylog commented Mar 26, 2014

Should we rip out shell_out entirely while we're in yumhelper.py? The comment says it's for RHEL3, and can be ripped out once that is EOL, and meanwhile your quick test on CentOS 6.4 suggests that it is in fact broken if used there.

@puppetcla
Copy link

CLA signed by all contributors.

@domcleal
Copy link
Contributor Author

@kylog interesting idea, it'd certainly reduce risk of yumhelper going wrong. RHEL3 has been EOL for some time, so I think this would be fine.

The other thing I was thinking of was to start logging bugs against it, but I wouldn't be in a particular hurry to fix it.

The yumhelper had two paths of code - either it could load the yum
library and would use this to enumerate the updates, or it would call
`yum check-update` and parse the output.

The latter was typically used on very old versions of Yum (i.e. RHEL3
era) which have long been EOL.  To simplify this code, the shell
fallback method has been removed.  Failures to load the library will
result in the script raising an error.
@domcleal
Copy link
Contributor Author

domcleal commented Apr 7, 2014

@kylog sorry for the delay. I've updated the PR to remove the legacy code.

@adrienthebo
Copy link
Contributor

@domcleal to check, you've been able to validate this code for yourself that this works as expected?

@domcleal
Copy link
Contributor Author

@adrienthebo yes, I tested using a "sleeper" RPM (which I attached to PUP-1775) to verify that when the sleeper RPM is being installed and you run Puppet in parallel that the prefetching through yumhelper pauses until the lock is fetched. I've only tested on RHEL 6 x86_64.

@adrienthebo
Copy link
Contributor

Awesome, thanks for verifying this! With luck we'll have this merged shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is far longer than we should wait while trying to install a package, I think this should be closer to 10 seconds. If we can't retrieve this information then we should fail hard.

@adrienthebo
Copy link
Contributor

@domcleal the more I look at this, the more nervous I get about using the yum python bindings. I checked the version of yum back to CentOS 4 and yum check-update hasn't changed in format since this version; I would be interested in using that to avoid any issues with locking the rpm DB. I seem to recall that you wanted to avoid using yum check-update because of possible issues with output changes; does that concern still stand?

Copy link
Contributor

Choose a reason for hiding this comment

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

@domcleal do we want to unlock first, then close (in reverse order that those were acquired)?

Copy link

Choose a reason for hiding this comment

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

No, that would be wrong. This patch acquires the yum lock, the rpm db is then implicitely opened later in the middle of the yum guts. You need to close tht db first, before unlocking yum.

@domcleal
Copy link
Contributor Author

@adrienthebo My concern was only that this was the original implementation, and that it was changed to the python helper by @lutter in response to a change in behaviour. As you said, it hasn't changed again recently so probably would be OK to switch back. I'm not likely to have time to implement this though, sorry.

The ancient bug http://projects.puppetlabs.com/issues/836 has some information.

It looks like the provider switched to prefetching in the same commit (8722e43), so the original issue about calling "yum list available" for a package without an update available is actually moot (see comment in http://projects.puppetlabs.com/attachments/175/yum.patch).

@kylog kylog mentioned this pull request May 13, 2014
@adrienthebo
Copy link
Contributor

This is superseded by GH-2670 which removes yumhelper.py in favor of parsing the output of yum check-update; I'm closing this issue in favor of that pull request. Thanks again for your work on this pull request and sorry about the duplicate work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants