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.

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

Sync to happen inside working directory #1879

Merged
merged 1 commit into from Nov 11, 2020

Conversation

pavelpicka
Copy link
Contributor

@pavelpicka pavelpicka commented Nov 4, 2020

few downloads or testing remote remomd.xml file happened outside a
working directory

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

[nocoverage]
[notest]

@pulpbot
Copy link
Member

pulpbot commented Nov 4, 2020

Attached issue: https://pulp.plan.io/issues/7698

CHANGES/7698.bugfix Outdated Show resolved Hide resolved
Copy link
Member

@goosemania goosemania left a comment

Choose a reason for hiding this comment

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

I think the suggested approach would work.
My question is whether it's easier/better to have only one context manager at the very beginning of the sync task which will cover all the code and not adding it selectively in many places.
I automatically think that one is better, just because iI don't need to remember in which place what happens and if any chages to it, they are in one place. Thoughts?

@pavelpicka
Copy link
Contributor Author

It won't be easier as we can't call declarative_version.create inside context manager as it already uses one (https://github.com/pulp/pulpcore/blob/master/pulpcore/plugin/stages/declarative_version.py#L139). And in rpm sync we call declarative version multiple times as for main repo and many times if distribution trees are present.

@goosemania
Copy link
Member

Oh that's a pain. I wish we could do get_or_create here https://github.com/pulp/pulpcore/blob/772e56e3cf5583fef0768dcc81b5cd8adb4b7ceb/pulpcore/tasking/services/storage.py#L168.

Could we reuse existing working directory? or is it a security or data consistency concern because of which we recreate it every time? The desire here is to support nested context manager:

with WorkingDirectory():
    ...
    with WorkingDirectory():
        ...

@daviddavis , @dralley , @bmbouter

@goosemania
Copy link
Member

There needs to be an adjustment made in pulpcore. We discussed today to deprecate WorkingDirectory at all and move the working directory creation to the task management level. So for now I think we should merge it to fix the issue and after 3.10 we'll be able to remove this code.

@@ -0,0 +1 @@
Fixed downloads outside working directory during sync.
Copy link
Member

Choose a reason for hiding this comment

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

A typo (it should be outside of the) led me to a longer message to make it more user-friendly :)
What do you think?
Fixed the case when downloads were happening outside of the task working directory during sync.

few downloads or testing remote remomd.xml file happened outside a
working directory

[nocoverage]
[notest]

closes: #7698
https://pulp.plan.io/issues/7698
@goosemania goosemania merged commit 8616249 into pulp:master Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants