Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve optimize sync conditions #902

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Improve optimize sync conditions #902

merged 1 commit into from
Oct 4, 2023

Conversation

quba42
Copy link
Collaborator

@quba42 quba42 commented Sep 26, 2023

This PR achieves the following:

  • Do not disable optimize if we are switching from mirror to not mirror between two syncs. We can optmize in this case.
  • Log additional reasons why we did not optimize in certain cases.
  • Improve the readability of optimize conditions.

@quba42 quba42 force-pushed the optimize_improvements branch from 73a26ab to 2cee81c Compare September 27, 2023 09:44
@quba42 quba42 changed the title WIP: Improve optimize sync conditions Improve optimize sync conditions Sep 27, 2023
@quba42 quba42 linked an issue Sep 27, 2023 that may be closed by this pull request
When switching from mirrored to not mirrored mode between syncs optimize sync is not used #903
Closed
hstct
hstct reviewed Sep 27, 2023
View reviewed changes
Copy link
Contributor

@hstct hstct left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This should be a test case as well. To test it we need to allow passing arguments to the deb_sync_repository fixture and adapt the deb_init_and_sync fixture to use the arguments as well. The actual test case should be relatively simple then.

@quba42
Copy link
Collaborator Author

quba42 commented Sep 27, 2023

@hstct I wonder if there is some simple way to assert that certain log statements have or have not happened.

@hstct
Copy link
Contributor

hstct commented Sep 27, 2023

Checking for specific log statements is somewhat messy. We can just check the task if content has been skipped or not.

@hstct
Copy link
Contributor

hstct commented Sep 27, 2023

In test_sync.py is a helper function that checks this provided a task and the correct report code.

Here we check if the release file and the package indices have not been skipped in the sync: https://github.com/pulp/pulp_deb/blob/main/pulp_deb/tests/functional/api/test_sync.py#L226-L227

@quba42 quba42 force-pushed the optimize_improvements branch from 85fbfc5 to 99b7220 Compare September 27, 2023 14:43
@quba42 quba42 marked this pull request as ready for review September 27, 2023 14:44
hstct
hstct reviewed Sep 28, 2023
View reviewed changes
pulp_deb/app/tasks/synchronizing.py
log.info(_("Falling back to optimize=False behaviour since mirror=True is set!"))
log.info(_("See https://github.com/pulp/pulp_deb/issues/631 for more information."))
self.optimize = False
self.sync_info["sync_options"]["optimize"] = False
Copy link
Contributor

Choose a reason for hiding this comment

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

This will likely run into the same edge case as above if we manage to get here on the repositories first sync.

Copy link
Collaborator Author

@quba42 quba42 Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This one should be fine, since this is self.sync_info and not previous_sync_info and self.sync_info was explicitly seeded with the field a few lines further up.

@quba42 quba42 force-pushed the optimize_improvements branch 2 times, most recently from ea6e7cf to 76a7e21 Compare October 2, 2023 09:54
@quba42
Improve optimize sync conditions
1854a42 
closes pulp#903

This PR achieves the following:

- Do not disable optimize if we are switching from mirror to not mirror
  between two syncs. We can optmize in this case.
- Log additional reasons why we did not optimize in certain cases.
- Improve the readability of optimize conditions.
@quba42 quba42 force-pushed the optimize_improvements branch from 76a7e21 to 1854a42 Compare October 2, 2023 11:28
@quba42 quba42 added .bugfix CHANGES/<issue_number>.bugfix backport-3.0 labels Oct 2, 2023
@hstct hstct merged commit 8ca8fbe into pulp:main Oct 4, 2023
@patchback
Copy link

patchback bot commented Oct 4, 2023 •
edited
Loading

Backport to 3.0: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.0/8ca8fbe4772dc7508fe2c7afe741cd89c7f3d8e5/pr-902

Backported as #908

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@hstct hstct deleted the optimize_improvements branch October 4, 2023 10:23
patchback bot pushed a commit that referenced this pull request Oct 4, 2023
@hstct @patchback
Merge pull request #902 from ATIX-AG/optimize_improvements
7534300 
Improve optimize sync conditions

(cherry picked from commit 8ca8fbe)
@patchback patchback bot mentioned this pull request Oct 4, 2023
Merged
hstct added a commit that referenced this pull request Oct 4, 2023
@hstct
Merge pull request #908 from pulp/patchback/backports/3.0/8ca8fbe4772…
b663395 
…dc7508fe2c7afe741cd89c7f3d8e5/pr-902

[PR #902/8ca8fbe4 backport][3.0] Improve optimize sync conditions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hstct hstct hstct approved these changes

Assignees
No one assigned
Labels
.bugfix CHANGES/<issue_number>.bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When switching from mirrored to not mirrored mode between syncs optimize sync is not used
2 participants
@quba42 @hstct