Fix dist serializer. #264

ipanova · 2020-11-11T12:47:08Z

No description provided.

ipanova · 2020-11-12T11:06:01Z

@goosemania @dralley i seemed to address issue distribution serialization however the data is still not displayed correctly because of this isue https://pulp.plan.io/issues/7823

pulp_2to3_migration/app/models/repository.py

goosemania · 2020-11-13T11:33:52Z

pulp_2to3_migration/app/migrations/0017_pulp2distributor_pulp2_repos.py

+    Pulp2Distributor = apps.get_model('pulp_2to3_migration', 'Pulp2Distributor')
+    pulp2_dists_qs = Pulp2Distributor.objects.all()
+
+    for dist in pulp2_dists_qs:


s/pulp2_dists_qs/pulp2_dists_qs.iterator()/

goosemania · 2020-11-13T11:41:00Z

pulp_2to3_migration/app/models/repository.py

@@ -175,7 +176,7 @@ class Pulp2Distributor(BaseModel):
                                              null=True)

    class Meta:
-        unique_together = ('pulp2_repository', 'pulp2_id')
+        unique_together = ('pulp2_object_id',)


Do you remember why we had unique_together = ('pulp2_repository', 'pulp2_id') ?
Is pulp2_repository always native to the distributor? or is it the repository from the plan?
If it's always native, then I think we can change the uniqueness.

i don't remember why we had this constraint, in my understanding( and memory) pulp2_repository was always native to the distributor

goosemania · 2020-11-13T12:23:25Z

pulp_2to3_migration/app/pre_migration.py

                      'pulp2_repo_id': repo_id,
                      'is_migrated': False})

+
+        if repo:
+            distributor.pulp2_repos.add(repo)


You are adding a native repo (not the one from the plan) here, is it on purpose?

yes it is on purpose.
pulp2_repos will contain both native repos and repos from plan, this is needed to cover:

simple plan --> it will always take and serialize native objects

complex plan--> even though this relation will contain a mix of native relations and those specified in plan, in the serializer we will display only those distributors that are migrated and are in plan

We agreed to add in this condition the pulp3_dist relations as well to cover the simple migration plan

goosemania · 2020-11-13T12:28:14Z

pulp_2to3_migration/app/serializers.py

+        dists = obj.pulp3_dists.filter(not_in_plan=False, is_migrated=True)
+        return [get_pulp_href(dist.pulp3_distribution) for dist in dists]


goosemania · 2020-11-13T15:06:15Z

pulp_2to3_migration/app/plugin/docker/repository.py

        if not repo_version:
-            repo_version = pulp2distributor.pulp2_repository.pulp3_repository_version
+            repo = pulp2distributor.pulp2_repos.filter(not_in_plan=False, is_migrated=True)
+            repo_version = repo.pulp3_repository_version


Can you add a comment in the plugins that this will go away with the simple-complex plan conversion work?

ipanova · 2020-11-13T19:57:31Z

pulp_2to3_migration/app/pre_migration.py

                      'pulp2_repo_id': repo_id,
                      'is_migrated': False})

+        if repo:


@goosemania I started to go the way we have discussed on the call and then I have realized that we have probably confused ourselves. I have not added anything.
Simple plan- the relations will be handled here, because in simple plan we always have 1:1 repo vs distributor, so we do not need to add anything else in the simple_plugin_migration
Complex plan - if you happened to specify a native distributor from the repo you are trying to migrate, it will be added here, and then later in complex_repo_migration

I agree that simple one is going to work.
I have a new concern. I think we need to identify if it's a simple plan and only then add the native distributor.

Imagine a complex plan with swapped distributors:

for repo A use distributor B

for repo B use distributor A

I think with the current approach both repos will have both distributors which is wrong.
So I propose to add a native distributor only for simple plan:

if distributors and repo: repo.pulp2_dists.add(distributor)

Thoughts?

did you mean :

if not distributors and repo: repo.pulp2_dists.add(distributor)

In case it is a simple plan distributors will be [], but in case it is a complex plan it will contain all the distributors specified in it. This is a fine approach for now, but once we get done simple-complex plan conversion work, it will no longer work, which is fine. We just need to leave a note to remove these 2 lines.

