Multiple plugin migrations and migrating empty repos when specified in the plan #87
Conversation
891cdc6
to
58e2133
Compare
Currently, I can only test this with pulpcore 3.0 and pulp_container 1.0, since the docker migration is broken with 3.1 |
Daniel, have you tested with a MP where you migrate fully all iso plugin
and a specific docker repo? Or specific iso repo and specific repo? I was
thinking we'd need to change how we process the plan and get it to the
structure where we split repos, importers, distributors per plugin.
…On Tue, 4 Feb 2020, 19:48 Daniel Alley, ***@***.***> wrote:
closes: #5978
https://pulp.plan.io/issues/5978
------------------------------
You can view, comment on, or merge this pull request online at:
#87
Commit Summary
- Fix migration of multiple plugins simultaneously
File Changes
- *M* pulp_2to3_migration/app/migration.py
<https://github.com/pulp/pulp-2to3-migration/pull/87/files#diff-0>
(28)
- *M* pulp_2to3_migration/app/serializers.py
<https://github.com/pulp/pulp-2to3-migration/pull/87/files#diff-1> (2)
Patch Links:
- https://github.com/pulp/pulp-2to3-migration/pull/87.patch
- https://github.com/pulp/pulp-2to3-migration/pull/87.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#87?email_source=notifications&email_token=ABAOQ6HN5QKOEUFC75JBT23RBGZ6HA5CNFSM4KP4AFTKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IK7RKQA>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOQ6DQNOV3CLWNKVAVSCTRBGZ6HANCNFSM4KP4AFTA>
.
|
I'm still working on testing it thoroughly (I wasn't really expecting it to be looked at today). Probably needs a WIP tag. |
No problem, I did not really look at it just quickly scrolled through.
…On Tue, 4 Feb 2020, 20:32 Daniel Alley, ***@***.***> wrote:
I'm still working on testing it thoroughly (I wasn't really expecting it
to be looked at today). Probably needs a WIP tag.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#87?email_source=notifications&email_token=ABAOQ6EHDCMCIQWRLE2TG3LRBG7DHA5CNFSM4KP4AFTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKY4TTA#issuecomment-582076876>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOQ6BW4RZWHVKH67FI6PLRBG7DHANCNFSM4KP4AFTA>
.
|
@ipanova I used this plan to test:
And got the following results, which looks correct.
|
@ipanova However, I think I've found a bug in the docker migrator. Using this plan (which only migrates docker, no iso) and a clean database, and pulpcore 3.0, pulp_container 1.0, and master branch of the migration plugin (ha! not making that mistake again...): If you run a migration plan for the docker plugin twice (any plan), you get this
I've filed an issue for this: https://pulp.plan.io/issues/6099 |
Aside from that, a migration plan for all ISO and specific docker does seem to have issues though. |
I got it mostly working with that example (blanket ISO migrate, specific docker), it still needs a little more work and testing. |
The container plugin doesn't use publications, correct? |
correct, it has only distributions |
f4d293e
to
1e01437
Compare
Thanks @ipanova ! This should be working now. Tested with:
|
e7c1a23
to
62bf203
Compare
6cb5ee9
to
523cb5c
Compare
Also, should now migrate empty repositories when they're specified. |
pulp_2to3_migration/app/migration.py
Outdated
pulp3_distribution=None, | ||
pulp3_publication=None, | ||
not_in_plan=False, | ||
pulp2_type_id=plugin.type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when i run the migration it does not create any publications or distributions, i think because of this line.
In [3]: Pulp2Distributor.objects.all()[0].pulp2_type_id
Out[3]: 'iso_distributor'
in plugin.type you will have 'iso'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can fetch the types for distributors from the plugin.migrator.distriubutor_migrators.keys()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dralley you applied this logic to pulp2repos and pulp2 distributors, what about pulp2importers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, I didn't seem to have that issue -- but I'll check again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, I didn't seem to have that issue -- but I'll check again.
@@ -156,6 +156,7 @@ class Pulp2RepositoriesSerializer(ModelSerializer): | |||
) | |||
pulp2_object_id = serializers.CharField(max_length=255) | |||
pulp2_repo_id = serializers.CharField() | |||
type = serializers.CharField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about pulp2_repo_type to stay consistent with other fields names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I rename the database field as well? It is just type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you also like the database field renamed? It is just type
3f7fd96
to
baabd6c
Compare
a3e07ea
to
9317b43
Compare
@ipanova With continued testing I found a new bug :( I'll let you know when this is ready |
9317b43
to
f797801
Compare
Working again @ipanova |
pulp_2to3_migration/app/migration.py
Outdated
|
||
for plugin in plan.get_plugin_plans(): | ||
distributor_migrators.update(**plugin.migrator.distributor_migrators) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we process distributors by plugin type now, i believe we can just assign directly distributor_migrators = plugin.migrator.distributor_migrators
, no need to update the dict
pulp_2to3_migration/app/migration.py
Outdated
not_in_plan=False, | ||
pulp2_type_id__in=distributor_types | ||
) | ||
pb.total = pulp2distributors_qs.count() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pb.total += pulp2distributors_qs.count()
pulp2_type_id__in=distributor_types | ||
) | ||
pb.total = pulp2distributors_qs.count() | ||
pb.save() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should save the counters outside of the for loop not that important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a minor comment about counters. I have tested various corner cases and they do seems to work!
nice work 🌴 🍺
f797801
to
648bcfc
Compare
closes: #5978
https://pulp.plan.io/issues/5978
closes: #5978
https://pulp.plan.io/issues/5978