Don't create a new repository version if no content was added or removed. #380

dkliban · 2019-11-12T13:48:52Z

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

CHANGES/3308.feature

pulpcore/app/models/repository.py

pulpcore/tests/functional/api/using_plugin/test_crd_publications.py

pulpcore/tests/functional/api/using_plugin/test_repo_versions.py

pulpcore/tests/functional/api/using_plugin/test_tasks.py

pulpcore/tests/functional/api/using_plugin/test_distributions.py

pulpcore/tests/functional/api/using_plugin/test_tasks.py

Required PR: pulp/pulpcore#380 re: #3308 https://pulp.plan.io/issues/3308

gmbnomis · 2019-11-12T18:45:57Z

pulpcore/app/models/repository.py

@@ -628,7 +628,8 @@ def __exit__(self, exc_type, exc_value, traceback):
        """
        Save the RepositoryVersion if no errors are raised, delete it if not
        """
-        if exc_value:
+        no_change = not self.added() and not self.removed()
+        if exc_value or no_change:
            self.delete()


What happens to the task's created_resource entry if there is no change? It is set when creating the new version, but I don't see it being unset anywhere?

I believe the delete cascades to the CreatedResource which inherits from GenericRelationModel.

https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/task.py#L411
https://github.com/pulp/pulpcore/blob/master/pulpcore/app/models/generic.py#L22

Ahh, thank you. Relational databases are magic.

Note that because of how GFKs work the cascade doesn't actually happen in the database, it's enforced by Django. So it might be worthwhile to double check all of this.

gmbnomis · 2019-11-12T18:50:40Z

pulpcore/app/models/repository.py

@@ -628,7 +628,8 @@ def __exit__(self, exc_type, exc_value, traceback):
        """
        Save the RepositoryVersion if no errors are raised, delete it if not
        """
-        if exc_value:
+        no_change = not self.added() and not self.removed()


The way this is implemented right now applies to all finalizations of a repo version. I thought when wanted to implement it for sync only?

I think there is a use case for creating repo versions with empty changes for /modify: If you want to set an old repo version to be the latest, just do a modify giving the old repo_version as the base version and no artifacts to add/remove

If this old version is different from the latest version, then self.added() and self.removed() will contain values that reflect those differences. If you are trying to create the exact same version as the latest, then no new version will be created. What do you think?

I just realized that my mental model of added/removed was wrong when discussing validation/modification with @bmbouter. Hooks for add_content/remove_content actually are different from inspecting added/removed content during finalization.

But you are right of course, .added/.removed is the difference to the previous version.

This convo made me thinking. How does the user of the API finds out to which repo version the "no change" result refers to?

For example, suppose I want to sync and publish

Prior to this change: After the sync, I can use the created repository version resource I got back in the task result for publication. As a user, I am sure that I publish the result of the sync.

With this change: I can do the same if a get a created resource back. But when I don't, this may happen in different scenarios:
a) the latest version is the same as the published one and there is no change -> no need to publish
b) the latest version is not the same as the published one and there is no change between the latest version and the previous one -> publication required

To find out whether I need to publish, I could try to query the repository and get the latest version. But I can't be sure that this is the latest version the sync has seen (another task may have sneaked in). Thus, I may publish the result of a different operation.

To rule out the "sneak in" case, I might be able to infer from the timestamps (latest version vs. task finish) that this has happened.

I discussed this with @dkliban and we came out with two options:

In the no change case, put the latest version used for comparison into the created resource field. Although the version already existed and wasn't created)

Accepting that there is no repo version information is better than providing information in the wrong field

This choice isn't nice (to put it mildly). I would love to hear about other solution proposals.

I'd opt for option (2) because having the data be incorrect is a liability because the user can't distinguish between the one that just ran versus the one that ran two days ago.

It will be a bit difficult for integrators to receive a CreatedResource 80% of the time when calling sync(), and then one day they don't get one. We should document this up-front in the docs (maybe a story could be filed to do so and I could fix soon). It's a case integrators need to think about and be aware of and think about how they want to handle. I think that would be better than us handling it in this way for them, although it will take more effort from them.

