Refactor the repository retrieval in get_dr_push #1376

lubosmj · 2023-08-25T18:38:36Z

closes #1275
closes #489

ipanova · 2023-10-31T15:44:41Z

CHANGES/1275.misc

@@ -0,0 +1,2 @@
+Ensured repositories are correctly retrieved or initialized when handling concurrent requests


s/when handling concurrent requests/when handling client parallel blob upload

ipanova · 2023-10-31T15:51:21Z

pulp_container/app/registry_api.py

+        try:
+            dist_serializer.is_valid(raise_exception=True)
+            distribution = dist_serializer.create(dist_serializer.validated_data)
+        except (IntegrityError, ValidationError):


you should catch these error separately because the error message is different

ipanova · 2023-10-31T15:53:47Z

pulp_container/app/registry_api.py

@@ -349,6 +325,32 @@ def get_dr_push(self, request, path, create=False):
                raise RepositoryNotFound(name=path)
        return distribution, repository

+    @transaction.atomic


there is a different wrapping into a transaction whole method vs just the try block

I removed this change. The logic should be similar to the previous one, where we catch exceptions outside the transaction.atomic()'s context manager.

lubosmj · 2023-11-01T17:36:48Z

@ipanova, I believe this is the best I can do. We no longer granularly handle the validation and creation of entities inside pulp-container, see https://github.com/pulp/pulpcore/blob/3594624a3556af2ad400c29871041252ae947495/pulpcore/app/serializers/base.py#L360.

ipanova · 2023-11-01T19:37:43Z

pulp_container/app/registry_api.py

+                distribution = serializers.ContainerDistributionSerializer.get_or_create(
+                    {"base_path": path, "name": path}, {"repository": get_url(repository)}
+                )
+        except ObjectDoesNotExist:


this seems to be unnecessary, we whether retrieve existing instance or create a new one

I think this is necessary. Once the validation fails (e.g., because of an invalid base_path), it tries to fetch an object from the database: https://github.com/pulp/pulpcore/blob/3594624a3556af2ad400c29871041252ae947495/pulpcore/app/serializers/base.py#L374. If the object does not exist after the last attempt, we should return RepositoryNotFound.

I see what you're saying and you are right only because the get_or_create is not implemented fully as it should be. Get or create implies that on success an object will always be returned and failure shuld not be related to object existence but rather only validation.
Here https://github.com/pulp/pulpcore/blob/3594624a3556af2ad400c29871041252ae947495/pulpcore/app/serializers/base.py#L372 we should not catch these together but separately, because if validation fails because of field being missing or invalid we need to raise it. And if it failed because 'this field must be unique; then we retrieve existing object.
Here I took a stab pulp/pulpcore#4643

ipanova · 2023-11-01T19:38:07Z

pulp_container/app/registry_api.py

+                    {"name": path}
+                )
+                distribution = serializers.ContainerDistributionSerializer.get_or_create(
+                    {"base_path": path, "name": path}, {"repository": get_url(repository)}


great, you figured it 👍

here you need to check whether returned distribution has a repo. and if it has it should be same repo-id as on line 330 otherwise we need to raise an error that repo is read-only
how about this workflow:

create repo(regular) foo and distribution foo

docker push foo and it should fail

ipanova · 2023-11-01T19:45:08Z

pulp_container/app/registry_api.py

+            raise RepositoryInvalid(name=path)
+
+        repository = repository.cast()
+        if not repository.PUSH_ENABLED:


yeah so this needs to be moved upper per my previous comment. now you're checking the repo you have created on line 330

ipanova · 2023-11-01T19:47:35Z

pulp_container/app/registry_api.py

-                        repository = repository.cast()
-                        if not repository.PUSH_ENABLED:
-                            raise RepositoryInvalid(name=path, message="Repository is read-only.")
+                distribution, repository = self.create_dr(path, request)
            else:
                raise RepositoryNotFound(name=path)
        else:


Have you checked the else branch? I think it needs re-working too.

line 328 makes little sense without trying to understand much the code, can you double check the logic if this is still needed?

I revised the else branch and it also needs to switch the serializers to use get_or_create.

The code has changed significantly after the approval. New comments arose.

lubosmj · 2023-11-06T11:17:31Z

Waiting for pulp/pulpcore@bf0654b to be released.

ipanova · 2023-11-08T16:01:39Z

pulp_container/app/registry_api.py

                        )