Another option where to add the relations in case of simple plan would be in simple_plugin_migration

$ git diff diff --git a/pulp_2to3_migration/app/migration.py b/pulp_2to3_migration/app/migration.py index 8380e14..680c604 100644 --- a/pulp_2to3_migration/app/migration.py +++ b/pulp_2to3_migration/app/migration.py @@ -189,6 +189,8 @@ def simple_plugin_migration(plugin): for pulp2_dist in pulp2distributors_qs: dist_migrator = distributor_migrators.get(pulp2_dist.pulp2_type_id) migrate_repo_distributor(dist_migrator, progress_dist, pulp2_dist) + pulp2_repo = repos_to_migrate.filter(pulp2_repo_id=pulp2_dist.pulp2_repo_id)[0] + pulp2_repo.pulp2_dists.add(pulp2_dist)

This piece of code ^ will entirely go away after simple-complex plan conversion

I think I prefer the first option.

@jlsherrill 1)when you create a complex migration plan is it a normal case when you use a distributor from a different repo, e.g. distribute repoA with the ditributor from repoB?
2) is it supported to swap distributors/importers between the migrations? for example in first run you distribute repoA with the distributor from repoB and repoB with the distributor from repoA, and in the second run(in the new plan) you distribute repoA with the distributor from repoA and repoB with the distributor from repoB?

The situation you describe would not happen, however, we could have:

Repo A
Repo B

and ask you to distribute the repo from Repo A with the distributor from Repo B. Then on the next migration we have:

Repo A
Repo B
Repo C

and now we ask you to distribute Repo C with the distributor from Repo B.

The situation you describe would not happen, however, we could have:

Repo A
Repo B

and ask you to distribute the repo from Repo A with the distributor from Repo B. Then on the next migration we have:

Repo A
Repo B
Repo C

and now we ask you to distribute Repo C with the distributor from Repo B.

And what do you ask to distribute repo A with? some other distributor or repo A is no longer in the plan?

It could still be in the plan, we could ask for it to be distributed via Repo D's distributor.

@ipanova , let's merge and file this as a separate issue. WDYT?

ipanova · 2020-11-13T19:58:47Z

pulp_2to3_migration/app/migration.py

@@ -273,6 +273,9 @@ def complex_repo_migration(plugin, pulp3_repo_setup, repo_name):
                    dist_migrator, progress_dist, dist,
                    migrated_repo.pulp3_repository_version
                )
+                # add distirbutors specified in the complex plan
+                # these can be native and not native distributors
+                migrated_repo.pulp2_dists.add(dist)


here, we will add all the distributors specified in the complex migration plan - whether native or not.
Although, at this point a native relation has been established in the pre_migration step.

goosemania

See comments + please rebase and renumber migrations because of the pulp_deb work got merged.

pulp_2to3_migration/app/migrations/0017_pulp2distributor_pulp2_repos.py

goosemania · 2020-11-19T19:54:06Z

pulp_2to3_migration/app/plugin/docker/repository.py

@@ -48,8 +48,10 @@ def migrate_to_pulp3(cls, pulp2distributor, repo_version):
            created(bool): True if Distribution has just been created;
                           False if Distribution is an existing one
        """
+        # this will go away with the simple-complex plan conversion work


goosemania · 2020-11-19T19:57:22Z

pulp_2to3_migration/app/plugin/rpm/repository.py

+        # this will go away with the simple-complex plan conversion work
        if not repo_version:
-            repo_version = pulp2distributor.pulp2_repository.pulp3_repository_version
+            repo = pulp2distributor.pulp2_repos.filter(not_in_plan=False, is_migrated=True)
+            repo_version = repo[0].pulp3_repository_version


Since pulp_deb migration got merged, could you fix this for pulp_deb as well, plz.

goosemania · 2020-11-19T20:02:07Z

pulp_2to3_migration/app/pre_migration.py

                      'pulp2_repo_id': repo_id,
                      'is_migrated': False})

+        if repo:


I agree that simple one is going to work.
I have a new concern. I think we need to identify if it's a simple plan and only then add the native distributor.

Imagine a complex plan with swapped distributors:

for repo A use distributor B

for repo B use distributor A

I think with the current approach both repos will have both distributors which is wrong.
So I propose to add a native distributor only for simple plan:

if distributors and repo: repo.pulp2_dists.add(distributor)

Thoughts?

docs/workflows.rst

ipanova · 2020-11-20T16:01:34Z

pulp_2to3_migration/app/plugin/deb/repository.py

@@ -62,7 +62,8 @@ def migrate_to_pulp3(cls, pulp2distributor, repo_version):
                           False if Distribution is an existing one;
        """
        if not repo_version:
-            repo_version = pulp2distributor.pulp2_repository.pulp3_repository_version
+            repo = pulp2distributor.pulp2_repos.filter(not_in_plan=False, is_migrated=True)


@quba42 please proofread this change

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

ipanova marked this pull request as draft November 11, 2020 12:47

ipanova force-pushed the dist-serializer branch 4 times, most recently from 5e383c3 to 15dcf14 Compare November 12, 2020 11:05

ipanova force-pushed the dist-serializer branch 4 times, most recently from 4a5c020 to 9082910 Compare November 12, 2020 16:12

goosemania reviewed Nov 13, 2020

View reviewed changes

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

goosemania reviewed Nov 13, 2020

View reviewed changes

ipanova force-pushed the dist-serializer branch 2 times, most recently from d3afddb to 7e5e36b Compare November 13, 2020 12:02

goosemania reviewed Nov 13, 2020

View reviewed changes

ipanova force-pushed the dist-serializer branch 2 times, most recently from f16c0bc to e5a48e4 Compare November 13, 2020 19:53

ipanova commented Nov 13, 2020

View reviewed changes

ipanova marked this pull request as ready for review November 18, 2020 11:51

goosemania suggested changes Nov 19, 2020

View reviewed changes

ipanova force-pushed the dist-serializer branch 2 times, most recently from eaad091 to b9f8e51 Compare November 20, 2020 15:58

ipanova commented Nov 20, 2020

View reviewed changes

docs/workflows.rst Outdated Show resolved Hide resolved

ipanova commented Nov 20, 2020

View reviewed changes

ipanova force-pushed the dist-serializer branch 2 times, most recently from f4ebd7a to 5466662 Compare November 23, 2020 16:58

Fix distribution serialization.

4ef556d

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

ipanova force-pushed the dist-serializer branch from 5466662 to 4ef556d Compare November 23, 2020 17:05

goosemania approved these changes Nov 23, 2020

View reviewed changes

ipanova merged commit 1533ea7 into pulp:master Nov 23, 2020

Fix dist serializer. #264

Fix dist serializer. #264

ipanova commented Nov 11, 2020

ipanova commented Nov 12, 2020

goosemania Nov 13, 2020

goosemania Nov 13, 2020

ipanova Nov 13, 2020

goosemania Nov 13, 2020 •

edited

ipanova Nov 13, 2020

goosemania Nov 13, 2020

goosemania Nov 13, 2020

goosemania Nov 13, 2020

ipanova Nov 13, 2020

goosemania Nov 19, 2020

ipanova Nov 20, 2020

ipanova Nov 20, 2020

jlsherrill Nov 23, 2020

goosemania Nov 23, 2020

jlsherrill Nov 23, 2020

goosemania Nov 23, 2020

ipanova Nov 13, 2020

goosemania left a comment

goosemania Nov 19, 2020

goosemania Nov 19, 2020

goosemania Nov 19, 2020

ipanova Nov 20, 2020

		dists = obj.pulp3_dists.filter(not_in_plan=False, is_migrated=True)
		return [get_pulp_href(dist.pulp3_distribution) for dist in dists]

Fix dist serializer. #264

Fix dist serializer. #264

Conversation

ipanova commented Nov 11, 2020

ipanova commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosemania Nov 13, 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

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

goosemania 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

goosemania Nov 13, 2020 •

edited