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

Acs refresh #536

Merged
merged 1 commit into from
Sep 16, 2021
Merged

Acs refresh #536

merged 1 commit into from
Sep 16, 2021

Conversation

pavelpicka
Copy link
Collaborator

@pavelpicka pavelpicka commented Jul 16, 2021 •
edited
Loading

Added refresh endpoint to support ACS.

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

Required PR: pulp/pulpcore#1496

@pavelpicka pavelpicka marked this pull request as draft July 16, 2021 14:16
@pavelpicka pavelpicka marked this pull request as ready for review September 1, 2021 12:48
@pavelpicka pavelpicka force-pushed the acs-refresh branch 3 times, most recently from ea09754 to 9554571 Compare September 2, 2021 12:06
daviddavis
daviddavis reviewed Sep 2, 2021
View reviewed changes
CHANGES/9097.feature Outdated Show resolved Hide resolved
bmbouter
bmbouter reviewed Sep 8, 2021
View reviewed changes
pulp_file/app/tasks/acs.py Outdated
acs_path.repository = repo
acs_path.save()
acs_url = os.path.join(acs.remote.url, acs_path.path)
synchronize(acs.remote.pk, repo.pk, False, acs_url)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of calling synchronize directly here dispatch this task instead: https://github.com/pulp/pulp_file/blob/main/pulp_file/app/tasks/synchronizing.py#L27 You can do that similarly to how the sync viewset does: https://github.com/pulp/pulp_file/blob/main/pulp_file/app/viewsets.py#L98-L107

The main difference is that you want all of your tasks to be a task group. You should create a TaskGroup and then set it on each task dispatched https://github.com/pulp/pulpcore/blob/24be668caed280eeda7413a563cdd612d423441c/pulpcore/tasking/tasks.py#L45

Here's an example of TaskGroup being created https://github.com/pulp/pulpcore/blob/de67d41c39d9d4bde2e8f86e4bf3f51d6cc774e7/pulpcore/app/tasks/importer.py#L376 Also list it as a CreatedResource https://github.com/pulp/pulpcore/blob/de67d41c39d9d4bde2e8f86e4bf3f51d6cc774e7/pulpcore/app/tasks/importer.py#L379 Then set the TaskGroup on the tasks dispatched.

I'd recommend not doing any GroupProgressReport for now, and instead we can add that in after all ^ is done.

Post any questions you have please. Thanks!

bmbouter
bmbouter reviewed Sep 8, 2021
View reviewed changes
docs/workflows/alternate-content-source.rst

.. code-block:: bash

http POST localhost:24817/pulp/api/v3/remotes/file/file/ name="remoteForACS" policy="on_demand" url="http://fixtures.pulpproject.org/"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use pulp CLI commands instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

The CLI commands haven’t been merged yet so they’re subject to changing. I’d be happy to update this in a follow up PR.

bmbouter
bmbouter reviewed Sep 8, 2021
View reviewed changes
pulp_file/app/tasks/acs.py Outdated Show resolved Hide resolved
bmbouter
bmbouter reviewed Sep 8, 2021
View reviewed changes
pulp_file/app/tasks/synchronizing.py