-                        repo_serializer.is_valid(raise_exception=True)
-                        repository = repo_serializer.create(repo_serializer.validated_data)
                        distribution.repository = repository
                        distribution.save()
                except IntegrityError:


this can be removed

I was unsure if save() could raise any error. By the way, should we enforce saving the object just in time with force_update=True?

i don't think so https://docs.djangoproject.com/en/dev/ref/models/instances/#ref-models-force-insert

ipanova · 2023-11-08T16:09:04Z

pulp_container/app/registry_api.py

+                {"base_path": path, "name": path}, {"repository": get_url(repository)}
+            )
+
+            dist_repository = distribution.repository.cast()


you need to account for the case when distribution.repository will be None, cast will fail

ipanova · 2023-11-08T17:26:37Z

pulp_container/app/registry_api.py

+                {"base_path": path, "name": path}, {"repository": get_url(repository)}
+            )
+
+            if distribution.repository:


it needs an else:
distribution.repository = repository
distribution.save()

It is not needed if we create a new distribution. We want to check the names and references without updating anything.

In [1]: from pulp_container.app.serializers import ContainerPushRepositorySerializer In [2]: from pulp_container.app.serializers import ContainerDistributionSerializer In [3]: repo = ContainerPushRepositorySerializer.get_or_create({"name": "bla"}) In [4]: from pulp_container.app.models import ContainerPushRepository, ContainerDistribution In [5]: ContainerPushRepository.objects.filter(name="bla") Out[5]: <QuerySet [<ContainerPushRepository (pulp_type=container.container-push): pk=018bb01f-a735-79ab-a913-8878aec9226c>]> In [6]: ContainerDistribution.objects.filter(name="bla") Out[6]: <QuerySet []> In [7]: from pulpcore.plugin.util import get_url In [8]: dist = ContainerDistributionSerializer.get_or_create({"name": "bla", "base_path": "bla"}, {"repository": get_url(repo)}) In [9]: dist Out[9]: <ContainerDistribution (pulp_type=container.container): pk=018bb022-33d0-723e-8562-576ce14e9ed8> In [10]: dist.repository Out[10]: <ContainerPushRepository (pulp_type=container.container-push): pk=018bb01f-a735-79ab-a913-8878aec9226c> In [11]: repo Out[11]: <ContainerPushRepository (pulp_type=container.container-push): pk=018bb01f-a735-79ab-a913-8878aec9226c>

The code path suggests that we are about to create a new distribution:

distribution = models.ContainerDistribution.objects.get(base_path=path) except models.ContainerDistribution.DoesNotExist: if create: distribution, repository = self.create_dr(path, request)

Do you agree?

i agree that we are about to create new distribution, however to account for race condition during get_or_create ( and that's why we're using it here) another process could have created a distribution(without a repo) and the get_or_create would get the newly-created-dist by another process

Yes, you are right!

ipanova · 2023-11-08T17:28:21Z

pulp_container/app/registry_api.py

+                    )
+                    distribution.repository = repository
+                    distribution.save()
+                # Seems like another process created our stuff already. Retry fetching it.


line 309-315 can be entirely removed

closes pulp#1275 closes pulp#489

lubosmj · 2023-11-09T10:45:23Z

I am closing #489 with this change.

ipanova

🥂

lubosmj marked this pull request as ready for review August 25, 2023 20:58

lubosmj force-pushed the refactor-get-dr-push-intererrors branch from cbfcfb4 to ef0d823 Compare October 31, 2023 13:53

ipanova reviewed Oct 31, 2023

View reviewed changes

lubosmj force-pushed the refactor-get-dr-push-intererrors branch from ef0d823 to aa2bf06 Compare November 1, 2023 13:22

ipanova previously approved these changes Nov 1, 2023

View reviewed changes

lubosmj marked this pull request as draft November 1, 2023 14:05