Also in terms of a human user, I am hoping for a log-streaming addition to the task API in the future which would allow the sync code to log data directly to the human user. pulp_ansible has a plugin-based version of this. If that feature is available in the future I think human users would know easily.

I'd like to propose option 3, which we have discussed quite a bit. We'd eventually provide an option to sync that would allow users to always create a repo version even if nothing changed. I'd suggest we NOT pursue this until users request it or end up in the situation that @gmbnomis describes but it does solve @gmbnomis's problem should we ever encounter it. It's also backwards compatible which means we don't have to do anything now.

I agree, that reporting something as created that isn't is wrong. (So this will be option 2)
As a long term solution, the tasks could be much more verbose (For failed tasks we already put the whole stacktrace there.). The CreatedResource model could turn into an AffectedResource that has some additional attributes.
But for the short term integrators, the proper solution imo is to query for the last repoversion, trigger the sync/modification (probably even based on that one) and in case none got created, use the result of before. Then there is no option for any other action to sneak in.

@daviddavis and @mdellweg yes, both proposals would solve the problem.

@mdellweg If you can't set the base_version (which is the case for sync), querying the latest repo version first does not help. Between the query and the sync, another task may sneak in. For example, another sync. Then the second sync would most probably produce no change and, consequently, one would not publish the repo version.

@gmbnomis So what you are saying is "If you have two users, one syncs the reopsitory daily at 8 and the other one at 9, the second one will never realize, that he needs to republish." It does not even help to look at that in a more Einsteinian way than Newtonian.
And we surely do not want to resync the same thing from another base_version.
In the end, it again sounds like sync should be 'find or create a repository version that equals upstream'. Alternatively, the sync task must be chained with the publish task that runs even if the new version has not been created by that sync task.

Required PR: pulp/pulpcore#380 re: #3308 https://pulp.plan.io/issues/3308

nixocio · 2019-11-13T03:17:24Z

pulpcore/tests/functional/api/using_plugin/test_distributions.py

@@ -75,8 +83,11 @@ def test_01_create(self):
            self.client.post(FILE_REMOTE_PATH, gen_file_remote())
        )
        # create 3 repository versions
-        for _ in range(3):


Could we still test whether multiples syncs using the same remote it will not created new repository versions? Has this changed was well?

We have tests like that in pulp-file. https://github.com/pulp/pulp_file/pull/311/files

Required PR: pulp/pulpcore#380 closes #3308

daviddavis · 2019-11-13T13:55:07Z

The code in this PR looks good to me. Can you update the docs though to state that new repo versions aren't created if content doesn't change?

Required PR: pulp/pulpcore#380 closes #3308

daviddavis

This LGTM and fulfills my expectations. I would get at least one more approval before merging though.

gmbnomis · 2019-11-13T19:00:23Z

pulpcore/app/models/repository.py

@@ -625,7 +625,8 @@ def __exit__(self, exc_type, exc_value, traceback):
        """
        Finalize and save the RepositoryVersion if no errors are raised, delete it if not
        """
-        if exc_value:
+        no_change = not self.added() and not self.removed()
+        if exc_value or no_change:
            self.delete()
        else:
            self.repository.finalize_new_version(self)


That's at the wrong place now. a finalizer may for example render a changed version into one without a change.

…ved. closes: pulp#3308 https://pulp.plan.io/issues/3308

Required PR: pulp/pulp_file#311 re: pulp#3308 https://pulp.plan.io/issues/3308

Required PR: pulp/pulpcore#380 closes #3308

Adapt to pulp/pulpcore#369 (Repository /modify mixin) and pulp/pulpcore#380 (Don't create a new repository version if no content was added or removed). No functional changes.

dkliban requested a review from a team November 12, 2019 13:48

bmbouter reviewed Nov 12, 2019

View reviewed changes

CHANGES/3308.feature Outdated Show resolved Hide resolved

goosemania reviewed Nov 12, 2019

View reviewed changes

pulpcore/app/models/repository.py Outdated Show resolved Hide resolved

dkliban force-pushed the no-change-no-repo-version branch from a599730 to eba2317 Compare November 12, 2019 14:45