diff --git a/CHANGES/1571.bugfix b/CHANGES/1571.bugfix new file mode 100644 index 000000000..5277ad326 --- /dev/null +++ b/CHANGES/1571.bugfix @@ -0,0 +1 @@ +Prevented a race condition that lead to contraint violations when calculating the highest version of a collection. diff --git a/pulp_ansible/app/migrations/0055_alter_collectionversion_version_alter_role_version.py b/pulp_ansible/app/migrations/0055_alter_collectionversion_version_alter_role_version.py new file mode 100644 index 000000000..37553566a --- /dev/null +++ b/pulp_ansible/app/migrations/0055_alter_collectionversion_version_alter_role_version.py @@ -0,0 +1,27 @@ +# Generated by Django 4.2.4 on 2023-09-15 09:23 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("ansible", "0054_split_collection_version_numbers"), + ] + + operations = [ + migrations.RunSQL( + # Custom roles are only allowed with postgreSQL 16. + sql="CREATE COLLATION pulp_ansible_semver (provider = 'icu', locale = 'en-US-u-kn-true')", + reverse_sql="DROP COLLATION pulp_ansible_semver", + ), + migrations.AlterField( + model_name="collectionversion", + name="version", + field=models.CharField(db_collation="pulp_ansible_semver", max_length=128), + ), + migrations.AlterField( + model_name="role", + name="version", + field=models.CharField(db_collation="pulp_ansible_semver", max_length=128), + ), + ] diff --git a/pulp_ansible/app/models.py b/pulp_ansible/app/models.py index 62d961e52..c7e8a4711 100644 --- a/pulp_ansible/app/models.py +++ b/pulp_ansible/app/models.py @@ -48,7 +48,7 @@ class Role(Content): namespace = models.CharField(max_length=64) name = models.CharField(max_length=64) - version = models.CharField(max_length=128) + version = models.CharField(max_length=128, db_collation="pulp_ansible_semver") @property def relative_path(self): @@ -179,7 +179,7 @@ class CollectionVersion(Content): repository = models.CharField(default="", blank=True, max_length=2000, editable=False) requires_ansible = models.CharField(null=True, max_length=255) - version = models.CharField(max_length=128, editable=False) + version = models.CharField(max_length=128, db_collation="pulp_ansible_semver") version_major = models.IntegerField() version_minor = models.IntegerField() version_patch = models.IntegerField() diff --git a/pulp_ansible/app/tasks/collections.py b/pulp_ansible/app/tasks/collections.py index 3dc1fd8bb..8fa173043 100644 --- a/pulp_ansible/app/tasks/collections.py +++ b/pulp_ansible/app/tasks/collections.py @@ -15,7 +15,7 @@ from async_lru import alru_cache from django.conf import settings from django.db import transaction -from django.db.models import Q +from django.db.models import F, Q from django.db.utils import IntegrityError from django.urls import reverse from django.utils.dateparse import parse_datetime @@ -344,9 +344,8 @@ def create_collection_from_importer(importer_result, metadata_only=False): tag, created = Tag.objects.get_or_create(name=name) collection_version.tags.add(tag) - _update_highest_version(collection_version) - collection_version.save() # Save the FK updates + _update_highest_version(collection_version) return collection_version @@ -421,60 +420,18 @@ def _get_backend_storage_url(artifact_file): return url -def _update_highest_version(collection_version, save=False): +def _update_highest_version(collection_version): """ Checks if this version is greater than the most highest one. - If this version is the first version in collection, is_highest is set to True. - If this version is greater than the highest version in collection, set is_highest - equals False on the last highest version and True on this version. - Otherwise does nothing. The collection version is updated to the database if `save=True`, - otherwise only the `is_highest` field is updated on the instance. This is an optimization - to prevent double saving when this method is called during syncs and uploads. + TODO: This function violates content immutability. It must be deprecated. """ - def is_new_highest(new, old): - if bool(new.prerelease) == bool(old.prerelease): - return new > old - return bool(old.prerelease) - - # did we have one set previously for this collection? - last_highest = collection_version.collection.versions.filter(is_highest=True).first() - - if not last_highest: - # we only have one version, so mark it as the highest - if collection_version.collection.versions.count() == 1: - collection_version.is_highest = True - if save: - collection_version.save(update_fields=["is_highest"]) - return - - # previous highest must have been removed, re-compute highest from the whole list ... - highest = None - for cv in collection_version.collection.versions.all(): - sv = Version(cv.version) - if highest is None or is_new_highest(sv, highest[0]): - highest = (sv, cv) - - highest[1].is_highest = True - if highest[1] != collection_version or save: - highest[1].save() - return - - # exit if the new CV is not higher - if not is_new_highest(Version(collection_version.version), Version(last_highest.version)): - # ensure that this collection_version doesn't have is_highest improperly set - if collection_version.is_highest: - collection_version.is_highest = False - if save: - collection_version.save(update_fields=["is_highest"]) - return - - last_highest.is_highest = False - last_highest.save(update_fields=["is_highest"]) - collection_version.is_highest = True - if save: - collection_version.save(update_fields=["is_highest"]) + qs = collection_version.collection.versions.annotate(prerelease=Q(version_prerelease__ne="")) + highest_subq = qs.order_by("prerelease", "-version")[0:1].values("pk") + # Order them in such a way, that only the latest updated record will recieve true. + # This should prevent hitting the uniquenes constraint. + qs.annotate(new_is_highest=Q(pk=highest_subq)).order_by("-prerelease", "version").update(is_highest=F("new_is_highest")) class AnsibleDeclarativeVersion(DeclarativeVersion): @@ -1244,6 +1201,5 @@ def _post_save(self, batch): continue setattr(collection_version, attr_name, attr_value) - _update_highest_version(collection_version) - collection_version.save() + _update_highest_version(collection_version) diff --git a/pulp_ansible/tests/unit/test_tasks.py b/pulp_ansible/tests/unit/test_tasks.py index 6b3f68528..7a9d608f1 100644 --- a/pulp_ansible/tests/unit/test_tasks.py +++ b/pulp_ansible/tests/unit/test_tasks.py @@ -114,7 +114,7 @@ def test_is_highest_set(self): collection_versions = build_cvs_from_specs(specs, build_artifacts=False) for cv in collection_versions: - _update_highest_version(cv, save=True) + _update_highest_version(cv) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1") @@ -133,7 +133,7 @@ def test_is_highest_set_on_multple_stable_releases(self): collection_versions = build_cvs_from_specs(specs, build_artifacts=False) for cv in collection_versions: - _update_highest_version(cv, save=True) + _update_highest_version(cv) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1") @@ -158,7 +158,7 @@ def test_is_highest_set_on_single_prerelease(self): collection_versions = build_cvs_from_specs(specs, build_artifacts=False) for cv in collection_versions: - _update_highest_version(cv, save=True) + _update_highest_version(cv) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc2") @@ -178,7 +178,7 @@ def test_is_highest_set_on_multiple_prereleases(self): collection_versions = build_cvs_from_specs(specs, build_artifacts=False) for cv in collection_versions: - _update_highest_version(cv, save=True) + _update_highest_version(cv) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc1") @@ -211,7 +211,7 @@ def test_is_highest_set_on_multiple_prereleases_one_pass(self): ] collection_versions = build_cvs_from_specs(specs, build_artifacts=False) - _update_highest_version(collection_versions[1], save=True) + _update_highest_version(collection_versions[1]) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc1") @@ -246,7 +246,7 @@ def test_is_highest_set_on_multiple_prereleases_backwards_order(self): collection_versions = build_cvs_from_specs(specs, build_artifacts=False) for cv in collection_versions[::-1]: - _update_highest_version(cv, save=True) + _update_highest_version(cv) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1-rc1") @@ -286,7 +286,7 @@ def test_prerelease_not_higher_than_stable(self): collection_versions = build_cvs_from_specs(specs, build_artifacts=False) for cv in collection_versions: - _update_highest_version(cv, save=True) + _update_highest_version(cv) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="1.0.1") @@ -312,7 +312,7 @@ def test_prerelease_matches_stable(self): collection_versions = build_cvs_from_specs(specs, build_artifacts=False) for cv in collection_versions: - _update_highest_version(cv, save=True) + _update_highest_version(cv) assert ( CollectionVersion.objects.filter(namespace=namespace, name=name, version="2.0.0-rc1")