lubosmj force-pushed the refactor-get-dr-push-intererrors branch 2 times, most recently from 695a008 to 13906c8 Compare November 1, 2023 17:03

lubosmj marked this pull request as ready for review November 1, 2023 17:37

ipanova reviewed Nov 1, 2023

View reviewed changes

lubosmj marked this pull request as draft November 3, 2023 12:41

lubosmj force-pushed the refactor-get-dr-push-intererrors branch 3 times, most recently from 4954d4c to 424f2a3 Compare November 8, 2023 14:43

lubosmj marked this pull request as ready for review November 8, 2023 15:14

lubosmj requested a review from ipanova November 8, 2023 15:14

ipanova reviewed Nov 8, 2023

View reviewed changes

lubosmj force-pushed the refactor-get-dr-push-intererrors branch from 424f2a3 to f761804 Compare November 8, 2023 17:11

ipanova reviewed Nov 8, 2023

View reviewed changes

lubosmj force-pushed the refactor-get-dr-push-intererrors branch from f761804 to 8ea9166 Compare November 8, 2023 18:20

lubosmj force-pushed the refactor-get-dr-push-intererrors branch from 8ea9166 to b856afd Compare November 8, 2023 19:52

lubosmj requested a review from ipanova November 8, 2023 19:53

Refactor the repository retrieval in get_dr_push

afe751c

closes pulp#1275 closes pulp#489

lubosmj force-pushed the refactor-get-dr-push-intererrors branch from b856afd to afe751c Compare November 9, 2023 10:29

ipanova approved these changes Nov 9, 2023

View reviewed changes

lubosmj merged commit 499e084 into pulp:main Nov 9, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the repository retrieval in get_dr_push #1376

Refactor the repository retrieval in get_dr_push #1376

lubosmj commented Aug 25, 2023 •

edited

Loading

ipanova Oct 31, 2023 •

edited

Loading

ipanova Oct 31, 2023 •

edited

Loading

ipanova Oct 31, 2023

lubosmj Nov 1, 2023

lubosmj commented Nov 1, 2023

ipanova Nov 1, 2023

lubosmj Nov 2, 2023

ipanova Nov 2, 2023 •

edited

Loading

ipanova Nov 1, 2023

ipanova Nov 1, 2023 •

edited

Loading

ipanova Nov 1, 2023

ipanova Nov 1, 2023

ipanova Nov 1, 2023

ipanova Nov 2, 2023

lubosmj commented Nov 6, 2023

ipanova Nov 8, 2023

lubosmj Nov 8, 2023

ipanova Nov 8, 2023

ipanova Nov 8, 2023 •

edited

Loading

lubosmj Nov 8, 2023

ipanova Nov 8, 2023 •

edited

Loading

lubosmj Nov 8, 2023

lubosmj Nov 8, 2023 •

edited

Loading

ipanova Nov 8, 2023

lubosmj Nov 8, 2023

ipanova Nov 8, 2023

lubosmj commented Nov 9, 2023

ipanova left a comment

		@@ -0,0 +1,2 @@
		Ensured repositories are correctly retrieved or initialized when handling concurrent requests

Refactor the repository retrieval in get_dr_push #1376

Refactor the repository retrieval in get_dr_push #1376

Conversation

lubosmj commented Aug 25, 2023 • edited Loading

ipanova Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

ipanova Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj commented Nov 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipanova Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipanova Nov 1, 2023 • edited Loading

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

lubosmj commented Nov 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipanova Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipanova Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj Nov 8, 2023 • edited Loading

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

lubosmj commented Nov 9, 2023

ipanova left a comment

Choose a reason for hiding this comment

lubosmj commented Aug 25, 2023 •

edited

Loading

ipanova Oct 31, 2023 •

edited

Loading

ipanova Oct 31, 2023 •

edited

Loading

ipanova Nov 2, 2023 •

edited

Loading

ipanova Nov 1, 2023 •

edited

Loading

ipanova Nov 8, 2023 •

edited

Loading

ipanova Nov 8, 2023 •

edited

Loading

lubosmj Nov 8, 2023 •

edited

Loading