Introducing asyncio stages #3559

bmbouter · 2018-07-23T14:42:05Z

This PR pairs with pulp_file changes here: pulp/pulp_file#102

I use the test script below. It resets my Pulp3 installation, sync pulp_file, upload another file, associate with the repo, and then resync again with mirror='true'.

It's expected to make 3 versions, and Pulp should have 4 content units in it.
v1 - 3 content units matching the remote pulp_file repo
v2 - 4 content units, included the newly uploaded one
v3 - 3 content units, after the sync with mirror='true'. This verifies that mirror is working.

set -v 

# Reset everything

read -p "Stop the webserver, then press enter"

sudo systemctl stop pulp_resource_manager
sudo systemctl stop pulp_worker@1
sudo systemctl stop pulp_worker@2

pulp-manager reset_db --noinput
pulp-manager migrate
pulp-manager reset-admin-password --password admin
rm -rf /var/lib/pulp/artifact/*

sudo systemctl start pulp_resource_manager
sudo systemctl start pulp_worker@1
sudo systemctl start pulp_worker@2

read -p "Start the webserver, then press enter"

# Create a repository, 'foo'

http POST http://localhost:8000/pulp/api/v3/repositories/ name=foo

export REPO_HREF=$(http :8000/pulp/api/v3/repositories/ | jq -r '.results[] | select(.name == "foo") | ._href')

# Create a remote, 'bar'

http POST http://localhost:8000/pulp/api/v3/remotes/file/ name='bar' url='https://repos.fedorapeople.org/pulp/pulp/demo_repos/test_file_repo/PULP_MANIFEST'

export REMOTE_HREF=$(http :8000/pulp/api/v3/remotes/file/ | jq -r '.results[] | select(.name == "bar") | ._href')

# Sync 'foo' using 'bar'

http POST $REMOTE_HREF'sync/' repository=$REPO_HREF

# Upload an artifact

http --form POST http://localhost:8000/pulp/api/v3/artifacts/ file@/home/vagrant/devel/foo.tar.gz

export ARTIFACT_HREF=$(http http://localhost:8000/pulp/api/v3/artifacts/?sha256=2a123bec2a2e1df9cd8705861f61cb6df5453f7eaab424f1984658152211e4a5 | jq -r '.results[] | ._href')

http POST http://localhost:8000/pulp/api/v3/content/file/files/ relative_path=foo.tar.gz artifact="$ARTIFACT_HREF"

export CONTENT_HREF=$(http :8000/pulp/api/v3/content/file/files/ | jq -r '.results[] | select(.relative_path == "foo.tar.gz") | ._href')

http POST $REPO_HREF'versions/' add_content_units:="[\"$CONTENT_HREF\"]"

# Sync 'foo' using 'bar'

http POST $REMOTE_HREF'sync/' repository=$REPO_HREF

pep8speaks · 2018-07-23T14:42:09Z

Hello @bmbouter! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 03, 2018 at 20:21 Hours UTC

codecov · 2018-07-23T20:27:14Z

Codecov Report

Merging #3559 into master will decrease coverage by 0.19%.
The diff coverage is 14.28%.

@@            Coverage Diff            @@
##           master    #3559     +/-   ##
=========================================
- Coverage    58.2%   58.01%   -0.2%     
=========================================
  Files          59       60      +1     
  Lines        2467     2477     +10     
=========================================
+ Hits         1436     1437      +1     
- Misses       1031     1040      +9

Impacted Files	Coverage Δ
__init__.py	`0% <ø> (ø)`
pulpcore/pulpcore/app/models/progress.py	`51.78% <0%> (-2.94%)`	⬇️
pulpcore/pulpcore/app/models/content.py	`68.57% <50%> (-3.74%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fe4ca0...631031e. Read the comment docs.

jortel · 2018-07-25T17:08:32Z

pulpcore/pulpcore/app/models/content.py

@@ -138,6 +138,15 @@ def natural_key(self):
        """
        return tuple(getattr(self, f) for f in self.natural_key_fields())

+    def natural_key_dict(self):


This should be simplified to:

return {f: getattr(self, f) for f in self.natural_key_fields()}

This is simple enough for the caller to just do. I don't think this (1-line) method adds sufficient value to be added.

The use case I think about with this is plugin writers wanting to query for a unit they have in memory using filter() or Q(). There are two places I needed to do this for in DeclarativeVersion. I wanted to state it's purpose and see what you thought.

That is the use case I'd imaged as well so my opinion hasn't changed.

dkliban · 2018-07-26T14:47:15Z

plugin/pulpcore/plugin/stages/models.py

+    the `artifact` attributes may be incomplete because not all digest information can be computed
+    until the Artifact is downloaded.
+
+    Attributes:


This is causing the generated docs to not call these parameters, but variables instead. It renders weird:

I'm fixing this in the next push by using Sphinx's :no_members:

dkliban · 2018-07-26T17:34:24Z

plugin/pulpcore/plugin/stages/artifact_stages.py

+
+    >>> artifact_downloader(max_concurrent_downloads=42).stage  # This is the real stage
+
+    in_q data type: A `~pulpcore.plugin.stages.DeclarativeContent` with potentially files missing


The references to other docs are not resolving correctly in the built docs.

This problem is all over the docs in this PR. I'm going through and fixing all of them with the next push.

dkliban

Thank you!

https://pulp.plan.io/issues/3844 re pulp#3844

just about to refactor https://pulp.plan.io/issues/3844 re pulp#3844

https://pulp.plan.io/issues/3844 re pulp#3844

pulp#3539 https://pulp.plan.io/issues/3844 re pulp#3844

https://pulp.plan.io/issues/3844 re pulp#3844

jortel · 2018-08-01T17:08:18Z

plugin/pulpcore/plugin/stages/models.py

+        if not content:
+            raise ValueError(_("DeclarativeContent must have a 'content'"))
+        if d_artifacts:
+            self.d_artifacts = d_artifacts


Suggest:

self.d_artifacts = d_artifacts or []

would be simpler.

cool will include in next push.

jortel · 2018-08-01T17:11:31Z

plugin/pulpcore/plugin/stages/api.py

+            and defaults to 100.
+
+    Returns:
+        A single coroutine that can be used to run, wait, or cancel the entire pipeline with.


This probably should document it returns an asyncio.Future. (I think)

At this point it's only returning a coroutine. I looked into sphinx's support for indicating something is a coroutine, but they aren't there yet unfortunately. sphinx-doc/sphinx#4777

jortel · 2018-08-01T17:14:39Z

@bmbouter, Review finished. A few of the comments ended up being repetitive, sorry.

jortel

Most of the comments are not merge blockers but would improve code quality. However, a few comments regarding API and correctness need to be addressed.

https://pulp.plan.io/issues/3844 re pulp#3844

Mostly fixes class names for pep8 compliance https://pulp.plan.io/issues/3844 re pulp#3844

This is a big diff, but mainly due only to indention https://pulp.plan.io/issues/3844 re pulp#3844

bmbouter · 2018-08-02T21:46:30Z

@dralley @jortel Now that all stages inherit from BaseStage I think FirstStage should be deleted, and I'll refactor the docs to have plugin writer's make their first stage directly from BaseStage. Sound good?

bmbouter · 2018-08-02T21:47:54Z

@jortel This PR still needs 3 things I'll do tomorrow morning:

remove FirstStage and use BaseStage instead
Handle the artifact collision concern
Switch default_storage() to DefaultStorage()

I'll ping you when it's ready for final review, Friday AM.

https://pulp.plan.io/issues/3844 re pulp#3844

jortel · 2018-08-03T13:20:32Z

plugin/pulpcore/plugin/stages/api.py

+    await asyncio.gather(*futures)
+
+
+class EndStage(BaseStage):


Big 👍 for making the stages objects! This will unlock a lot of potential.

At the risk of being pedantic :(

Why not just Stage?

The rare cases I see the Base prefix, it strikes me as odd. The class name is a classification of objects and should read naturally using the IsA lexicon.

For example, the natural way to describe an QueryExistingArtifacts is to say:

"A QueryExistingArtifacts` IsA Stage".

not

"A QueryExistingArtifacts` IsA BaseStage".

I don't think designating this as a base class though naming adds value. This isn't a blocker but, IMHO, it would be more correct in the classic object modeling sense.

I think s/BaseStage/Stage/ is a great change. I'll make it on my next push along w/ the last remaining fixes.

jortel · 2018-08-03T13:23:43Z

plugin/pulpcore/plugin/stages/declarative_version.py

+from .content_unit_stages import ContentUnitSaver, QueryExistingContentUnits
+
+
+class FirstStage:


Why is FirstStage still needed?

It's not, but I was too tired to remove it last night. It'll be removed in next push.

jortel · 2018-08-03T13:28:37Z

plugin/pulpcore/plugin/stages/artifact_stages.py

+            run. Default is 100.
+    """
+
+    def __init__(self, max_concurrent_downloads=100, *args, **kwargs):


What are the *args, **kwargs for?

If needed, please docstring what can be passed.

It's not formally spelled out in Python, but *args and **kwargs should be accepted on all init() calls and used when calling super().init(*args, **kwargs). Otherwise the parent object can never receive its parameters through the subclass' init().

For places where *args and **kwargs are used, I'm going to add a docstring that it takes *args and **kwargs and it will say unused params passed on to Stage to stay DRY w/ the docstrings.

jortel · 2018-08-03T13:31:04Z

plugin/pulpcore/plugin/stages/artifact_stages.py

+
+    def __init__(self, max_concurrent_downloads=100, *args, **kwargs):
+        self.max_concurrent_downloads = max_concurrent_downloads
+        super().__init__(*args, **kwargs)


It's better practice to initialize objects in heirarchical order. Please call super().__init__() (first) before setting extended attributes.

ok will do in next push

jortel · 2018-08-03T13:37:53Z

plugin/pulpcore/plugin/stages/association_stages.py

+        for unit in self.new_version.content.all():
+            unit = unit.cast()
+            self.unit_keys_by_type[type(unit)].add(unit.natural_key())
+        super().__init__(*args, **kwargs)


Same re: calling super as: here
Same re: args, kwargs as: here

yup will fix in next push.

jortel · 2018-08-03T13:38:12Z

plugin/pulpcore/plugin/stages/association_stages.py

+
+    def __init__(self, new_version, *args, **kwargs):
+        self.new_version = new_version
+        super().__init__(*args, **kwargs)


Same re: calling super as: here
Same re: args, kwargs as: here

yup will fix in next push.

jortel · 2018-08-03T13:40:03Z

plugin/pulpcore/plugin/stages/content_unit_stages.py

+                                    'remote': declarative_artifact.remote,
+                                }
+                                rel_path = content_artifact.relative_path
+                                remote_artifact_map[rel_path] = remote_artifact_data


Still correlated by rel_path.

yup this and the storage backend both are being fixed in next push.

* switch storage backend to DefaultStorage() * super() is now called first in anytime it's used * FirstStage is removed * BaseStage -> Stage * the relative path now handles duplicate rel_paths correctly * documenting args and kwargs I did some final hand testing, and I also ensure the docs build and look good. https://pulp.plan.io/issues/3844 re pulp#3844

jortel · 2018-08-03T18:47:42Z

plugin/pulpcore/plugin/stages/artifact_stages.py

+                                    )
+                                    declarative_artifact.artifact = new_artifact
+                                    to_download_count = to_download_count + 1
+                            pb.done = pb.done + to_download_count


Just FYI: using += would be more concise.

jortel · 2018-08-03T18:53:39Z

plugin/pulpcore/plugin/stages/content_unit_stages.py

+                                    'remote': declarative_artifact.remote,
+                                }
+                                content_pk = content_artifact.content.pk
+                                remote_artifact_map[content_pk] = remote_artifact_data


After switching to integer PK, the content_artifact.content.pk will be (None) because the model has not yet been saved. Right?

This is rebased onto the integer PK already so it uses them currently. The pk is set from L#151.

I verified I cannot use the models themselves because they are unsaved and Django raises an unhashable error. We could use sha512 instead. The only reason I didn't is to save memory. Should I switch it to that?

Actually I'm going to switch it.

Without this commit, an exception will occur when one content unit has two or more Artifacts are associated with it. https://pulp.plan.io/issues/3844 re pulp#3844

bmbouter · 2018-08-03T20:38:21Z

Thank you @jortel , @dkliban, @dralley , and @gmbnomis for all the collaboration and help with this! 💯 I really think the plugin writers are going to get a big benefit from it.

I'm merging it and https://github.com/pulp/pulp_file/pull/102/files

Required PR: pulp/pulp#3559 https://pulp.plan.io/issues/3890 re #3890

bmbouter added Feature Work In Progress 3.0 labels Jul 23, 2018

bmbouter force-pushed the introducing-asyncio-stages branch 2 times, most recently from bb1c765 to 07739d5 Compare July 23, 2018 20:13

bmbouter force-pushed the introducing-asyncio-stages branch from 00b2ea7 to 54bc039 Compare July 24, 2018 14:25

bmbouter mentioned this pull request Jul 24, 2018

Introducing asyncio stages pulp/pulp_file#102

Merged

bmbouter force-pushed the introducing-asyncio-stages branch from 1a3b078 to b656b74 Compare July 25, 2018 16:01

jortel reviewed Jul 25, 2018

View reviewed changes

bmbouter force-pushed the introducing-asyncio-stages branch 3 times, most recently from 4b9d312 to 95c9bda Compare July 25, 2018 19:13

bmbouter removed the Work In Progress label Jul 25, 2018

dkliban reviewed Jul 26, 2018

View reviewed changes

dkliban approved these changes Jul 31, 2018

View reviewed changes

Brian Bouterse added 11 commits July 31, 2018 10:15

half-working

1434642

https://pulp.plan.io/issues/3844 re pulp#3844

need relative_path at later stage.

6990563

just about to refactor https://pulp.plan.io/issues/3844 re pulp#3844

Working, but not bulk_create

613f6b6

https://pulp.plan.io/issues/3844 re pulp#3844

bulk_create does not work

789e6f4

https://pulp.plan.io/issues/3844 re pulp#3844

Working, but doesn't remove on mirror

607e102

https://pulp.plan.io/issues/3844 re pulp#3844

Last Stage working

2d34c92

https://pulp.plan.io/issues/3844 re pulp#3844

Moving docs fixes to its own PR

fb03774

pulp#3539 https://pulp.plan.io/issues/3844 re pulp#3844

sync_mode works and new docstrings

03473b5

https://pulp.plan.io/issues/3844 re pulp#3844

Adds some docs, committing before code changes

110945d

https://pulp.plan.io/issues/3844 re pulp#3844

switched last stage to use QuerSet

6b0f0c6

https://pulp.plan.io/issues/3844 re pulp#3844

more docstrings

56842b8

https://pulp.plan.io/issues/3844 re pulp#3844

jortel reviewed Aug 1, 2018

View reviewed changes

jortel suggested changes Aug 1, 2018

View reviewed changes

Brian Bouterse added 3 commits August 2, 2018 14:51

Updating stages w/ some clearer names

082e1e4

https://pulp.plan.io/issues/3844 re pulp#3844

Various fixes from review

34c8536

Mostly fixes class names for pep8 compliance https://pulp.plan.io/issues/3844 re pulp#3844

All stages are classes now

bc6d425

This is a big diff, but mainly due only to indention https://pulp.plan.io/issues/3844 re pulp#3844

flake8 fix

856d090

https://pulp.plan.io/issues/3844 re pulp#3844

jortel reviewed Aug 3, 2018

View reviewed changes

bmbouter force-pushed the introducing-asyncio-stages branch from ab54599 to 579c700 Compare August 3, 2018 19:23

jortel approved these changes Aug 3, 2018

View reviewed changes

Fixing a bug where 2 artifacts are used

631031e

Without this commit, an exception will occur when one content unit has two or more Artifacts are associated with it. https://pulp.plan.io/issues/3844 re pulp#3844

bmbouter merged commit 890ceba into pulp:master Aug 3, 2018

bmbouter deleted the introducing-asyncio-stages branch August 3, 2018 20:38

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request May 14, 2019

flake8 fixes

2534fd4

Required PR: pulp/pulp#3559 https://pulp.plan.io/issues/3890 re #3890

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request May 14, 2019

small docstring fix

1b1624d

Required PR: pulp/pulp#3559 https://pulp.plan.io/issues/3890 re #3890

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request May 14, 2019

Convert to __call__() as coroutine interface

f93d858

Required PR: pulp/pulp#3559 https://pulp.plan.io/issues/3890 re #3890

daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request May 14, 2019

Inherit from Stage directly

b1866b4

Required PR: pulp/pulp#3559 https://pulp.plan.io/issues/3890 re #3890

Introducing asyncio stages #3559

Introducing asyncio stages #3559

bmbouter commented Jul 23, 2018 •

edited

pep8speaks commented Jul 23, 2018 •

edited

codecov bot commented Jul 23, 2018 •

edited

jortel Jul 25, 2018 •

edited

bmbouter Jul 25, 2018

jortel Jul 31, 2018

dkliban Jul 26, 2018

bmbouter Jul 30, 2018

dkliban Jul 26, 2018

bmbouter Jul 30, 2018 •

edited

dkliban left a comment

jortel Aug 1, 2018

bmbouter Aug 1, 2018

jortel Aug 1, 2018

bmbouter Aug 1, 2018

jortel commented Aug 1, 2018

jortel left a comment

bmbouter commented Aug 2, 2018

bmbouter commented Aug 2, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

jortel Aug 3, 2018

jortel Aug 3, 2018

bmbouter Aug 3, 2018

bmbouter Aug 3, 2018

bmbouter commented Aug 3, 2018


		>>> artifact_downloader(max_concurrent_downloads=42).stage # This is the real stage

		in_q data type: A `~pulpcore.plugin.stages.DeclarativeContent` with potentially files missing

		from .content_unit_stages import ContentUnitSaver, QueryExistingContentUnits


		class FirstStage:

Introducing asyncio stages #3559

Introducing asyncio stages #3559

Conversation

bmbouter commented Jul 23, 2018 • edited

pep8speaks commented Jul 23, 2018 • edited

Comment last updated on August 03, 2018 at 20:21 Hours UTC

codecov bot commented Jul 23, 2018 • edited

Codecov Report

jortel Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter Jul 30, 2018 • edited

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jortel commented Aug 1, 2018

jortel left a comment

Choose a reason for hiding this comment

bmbouter commented Aug 2, 2018

bmbouter commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Aug 3, 2018

bmbouter commented Jul 23, 2018 •

edited

pep8speaks commented Jul 23, 2018 •

edited

codecov bot commented Jul 23, 2018 •

edited

jortel Jul 25, 2018 •

edited

bmbouter Jul 30, 2018 •

edited