Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Fix marking of distributors #159

Merged
merged 2 commits into from Jun 10, 2020
Merged

Fix marking of distributors #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f66e4f
Fixed marking of old distributors, when distributor only is migrated.
ipanova Jun 5, 2020
32f9ce8
Handle case when a publicaion is served by multiple distributions.
ipanova Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/6932.bugfix
View file
Open in desktop
@@ -0,0 +1 @@
Fixed marking of old distributors, when distributor only is migrated without the repo.
1 change: 1 addition & 0 deletions CHANGES/6947.bugfix
View file
Open in desktop
@@ -0,0 +1 @@
Fixed case when a publication is shared by multiple distributions.
8 changes: 7 additions & 1 deletion docs/plugin_writers_guide.rst
View file
Open in desktop
Expand Up @@ -37,7 +37,13 @@ first stage of DeclarativeContentMigration, on your Content model you also need:

5. Subclass the provided ``Pulp2to3Importer`` class and define ``migrate_to_pulp3`` method which
creates a plugin Remote instance based on the provided pre-migrated ``Pulp2Importer``.
When creating a Remote it is important to use ``update_or_create`` function in case on re-run
Importer has been changed.

6. Subclass the provided ``Pulp2to3Distributor`` class and define ``migrate_to_pulp3`` method which
creates a plugin Publication and/or Distribution instance (depends on the plugin) based on the
provided pre-migrated ``Pulp2Distributor``.
provided pre-migrated ``Pulp2Distributor``.
When creating a Distribution it is important to use ``update_or_create`` function in case on re-run
Distributor has been changed.
It is also important to relate Distributor to Publication or RepoVersion (depends on the plugin)
when defining ``migrate_to_pulp3`` method.
45 changes: 35 additions & 10 deletions pulp_2to3_migration/app/pre_migration.py
View file
Open in desktop
Expand Up @@ -12,8 +12,11 @@
from mongoengine.queryset.visitor import Q as mongo_Q

from pulpcore.constants import TASK_STATES
from pulpcore.plugin.models import ProgressReport