"""
super().__init__()
self.remote = remote
self.url = url if url else remote.url
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Maybe moving this to __init__ would make it more explicit (because it happens earlier)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which __init__ do you mean? I believe this is first place where it is needed to be checked.

Copy link
Member

Choose a reason for hiding this comment

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

My comment didn't make sense. I was meant to ask if it made sense to move this to synchronize() because then the value of url would be more concrete earlier in the call stack. Just a thought. Do what you think makes sense.

@pavelpicka pavelpicka force-pushed the acs-refresh branch 3 times, most recently from 9674b61 to 61be30c Compare September 10, 2021 13:47
@pulp pulp deleted a comment from pulpbot Sep 10, 2021
@pulpbot
Copy link
Member

pulpbot commented Sep 10, 2021

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

ipanova
ipanova reviewed Sep 10, 2021
View reviewed changes
pulp_file/app/viewsets.py Outdated Show resolved Hide resolved
bmbouter
bmbouter reviewed Sep 10, 2021
View reviewed changes
pulp_file/app/tasks/acs.py Outdated
acs_path.save()
acs_url = os.path.join(acs.remote.url, acs_path.path)
# Dispatch each ACS path to own task and assign it to common TaskGroup
task_group = TaskGroup.objects.create(description=f"Refresh of {acs_path.path}")
Copy link
Member

Choose a reason for hiding this comment

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

We won't be able to move the TaskGroup creation to the viewset until we implement this in pulpcore. Let's continue to keep the TaskGroup creation in this task for now until we can do that and then we'll refactor it. I plan to discuss this change at the pulpcore meeting next Tuesday.

Copy link
Member

Choose a reason for hiding this comment

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

I made this https://github.com/pulp/pulpcore/pull/1618/files

Copy link
Member

Choose a reason for hiding this comment

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

@pavelpicka this is now available for use in pulpcore pulp/pulpcore#1618

So I think move the TaskGroup creation to the viewset and return it to the user using the new Response type in pulpcore. Then have all tasks (including this one and its subtasks) be members of the task_group.

@pep8speaks
Copy link

pep8speaks commented Sep 13, 2021 •
edited
Loading

Hello @pavelpicka! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-09-15 12:29:01 UTC

bmbouter
bmbouter reviewed Sep 13, 2021
View reviewed changes
pulp_file/app/viewsets.py Outdated Show resolved Hide resolved
bmbouter
bmbouter reviewed Sep 13, 2021
View reviewed changes
pulp_file/app/viewsets.py Outdated Show resolved Hide resolved
bmbouter
bmbouter reviewed Sep 13, 2021
View reviewed changes
pulp_file/app/viewsets.py Outdated Show resolved Hide resolved
@pavelpicka pavelpicka force-pushed the acs-refresh branch 2 times, most recently from 4db4530 to 6ae5f5a Compare September 14, 2021 14:08
ipanova
ipanova reviewed Sep 14, 2021
View reviewed changes
CHANGES/9377.feature Outdated Show resolved Hide resolved
@pavelpicka pavelpicka force-pushed the acs-refresh branch 2 times, most recently from 38af16e to 79a4ba6 Compare September 14, 2021 15:37
ipanova
ipanova reviewed Sep 14, 2021
View reviewed changes
pulp_file/app/viewsets.py Outdated Show resolved Hide resolved
@ipanova
Copy link
Member

ipanova commented Sep 14, 2021

The code changes look good to me 👍

daviddavis
daviddavis reviewed Sep 14, 2021
View reviewed changes
pulp_file/app/viewsets.py Outdated
if created:
acs_path.repository = repo
acs_path.save()
acs_url = os.path.join(acs.remote.url, acs_path.path)
Copy link
Contributor

@daviddavis daviddavis Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should probably check if path is blank or not. I'm imagining that if you don't set paths for the ACS, it'll just use the remote url (e.g. http://fixtures.pulpproject.org/PULP_MANIFEST) but what it does instead is it appends a slash (http://fixtures.pulpproject.org/PULP_MANIFEST/).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds like good idea to add tests for no-path ACS and ACS with more paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, will do 👍

@pavelpicka pavelpicka force-pushed the acs-refresh branch 2 times, most recently from 8b2c7d3 to 84e22ee Compare September 15, 2021 12:27
@pavelpicka
ACS refresh
d06a0d0 
Added `refresh` endpoint to support ACS.

closes: #9377
https://pulp.plan.io/issues/9377
@daviddavis daviddavis mentioned this pull request Sep 16, 2021
Merged
@daviddavis daviddavis merged commit 4a126c2 into pulp:main Sep 16, 2021
@daviddavis daviddavis mentioned this pull request Sep 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmbouter bmbouter bmbouter left review comments

@ipanova ipanova ipanova left review comments

@daviddavis daviddavis daviddavis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@pavelpicka @pulpbot @pep8speaks @ipanova @daviddavis @bmbouter