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

Only go through the changed repositories on the re-run. #311

Merged
merged 1 commit into from
Feb 18, 2021
Merged

Only go through the changed repositories on the re-run. #311

merged 1 commit into from
Feb 18, 2021

Conversation

goosemania
Copy link
Member

@goosemania
Copy link
Member Author

@dralley , @ggainey , @ipanova , this PR is far from being well tested, but I'd appreciate any thoughts or concerns on the approach.

dralley
dralley reviewed Feb 8, 2021
View reviewed changes
pulp_2to3_migration/app/pre_migration.py Outdated
@@ -355,8 +355,45 @@ def pre_migrate_all_without_content(plan):
# filter by repo type
repos_to_check = plan.type_to_repo_ids[plugin_plan.type]

mongo_repo_q = mongo_Q(repo_id__in=repos_to_check)
mongo_repo_qs = Repository.objects(mongo_repo_q)
# figure out which repositories/importers/distributors were updated since the last run
Copy link
Contributor

@dralley dralley Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

☹️ (more code)

@goosemania
Copy link
Member Author

Approach

Looking at the last updated times in the pulp2to3 tables for repositories/importers/distributors.
Query only empty repos (can't filter them out in any way) or repos for which:

  • there were any content changes
  • importer changes
  • distributor changes
    Query in order of last_unit_added for the case when pre-migration is interrupted before we are done with repositories.

@ggainey
Copy link
Collaborator

ggainey commented Feb 8, 2021

Approach

Just a note, I'd like to see this as a comment at the start of the code-section. I'm glad that my understanding after reading the code matches this - but it'd be WAY easier for the next person, to have this summary right next to the complicated scenario it describes.

@goosemania goosemania marked this pull request as ready for review February 11, 2021 21:35
ipanova
ipanova reviewed Feb 15, 2021
View reviewed changes
CHANGES/7779.bugfix Outdated Show resolved Hide resolved
@ipanova
Copy link
Member

ipanova commented Feb 15, 2021 •
edited
Loading

The PR looks good to me. I would add comments on some querysets, while reading the code i needed to go back up and re-fresh my memory about what is_new_enough_repo_q contains, for example.

@goosemania goosemania force-pushed the issue7779 branch 2 times, most recently from 7f5c476 to 958d33e Compare February 15, 2021 21:15
@goosemania
Copy link
Member Author

This improved a huge setup migration only by 25% which is not good enough, so I continue working on it

@goosemania goosemania added WIP and removed WIP labels Feb 16, 2021
@ggainey
Copy link
Collaborator

ggainey commented Feb 17, 2021

This improved a huge setup migration only by 25% which is not good enough, so I continue working on it

The most-recent pushed changes took the migration-setup from 8.5 hours down to 1. There may be more we can do, but I think an order of magnitude is a great place to start.

ggainey
ggainey approved these changes Feb 17, 2021
View reviewed changes
Copy link
Collaborator

@ggainey ggainey left a comment

Choose a reason for hiding this comment

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

This code had a 10x impact on our worst-case server migration. I approve; I'd like to see it accepted, and if there's more to do in this area, let's do it in a new issue/PR.

dralley
dralley approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@dralley dralley left a comment

Choose a reason for hiding this comment

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

This looks good, I just left some suggestions about unrelated code.

@goosemania
Only go through the changed repositories on the re-run.
bc4ab3a 
Also improved how repo type is determined for complex plans.
There is no need to look at repo-content relations in such case becasue the plugin type is clear from the plan.

closes #7779
https://pulp.plan.io/issues/7779
@goosemania goosemania merged commit 7c200b5 into pulp:master Feb 18, 2021
@goosemania goosemania deleted the issue7779 branch February 18, 2021 17:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ipanova ipanova ipanova left review comments

@dralley dralley dralley approved these changes

@ggainey ggainey ggainey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@goosemania @ggainey @ipanova @dralley