from pulpcore.plugin.models import (
BaseDistribution,
Publication,
ProgressReport,
)
from pulp_2to3_migration.app.models import (
Pulp2Content,
Pulp2Distributor,
Expand Down Expand Up @@ -484,10 +487,21 @@ def pre_migrate_distributor(repo_id, distributors, distributor_migrators, repo=N
needs_new_distribution = dist_migrator.needs_new_distribution(distributor)
remove_publication = needs_new_publication and distributor.pulp3_publication
remove_distribution = needs_new_distribution and distributor.pulp3_distribution

if remove_publication:
# check if publication is shared by multiple distributions
# on the corresponding distributor flip the flag to false so the affected
# distribution will be updated with the new publication
pulp2dists = distributor.pulp3_publication.pulp2distributor_set.all()
for dist in pulp2dists:
if dist.is_migrated:
dist.is_migrated = False
dist.save()
distributor.pulp3_publication.delete()
distributor.pulp3_publication = None
if remove_publication or remove_distribution:
distributor.pulp3_distribution.delete()
distributor.pulp3_distribution = None

distributor.save()
ipanova marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -564,8 +578,8 @@ def mark_removed_resources(plan, type_to_repo_ids):

# Mark importers
mongo_imp_object_ids = set(str(i.id) for i in Importer.objects.only('id'))
premigrated_imps = Pulp2Importer.objects.filter(
pulp2_repository__pulp2_repo_type=plugin_plan.type)
imp_types = plugin_plan.migrator.importer_migrators.keys()
premigrated_imps = Pulp2Importer.objects.filter(pulp2_type_id__in=imp_types)
premigrated_imp_object_ids = set(premigrated_imps.values_list('pulp2_object_id',
flat=True))
removed_imp_object_ids = premigrated_imp_object_ids - mongo_imp_object_ids
Expand All @@ -582,8 +596,8 @@ def mark_removed_resources(plan, type_to_repo_ids):

# Mark distributors
mongo_dist_object_ids = set(str(i.id) for i in Distributor.objects.only('id'))
premigrated_dists = Pulp2Distributor.objects.filter(
pulp2_repository__pulp2_repo_type=plugin_plan.type)
dist_types = plugin_plan.migrator.distributor_migrators.keys()
premigrated_dists = Pulp2Distributor.objects.filter(pulp2_type_id__in=dist_types)
premigrated_dist_object_ids = set(premigrated_dists.values_list('pulp2_object_id',
flat=True))
removed_dist_object_ids = premigrated_dist_object_ids - mongo_dist_object_ids
Expand Down Expand Up @@ -622,13 +636,24 @@ def delete_old_resources(plan):

with transaction.atomic():
pulp2distributors_with_old_distributions_qs = Pulp2Distributor.objects.filter(
old_dist_query).only('pulp3_distribution')

old_dist_query)
pubs_to_delete = set()
dists_to_delete = []
for pulp2distributor in pulp2distributors_with_old_distributions_qs:
if pulp2distributor.is_migrated:
pulp2distributor.is_migrated = False
pulp2distributor.save()
if pulp2distributor.pulp3_publication:
pulp2distributor.pulp3_publication.delete()
# check if publication is shared by multiple distributions
# on the corresponding distributor flip the flag to false so the affected
# distribution will be updated with the new publication
pulp2dists = pulp2distributor.pulp3_publication.pulp2distributor_set.all()
for dist in pulp2dists:
if dist.is_migrated:
dist.is_migrated = False
dist.save()
Comment on lines +651 to +654
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it enough to mark distributors as is_migrated=False and not save their pk for removal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

distributions that have been affected indirectly ( aka they lost their publication only) it is enough just to flip the is_migrated flag to False so then the distribution will be updated with the new publication.
distributions that have been affected directly( aka repos were recreated in pulp2), will be removed , because the corresponding pulp2distributor will be present in this pulp2distributors_with_old_distributions_q

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting that the affected ones are updated later

Copy link
Member Author

@ipanova ipanova Jun 10, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is hit where we take into consideration distributors that are in plan and is_mifrated=False https://github.com/pulp/pulp-2to3-migration/blob/master/pulp_2to3_migration/app/migration.py#L224
then we are trying to migrate that distributor https://github.com/pulp/pulp-2to3-migration/blob/master/pulp_2to3_migration/app/migration.py#L331
and here we are 1) taking already created publication from the repo and 2) update the existing distribution https://github.com/pulp/pulp-2to3-migration/blob/master/pulp_2to3_migration/app/plugin/iso/repository.py#L76 that's why it is unnecessary to remove it in the remove_old_resources

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The missing part for me was that we take care of publication update in every plugin which has publications. And also publication ends up as a part of base_config, it was unexpected I missed that.

So I'm torn now. Let me know what you think.
My concern is that plugin writer needs to know that they are responsible for the update case (when publication changed) and not only for a create one. So we can either add some docs to the plugin writer's guide about that. Or we can remove distributors here and they will only worry about creation.
I'm fine with both, not so many plgin writers will need to extend the migration plugin. Just the problem is painful if the implementation of update case is missed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we keep publication or repo_version( depending on the plugin) among base_config data then it will be handled in obth cases, whether it is a create or update or the object.
If we remove the distribution but this line is missing https://github.com/pulp/pulp-2to3-migration/blob/master/pulp_2to3_migration/app/plugin/iso/repository.py#L75 it does not really matter whether we remove the distribution or just flip the flag in pulp2dist to later update the distribution, in both cases distribution object will miss the publication

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tldr the problem is not in the update or creation of the object but to make sure we include the relation between the publication and distribution

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to document that it is important to use update_or_create in distributions/remotes

pubs_to_delete.add(pulp2distributor.pulp3_publication.pk)
if pulp2distributor.pulp3_distribution:
pulp2distributor.pulp3_distribution.delete()
dists_to_delete.append(pulp2distributor.pulp3_distribution.pk)
Publication.objects.filter(pk__in=pubs_to_delete).delete()
BaseDistribution.objects.filter(pk__in=dists_to_delete).delete()