Adds 'policy' to Remote #3738

bmbouter · 2018-11-05T22:59:39Z

Required PR: pulp/pulp_file#132

This includes a serializer that validates input correctly.

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

pep8speaks · 2018-11-05T22:59:42Z

Hello @bmbouter! Thanks for updating the PR.

There are no PEP8 issues in the file plugin/pulpcore/plugin/download/base.py !
There are no PEP8 issues in the file plugin/pulpcore/plugin/download/file.py !
There are no PEP8 issues in the file plugin/pulpcore/plugin/download/http.py !
There are no PEP8 issues in the file plugin/pulpcore/plugin/models/remote.py !
There are no PEP8 issues in the file plugin/pulpcore/plugin/stages/artifact_stages.py !
There are no PEP8 issues in the file plugin/pulpcore/plugin/stages/declarative_version.py !
There are no PEP8 issues in the file pulpcore/pulpcore/app/models/repository.py !
There are no PEP8 issues in the file pulpcore/pulpcore/app/serializers/repository.py !
There are no PEP8 issues in the file pulpcore/pulpcore/constants.py !

Comment last updated on November 30, 2018 at 19:45 Hours UTC

daviddavis · 2018-11-06T00:17:44Z

pulpcore/pulpcore/app/models/repository.py

@@ -105,6 +107,7 @@ def tls_storage_path(self, name):
    password = models.TextField(blank=True)
    last_synced = models.DateTimeField(blank=True, null=True)
    connection_limit = models.PositiveIntegerField(default=20)
+    policy = models.TextField(choices=POLICY_CHOICES, default=POLICY_MODE.IMMEDIATE)


Is it possible for a plugin writer to override these choices and default somehow?

They mostly could right now by defining the policy attribute on the subclass with another set of choices. There is one issue in that the helptext is duplicated because our use of choices isn't correct afaict. This PR kind of does the same thing since that problem already exists w/ the TASK_STATES. Other than that though they could do that, and we should fix that too but I'm not sure how yet. I filed it as this issue here: https://pulp.plan.io/issues/4127

Note that these values affect the behavior of the streamer so if a plugin writer starts modifying the CHOICES values they are enabling/disabling features for the streamer. They need to be aware of that.

Actually I'm removing this default because it doesn't work w/ DRF.

plugin/pulpcore/plugin/stages/declarative_version.py

bmbouter · 2018-11-29T21:38:37Z

There is an issue with this because the 'policy' field is not optional. A default of 'immediate' is intended and is applied to the model layer here. I can't see how to set a default as a ChoiceField in the drf docs here.

When I create a remote without setting it I get:

{
    "policy": [
        "This field is required."
    ]
}

Or should this be a required field with no default?

codecov · 2018-11-29T22:02:31Z

Codecov Report

Merging #3738 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3738      +/-   ##
==========================================
+ Coverage   54.25%   54.34%   +0.09%     
==========================================
  Files          64       64              
  Lines        2835     2841       +6     
==========================================
+ Hits         1538     1544       +6     
  Misses       1297     1297

Impacted Files	Coverage Δ
pulpcore/pulpcore/constants.py	`100% <ø> (ø)`	⬆️
pulpcore/pulpcore/app/serializers/repository.py	`94.8% <100%> (+0.03%)`	⬆️
pulpcore/pulpcore/app/models/repository.py	`56.37% <100%> (+1.51%)`	⬆️

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 1dfca08...3daf3bf. Read the comment docs.

dralley · 2018-11-30T14:44:01Z

pulpcore/pulpcore/constants.py

+    CACHE_ONLY='cache_only',
+)
+
+POLICY_CHOICES = (


Perhaps we should also have these descriptions in the docs somewhere?

Yes I agree, and an explanation of the whole lazy thing in general. Maybe I should go ahead and write this page? https://docs.pulpproject.org/en/3.0/nightly/workflows/deferred-download.html

What about also me renaming that page to "Lazy Download"? Deferred Download just sounds so formal to me.

@asmacdo what do you think? ^

Sounds good to me. The only hesitation I have is if this workflow differs plugin to plugin. For now, I think what you propose makes sense, if we get variation, we can move those docs to the plugin_template and each plugin.

@asmacdo I wrote this story which I can also do today. https://pulp.plan.io/issues/4218

Can you help me groom that story? Then I can write the docs in another PR very soon.

I'm taking the other PR as assigned so that we can merge this one. Thanks for grooming it @asmacdo

gmbnomis · 2018-11-30T14:59:29Z

plugin/pulpcore/plugin/stages/declarative_version.py

-        ]
+        pipeline = [self.first_stage]
+        if self.download_artifacts:
+            pipeline.extend([QueryExistingArtifacts(), ArtifactDownloader(), ArtifactSaver()])


