PR#3721: use table lock while adding new external rpms - koji

koji

#3721 use table lock while adding new external rpms

Closed 2 years ago by tkopecek. Opened 2 years ago by tkopecek.

tkopecek/koji issue3637 into master

Sort kiwi package output before import

Tomas Kopecek • 2 years ago

453e3de

Revert "use table lock while adding new external rpms"

Tomas Kopecek • 2 years ago

67cfe5b

use table lock while adding new external rpms

Tomas Kopecek • 2 years ago

544b968

plugins/builder/kiwi.py

file modified

+6 -2

		`@@ -275,7 +275,9 @@`
		`found = True`
		`if not found:`
		`raise koji.LiveCDError('No repos found in yum cache!')`
		`- return list(hdrlist.values())`
		`+ # Sort on return deterministically (key is ~nevra) to not cause race conditions on import`
		`+ return sorted(hdrlist.values(),`
		`+ key=lambda x: {x['name'], x['epoch'], x['version'], x['release'], x['arch']})`

		`def getImagePackages(self, result):`
		`"""Proper handler for getting rpminfo from result list,`
		`@@ -299,7 +301,9 @@`
		`'buildtime': 0,`
		`})`

		`- return hdrlist`
		`+ # Sort on return deterministically (key is ~nevra) to not cause race conditions on import`
		`+ return sorted(hdrlist,`
		`+ key=lambda x: {x['name'], x['epoch'], x['version'], x['release'], x['arch']})`

		`def handler(self, name, version, release, arch,`
		`target_info, build_tag, repo_info,`

tkopecek commented 2 years ago

PR #794 introduced savepoint-based solution for parallel addition of
external rpms. In some usecases there could be a lot of external rpms
being added, so potential for deadlock is much higher than before.
Locking the table seems to be a bit expensive but SHARE ROW EXCLUSIVE
should be a working compromise (anyone not writing to the table has
still read access).

rebased onto 9fca1283994c948b28f14884056e802b58a58048

2 years ago

mikem commented 2 years ago

A table lock seems like a very big hammer here. We don't actually do that anywhere else.

Do you have a reproducer? Some example hub traces?

I wonder if this would be helped by sorting rpm lists before passing them to add_external_rpm.

tkopecek commented 2 years ago

I wasn't able to reproduce it reliably. Total reproducer is your script, but don't have any real-world examples. @ignatenkobrain anything in your logs?

You're right that kiwi is doing the rpm retrieval in different way than the other task (getImagePackagesFromCache). I can add sorting there, but don't have reliable way to test that it really helped.

Edited 2 years ago by tkopecek

mikem commented 2 years ago

We previously had very similar deadlocks when updating buildroots. These were solved by sorting the inputs. I'd prefer to try the same here before we add locking.

If we do add locking, I'd rather not have the full table lock. A better option might be the upcoming lock functionality in #3786, but again, lets try the sorting first.

Absent a reproducer, we can't verify that any fix will work except by getting the reporter to test it out.

mikem commented 2 years ago

I've also added some questions in the original issue. It would be nice to have precise details about before we go making complex changes.

rebased onto 544b968

2 years ago

3 new commits added

Sort kiwi package output before import
Revert "use table lock while adding new external rpms"
use table lock while adding new external rpms

2 years ago

tkopecek commented 2 years ago

I've replaced it with sorting on input for now.

mikem commented 2 years ago

The current solution is so different that it might deserve a separate PR. Or at least fix the title of this one and squash out the reverted commit.

Sorting these inputs in the builder plugin seems at least harmless, but it would probably be simpler to just add

rpm_ids.sort()

in importImageInternal before the bulk inserts into archive_rpm_components. This would be directly analogous to the measure we have in BuildRoot._setList.

mikem commented 2 years ago

filed #3882

Pull-Request has been closed by tkopecek

2 years ago

tkopecek commented 2 years ago

closed in favour of #3882