Repository.latest_version() considering deleted versions #555

fao89 · 2020-02-19T19:27:06Z

https://pulp.plan.io/issues/6147
closes #6147

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

dralley · 2020-02-19T19:51:41Z

pulpcore/app/models/repository.py

@@ -740,6 +740,9 @@ def delete(self, **kwargs):
                # and delete the version
                repo_relations.filter(version_added=self).delete()
                repo_relations.filter(version_removed=self).update(version_removed=None)
+                if self.number == self.repository.last_version:
+                    self.repository.last_version = self.number - 1
+                    self.repository.save()


I haven't tested this out, but could you check this scenario? What happens if you create 3 new repository versions, so you have:

0, 1, 2, 3
repository.last_version = 3

If you delete 3 and then 2 you would probably get the correct result:

0, 1,
repository.last_version = 1

But if you delete 2 and then 3, then I think with this algorithm you would get:

0, 1
repository.last_version = 2

I could be wrong, it's just an edge case we should check :)

I will write a test for it

Probably just 2 versions is enough to test this, version 1 isn't really doing much as-is.

I guess this also brings a question to mind -- do we want repository version numbers to be re-usable? Because if we reset last_version appropriately, and we also set number of the next version as last_version + 1 , then that means that it will be possible to overwrite the value of ${REPO_HREF}/versions/2/ with a new version 2 that contains different content. The version number won't be monotonically increasing.

Solution from IRC:

Rename Repository.last_version to Repository.next_version and do a data migration to add +1 to its value

When creating a new repository version, use the value of Repository.next_version and if it is successfully created increment this value.

To determine the latest repository version, do this (and see if we can get rid of that custom field)

This sounds right to me. Does this change the REST API ?

@dkliban it shouldn't. We don't expose that field.

It shouldn't, no. last_version isn't serialized and next_version doesn't need to be either.

Great! thanks for confirming that.

dralley · 2020-02-20T14:25:30Z

pulpcore/app/migrations/0022_rename_last_version.py

+        Repository = apps.get_model('core', 'Repository')
+        if 'last_version' in dir(Repository):
+            for repository in Repository.objects.all().only('last_version'):
+                repository.last_version = int(repository.last_version) + 1


I think you could try something like this, it might be more efficient: https://stackoverflow.com/questions/12661253/how-to-bulk-update-with-django

The main benefit though is that I think .save() would change the last_updated timestamp and while I don't think that's necessarily a problem we should avoid it if we can help it.

dralley · 2020-02-20T14:26:48Z

pulpcore/app/models/repository.py