Wondering whether we should still have QueryExistingArtifacts() in the non-download case. I think this makes sense for on demand, we might not want to do it for cache only(?).

From a correctness interpretation of the name download_artifacts not including QueryExistingArtifacts is probably wrong. Query+Replace of Artifacts in the pipeline is not the same as downloading. So I see what you are saying.

My concern with adding it is the extra runtime cost; from our testing AIUI that stage adds a non-trivial amount of extra time to the overall runtime from the DB querying when processing say 70K Artifacts.

I think we should either do 1 of a few things.

a) Add back QueryExistingArtifacts and accept the runtime outcome
b) s/download_artifacts/handle_artifacts/ so that the name is more correct.
c) another idea

Is there a way to find out later that downloading a remote artifact is not necessary? (e.g. when the artifact is requested; I don't know the full picture) Then, not replacing artifacts here would not hurt too much.

I'm leaning toward leaving it the way it is. When artifacts are not downloaded they should not be query/replaced in the pending-artifact either. Mainly because the replacement isn't deterministic (may or may not be replaced) so logic further down the pipeline should not rely on it. Leaving QueryExistingArtifacts in the pipeline without the downloading stage seems to add overhead without adding any value.

As for the renaming (b), I'm not convinced it add clarity. I'm not a fan of names that include "handle" because it's such a vague term.

I agree the handle name is bad. What about include_artifact_stages=True and then it would include/disinclude all 3 of them?

What if the core, or a plugin includes an artifact related stage (in the future) that needs to be left in the pipeline even when artifacts are not downloaded? This naming assumes that will never happen.

The name download_artifacts is toggling a specific behavior that has the side-effect of omitting some of the stages. The renaming to include_artifact_stages changes the toggled behavior to something that has the side-effect of toggling downloading which seems less clear. I think.

Sounds good. I'm leaving the 3 stages being omitted and the name as is.

plugin/pulpcore/plugin/models/remote.py

jortel · 2018-11-30T15:25:57Z

plugin/pulpcore/plugin/stages/declarative_version.py

@@ -11,7 +11,7 @@

 class DeclarativeVersion:

-    def __init__(self, first_stage, repository, mirror=True):
+    def __init__(self, first_stage, repository, mirror=True, download_artifacts=True):


👍 passing (boolean) download_artifacts instead of sync policy name.

pulpcore/pulpcore/constants.py

jortel

Thanks for the PR @bmbouter.

bmbouter · 2018-11-30T16:21:23Z

@jortel I pushed those changes.

Required PR: pulp/pulp_file#132 This includes a serializer that validates input correctly. This also switches the finalize, write_data, and handle_headers callbacks to be coroutines themselves. This is required because within those callbacks the streamer needs to await on coroutines and that can only be done from withing a coroutine itself. This also adjusts the Remote.get_downloader() method signature to take `url` or `remote_artifact` as kwargs instead of `url` as a positional parameter. This allows plugin writers (and the streamer) to get downloaders configured with digest and size validation automatically from remote_artifacts. https://pulp.plan.io/issues/3763 closes pulp#3763

Required PR: pulp/pulp#3738 pulp_file now meaningfully uses the 'policy' attribute when syncing content. This is meant to be used with this PR: pulp/pulp#3738 https://pulp.plan.io/issues/4160 closes #4160

bmbouter added Feature lazy Work In Progress 3.0 rest-api labels Nov 5, 2018

bmbouter force-pushed the add-policy-attribute branch from ff2fd97 to 2ba95be Compare November 6, 2018 00:11

daviddavis reviewed Nov 6, 2018

View reviewed changes

bmbouter force-pushed the add-policy-attribute branch from 2ba95be to 30f8e84 Compare November 6, 2018 23:05

bmbouter force-pushed the add-policy-attribute branch from 30f8e84 to 65f79c8 Compare November 14, 2018 19:34

bmbouter mentioned this pull request Nov 14, 2018

Adds lazy download support pulp/pulp_file#132

Merged

bmbouter force-pushed the add-policy-attribute branch 2 times, most recently from f50d4f9 to beeb666 Compare November 14, 2018 20:00

gmbnomis reviewed Nov 14, 2018

View reviewed changes

plugin/pulpcore/plugin/stages/declarative_version.py Outdated Show resolved Hide resolved

bmbouter force-pushed the add-policy-attribute branch from beeb666 to 7aa2048 Compare November 14, 2018 21:11

bmbouter removed the Work In Progress label Nov 19, 2018

bmbouter force-pushed the add-policy-attribute branch 2 times, most recently from bd06557 to d0d5ede Compare November 20, 2018 21:40

bmbouter force-pushed the add-policy-attribute branch from d0d5ede to 88fedc9 Compare November 27, 2018 21:32

bmbouter force-pushed the add-policy-attribute branch from 88fedc9 to c4f201d Compare November 29, 2018 21:54

dralley reviewed Nov 30, 2018

View reviewed changes

gmbnomis reviewed Nov 30, 2018

View reviewed changes

plugin/pulpcore/plugin/models/remote.py Show resolved Hide resolved

jortel reviewed Nov 30, 2018

View reviewed changes

bmbouter added the Issue label Nov 30, 2018

pulp deleted a comment from bmbouter Nov 30, 2018

jortel reviewed Nov 30, 2018

View reviewed changes

pulpcore/pulpcore/constants.py Outdated Show resolved Hide resolved

jortel reviewed Nov 30, 2018

View reviewed changes

pulpcore/pulpcore/constants.py Outdated Show resolved Hide resolved

jortel suggested changes Nov 30, 2018

View reviewed changes

bmbouter force-pushed the add-policy-attribute branch from c4f201d to 16c8526 Compare November 30, 2018 16:20

jortel approved these changes Nov 30, 2018

View reviewed changes

bmbouter force-pushed the add-policy-attribute branch from 16c8526 to e39db91 Compare November 30, 2018 19:42

bmbouter force-pushed the add-policy-attribute branch from e39db91 to 3daf3bf Compare November 30, 2018 19:45

bmbouter merged commit e4488d0 into pulp:master Nov 30, 2018

bmbouter deleted the add-policy-attribute branch November 30, 2018 20:52

Adds 'policy' to Remote #3738

Adds 'policy' to Remote #3738

bmbouter commented Nov 5, 2018 •

edited

pep8speaks commented Nov 5, 2018 •

edited

daviddavis Nov 6, 2018 •

edited

bmbouter Nov 6, 2018

bmbouter Nov 29, 2018

bmbouter commented Nov 29, 2018

codecov bot commented Nov 29, 2018 •

edited

dralley Nov 30, 2018

bmbouter Nov 30, 2018

asmacdo Nov 30, 2018

bmbouter Nov 30, 2018

bmbouter Nov 30, 2018

gmbnomis Nov 30, 2018

bmbouter Nov 30, 2018

gmbnomis Nov 30, 2018 •

edited

jortel Nov 30, 2018

jortel Nov 30, 2018

bmbouter Nov 30, 2018

jortel Nov 30, 2018

bmbouter Nov 30, 2018

jortel Nov 30, 2018

jortel left a comment

bmbouter commented Nov 30, 2018

Adds 'policy' to Remote #3738

Adds 'policy' to Remote #3738

Conversation

bmbouter commented Nov 5, 2018 • edited

pep8speaks commented Nov 5, 2018 • edited

Comment last updated on November 30, 2018 at 19:45 Hours UTC

daviddavis Nov 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Nov 29, 2018

codecov bot commented Nov 29, 2018 • edited

Codecov Report

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

gmbnomis Nov 30, 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

Choose a reason for hiding this comment

jortel left a comment

Choose a reason for hiding this comment

bmbouter commented Nov 30, 2018

bmbouter commented Nov 5, 2018 •

edited

pep8speaks commented Nov 5, 2018 •

edited

daviddavis Nov 6, 2018 •

edited

codecov bot commented Nov 29, 2018 •

edited

gmbnomis Nov 30, 2018 •

edited