@@ -93,10 +92,8 @@ def new_version(self, base_version=None):
        with transaction.atomic():
            version = RepositoryVersion(
                repository=self,
-                number=int(self.last_version) + 1,


I love that this logic was made simpler!

dralley · 2020-02-20T14:31:51Z

pulpcore/app/models/repository.py

@@ -748,7 +760,7 @@ def delete(self, **kwargs):
                RepositoryContent.objects.filter(version_removed=self) \
                    .update(version_removed=None)
                CreatedResource.objects.filter(object_id=self.pk).delete()
-                self.repository.last_version = self.number - 1
+                self.repository.next_version = self.number - 1


I think this line should be deleted entirely. We should only increment repository.next_version once the repository version has been successfully created and finalized, so we shouldn't ever need to decrement it. If the repository version wasn't completed then it shouldn't have been incremented so we shouldn't need to decrement it here.

dralley · 2020-02-20T14:33:54Z

pulpcore/app/models/repository.py

+            adding = self._state.adding
+            super().save(*args, **kwargs)
+            if adding:
+                self.repository.next_version = self.number + 1


I think this actually needs to happen here: https://github.com/pulp/pulpcore/pull/555/files#diff-ae59196c41f139ec2b7e3ae3aa581a45R832

dralley

@fabricio-aguiar This looks excellent, thank you!

I think this should have a review from at least one other person, David/Brian/Dennis

dralley · 2020-02-20T21:22:31Z

pulpcore/app/migrations/0022_rename_last_version.py

+    # re: https://pulp.plan.io/issues/6147
+    with transaction.atomic():
+        Repository = apps.get_model('core', 'Repository')
+        if 'last_version' in dir(Repository):


I'm not totally sure I understand what the conditional does, do some Repository objects not have this field?

it is for not breaking if for any reason someone re-run the migration, because after migration last_version will no longer exist

I don't think it's possible to re-run a migration unless you go out of your way to do so, the most recent migration is stored somewhere and it picks up from there afterwards

It's not possible to rerun a migration because we don't provide downgrade migrations. Is this part of the logic still needed?

I just added it in case someone would rerun, since it is not possible to rerun, it is not needed

daviddavis

LGTM

bmbouter · 2020-02-21T15:53:09Z

CHANGES/6147.removal

@@ -0,0 +1 @@
+Replacing Repository.last_version with Repository.next_version


Can we rephrase this some? Can it be Renaming Repository.last_version to Repository.next_version

https://pulp.plan.io/issues/6147 closes #6147

bmbouter

Thank you @fabricio-aguiar this looks great!

[noissue] Notice this with pulp/pulpcore#555 Renaming last_version was breaking pulp_rpm

https://pulp.plan.io/issues/6147 Required PR: pulp/pulpcore#555 ref #6147

dralley reviewed Feb 19, 2020

View reviewed changes

fao89 force-pushed the 6147 branch 4 times, most recently from cfd5e91 to 260ddd4 Compare February 19, 2020 21:28

fao89 changed the title ~~Deleted versions update Repository.last_version~~ Repository.latest_version() considering deleted versions Feb 19, 2020

fao89 requested review from daviddavis, dkliban and dralley February 20, 2020 14:01

dralley reviewed Feb 20, 2020

View reviewed changes

fao89 force-pushed the 6147 branch from 260ddd4 to 5f88d01 Compare February 20, 2020 14:53

dralley approved these changes Feb 20, 2020

View reviewed changes

dralley reviewed Feb 20, 2020

View reviewed changes

daviddavis approved these changes Feb 20, 2020

View reviewed changes

fao89 force-pushed the 6147 branch from 5f88d01 to 2006f37 Compare February 21, 2020 15:47

bmbouter reviewed Feb 21, 2020

View reviewed changes

Deleted versions update Repository.last_version

d038c1f

https://pulp.plan.io/issues/6147 closes #6147

fao89 force-pushed the 6147 branch from 2006f37 to d038c1f Compare February 21, 2020 15:55

bmbouter approved these changes Feb 21, 2020

View reviewed changes

fao89 added a commit to fao89/pulp_rpm that referenced this pull request Feb 21, 2020

Remove core code

156db09

[noissue] Notice this with pulp/pulpcore#555 Renaming last_version was breaking pulp_rpm

fao89 added a commit to fao89/pulp_rpm that referenced this pull request Feb 21, 2020

Rename last_version

0d10299

https://pulp.plan.io/issues/6147 Required PR: pulp/pulpcore#555 ref #6147

fao89 added a commit to fao89/pulp_rpm that referenced this pull request Feb 21, 2020

Rename last_version

8940ce0

https://pulp.plan.io/issues/6147 Required PR: pulp/pulpcore#555 ref #6147

fao89 mentioned this pull request Feb 21, 2020

Rename last_version pulp/pulp_rpm#1607

Merged

fao89 added a commit to fao89/pulp_rpm that referenced this pull request Feb 21, 2020

Rename last_version

89bdff7

https://pulp.plan.io/issues/6147 Required PR: pulp/pulpcore#555 ref #6147

daviddavis merged commit 7be835f into pulp:master Feb 24, 2020

daviddavis pushed a commit to pulp/pulp_rpm that referenced this pull request Feb 24, 2020

Rename last_version

aa22d82

https://pulp.plan.io/issues/6147 Required PR: pulp/pulpcore#555 ref #6147

Repository.latest_version() considering deleted versions #555

Repository.latest_version() considering deleted versions #555

fao89 commented Feb 19, 2020

dralley Feb 19, 2020 •

edited

fao89 Feb 19, 2020

dralley Feb 19, 2020

dralley Feb 19, 2020 •

edited

dralley Feb 19, 2020 •

edited

dkliban Feb 19, 2020

daviddavis Feb 19, 2020

dralley Feb 19, 2020

dkliban Feb 19, 2020

dralley Feb 20, 2020

dralley Feb 20, 2020

dralley Feb 20, 2020

dralley Feb 20, 2020

dralley left a comment

dralley Feb 20, 2020

fao89 Feb 20, 2020

dralley Feb 20, 2020

bmbouter Feb 21, 2020

fao89 Feb 21, 2020

daviddavis left a comment

bmbouter Feb 21, 2020

bmbouter left a comment

		@@ -0,0 +1 @@
		Replacing Repository.last_version with Repository.next_version

Repository.latest_version() considering deleted versions #555

Repository.latest_version() considering deleted versions #555

Conversation

fao89 commented Feb 19, 2020

dralley Feb 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Feb 19, 2020 • edited

Choose a reason for hiding this comment

dralley Feb 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddavis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter left a comment

Choose a reason for hiding this comment

dralley Feb 19, 2020 •

edited

dralley Feb 19, 2020 •

edited

dralley Feb 19, 2020 •

edited