Project

Profile

Help

Story #3802

[Epic] As a user, I can override a remote's settings when making a request to start a sync

Added by ttereshc over 1 year ago. Updated 5 months ago.

Status:
NEW
Priority:
Normal
Assignee:
-
Category:
-
Sprint/Milestone:
-
Start date:
Due date:
% Done:

0%

Platform Release:
Blocks Release:
Backwards Incompatible:
No
Groomed:
Yes
Sprint Candidate:
No
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:

Description

Problem

Users performing a sync, e.g. Katello want to provide one-time options to override a Remote's configuration for that one sync.

Design

Core Changes

1. Have the Remote's options be added to the RepositoryURLSerializer via inheritnace.
2. Override RemoteViewSet.get_object() in core so that anytime a plugin calls it, they will receive the remote with the options already overlaid. This will log at the debug level about options that are overlaid. It's important not to log the 'password' or 'cert_key' field values for security reasons. Also the overlay needs to work generically so when plugin writers try to overlay additional, custom Remote attributes it works.
3. Have RemoteViewSet.get_object() remove the in-memory Remote's save() method. It's unsafe for this in-memory object to be accidentally saved by a plugin writer.
4. Add docs on this feature and ensure it's clear that these settings won't be saved and applied to requests that occur later via policy='streamed' or policy='on_demand'.

Plugin Changes

1. Adjust the signature of your sync task to replace remote_pk with remote and have the in-memory version passed.

Plugins can additionally have the override include custom Remote fields by subclassing RepositoryURLSerializer similar to how RepositoryURLSerializer subclassed RemoteSerializer in core. Additionally the plugin's view will specify their subclassed serializer. It's important the get_object() method work generically so the plugin writer does not have to take an additional step to overlay custom attributes.


Subtasks

RPM Support - Story #4413: As a user, I can specify one-time options during syncNEW

Actions
Ansible Plugin - Story #5038: As a user, I can submit one-time Remote options with syncNEW

Actions
Debian Support - Story #5039: As a user, I can submit one-time Remote options with syncNEW

Actions
Container Support - Story #5040: As a user, I can submit one-time Remote options with syncNEW

Actions
File Support - Story #5041: As a user, I can submit one-time Remote options with syncNEW

Actions
Python Support - Story #5042: As a user, I can submit one-time Remote options with syncNEW

Actions
Maven Plugin - Story #5043: As a user, I can submit one-time Remote options with syncNEW

Actions
Story #5044: As a plugin writer, I have docs on how to have custom Remote fields be usable for 1-time optionsNEW

Actions

History

#1 Updated by ttereshc over 1 year ago

  • Description updated (diff)

#2 Updated by ttereshc over 1 year ago

  • Subject changed from As a user, I can provide an SSL certificate with a request for syncing a remote to As a user, I can override a remote's settings when making a request to start a sync
  • Description updated (diff)

#3 Updated by jsherril@redhat.com over 1 year ago

  • Tags Katello-P1 added
  • Tags deleted (Pulp 3)

#4 Updated by jsherril@redhat.com over 1 year ago

  • Tags Pulp 3 added

#5 Updated by daviddavis over 1 year ago

  • Groomed changed from No to Yes
  • Sprint Candidate changed from No to Yes

#6 Updated by daviddavis over 1 year ago

  • Sprint/Milestone set to 3.0

#7 Updated by daviddavis about 1 year ago

  • Groomed changed from Yes to No
  • Sprint Candidate changed from Yes to No

#8 Updated by daviddavis about 1 year ago

  • Description updated (diff)

#10 Updated by daviddavis about 1 year ago

  • Description updated (diff)

#13 Updated by dkliban@redhat.com about 1 year ago

  • Groomed changed from No to Yes
  • Sprint Candidate changed from No to Yes
  • Sprint set to Sprint 44

#14 Updated by daviddavis about 1 year ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to daviddavis

#15 Updated by jortel@redhat.com about 1 year ago

I have some concerns about how this story is written and don't think it should be groomed. It requires all of the plugins to implement overriding these standard settings (which is undesirable by itself). This currently cannot be done by any plugin using the stages API. Mainly because the artifact-download stage uses Remote.get_downloader() to get a downloader that is configured based on the Remote's attributes using the downloader factory. There (currently) isn't a way for the plugin to inject the overridden settings into this flow short of every plugin writer stashing the overridden settings on the concrete Remote, overriding get_downloader(). I really hope that's not the plan.
I think this needs further design.

Also, what are the expectations around use cases when artifact downloading is deferred (Lazy)? This will only work for immediate artifact downloading. Does it really have value?

#16 Updated by daviddavis about 1 year ago

I have some concerns about how this story is written and don't think it should be groomed. It requires all of the plugins to implement overriding these standard settings (which is undesirable by itself). This currently cannot be done by any plugin using the stages API. Mainly because the artifact-download stage uses Remote.get_downloader() to get a downloader that is configured based on the Remote's attributes using the downloader factory. There (currently) isn't a way for the plugin to inject the overridden settings into this flow short of every plugin writer stashing the overridden settings on the concrete Remote, overriding get_downloader(). I really hope that's not the plan.
I think this needs further design.

The plan is to pass the override parameters in via get_downloader() to the download factory and have the download factory store and instantiate the aiohttp session using those parameters (or otherwise fall back to the settings on Remote).

Also, what are the expectations around use cases when artifact downloading is deferred (Lazy)? This will only work for immediate artifact downloading. Does it really have value?

I expect that the parameters won't be used for lazy syncing. Without lazy syncing being implemented though, it's hard to say. I can take a look at Pulp 2 though to see how we handle this there.

#17 Updated by jortel@redhat.com about 1 year ago

daviddavis wrote:

The plan is to pass the override parameters in via get_downloader() to the download factory and have the download factory store and instantiate the aiohttp session using those parameters (or otherwise fall back to the settings on Remote).

This is done by the artifact-download stage. How will that stage know about the settings passed to the plugin?

#18 Updated by daviddavis about 1 year ago

  • Status changed from ASSIGNED to NEW
  • Assignee deleted (daviddavis)
  • Groomed changed from Yes to No
  • Sprint Candidate changed from Yes to No
  • Sprint deleted (Sprint 44)

I talked to @jsherrill about this and they use it to set a few options that aren't currently available0 in pulp 3. Moreover the options are only relevant to pulp_rpm so we agreed to set this to a P2 instead of a P1.

Agreed that this needs more design work. I had only considered the FirstStage and not the artifact download stage. We'll need a way to pass these options on.

Removing from sprint and ungrooming.

[0] https://git.io/fxzXY

#19 Updated by daviddavis about 1 year ago

  • Tags Katello-P2 added
  • Tags deleted (Katello-P1)

#20 Updated by daviddavis about 1 year ago

  • Sprint/Milestone deleted (3.0)

#21 Updated by daviddavis 8 months ago

  • Sprint/Milestone set to 3.0

#22 Updated by bmbouter 7 months ago

  • Tags deleted (Pulp 3)

#23 Updated by daviddavis 6 months ago

  • Blocks Story #4413: As a user, I can specify one-time options during sync added

#24 Updated by bmbouter 5 months ago

I looked at this a lot yesterday, and I'm ready to write more into the plan. Here are is a roundup of the changes.

1. One issue is that task code re-looks up the Remote again. In terms of an approach, I think it's easiest to "overlay" the in-memory Remote in the viewset and then pass that in-memory as an RQ parameter to the task instead of remote_pk. This goes against the general idea of not passing around large objects, but in this case, since the object must always live in memory I think it's ok. If we passed the parameters we would have to re-assemble them again in the task and that seems like more, unecessary work.

2. Which fields? We should allow any field to be overridden for the 1-time options instead of adding specific ones.

3. How to add to the serializer? We need to have all the fields represented in the single RepositorySyncURLSerializer serializer for the task because that is how the options will be shown to the user in the openAPI schema. Yet we don't want to repeat the serialization definition already defined for a Remote. We should use inheritance to cause the fields from the Remote to become part of the RepositorySyncURLSerializer.

4. How to perform the merge? I think the easiest place is to override the get_object() here so the returned, in-memory Remote is already overridden.

#25 Updated by bmbouter 5 months ago

  • Description updated (diff)

#26 Updated by bmbouter 5 months ago

  • Blocks deleted (Story #4413: As a user, I can specify one-time options during sync)

#27 Updated by bmbouter 5 months ago

  • Subject changed from As a user, I can override a remote's settings when making a request to start a sync to [Epic] As a user, I can override a remote's settings when making a request to start a sync

#28 Updated by jsherril@redhat.com 5 months ago

Our minimum requirements to get parity with what we use in pulp2, is to provide a temporary override for the feed url. This allows a user to import content from a disconnected iso and then start syncing from the upstream cdn.

This would only be done with the immediate download policy.

#29 Updated by bmbouter 5 months ago

  • Description updated (diff)

#30 Updated by ttereshc 5 months ago

  • Groomed changed from No to Yes

#31 Updated by ttereshc 5 months ago

  • Sprint set to Sprint 55

#32 Updated by jsherril@redhat.com 5 months ago

  • Tags Katello-P3 added
  • Tags deleted (Katello-P2)

#33 Updated by bmbouter 5 months ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to bmbouter

#34 Updated by dkliban@redhat.com 5 months ago

  • Sprint changed from Sprint 55 to Sprint 56

#35 Updated by jsherril@redhat.com 5 months ago

  • Tags deleted (Katello-P3)

With the current design of import/export, katello no longer needs one time sync options.

#36 Updated by bmbouter 5 months ago

  • Status changed from ASSIGNED to NEW
  • Assignee deleted (bmbouter)
  • Sprint/Milestone deleted (3.0)
  • Sprint deleted (Sprint 56)

This work is being de-prioritized out of 3.0 due to no stakeholder or user still requesting it.

Here's the diff of the pulpcore branch code that was partially working in case this gets developed in the future:

commit 33ae16fb24be8abd809db303ac8b6c9ec0a99c39
Author: Brian Bouterse <bbouters@redhat.com>
Date:   Mon Jul 8 16:59:21 2019 -0400

    Add one-time remote options at sync time

    The sync serializer now accepts Remote attributes to override the Remote
    attributres saved on the Remote for syncing. Plugins will need to enable
    this functionality to use it.

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

diff --git a/CHANGES/3802.feature b/CHANGES/3802.feature
new file mode 100644
index 000000000..eded72d6c
--- /dev/null
+++ b/CHANGES/3802.feature
@@ -0,0 +1,3 @@
+Enable plugins writers to add one-time remote options at sync time. If a plugin is compatible, the
+sync call will accept Remote attributes, e.g. ``url`` or ``download_concurrency`` and override the
+Remote's saved options which remain unchanged.
diff --git a/pulpcore/app/serializers/repository.py b/pulpcore/app/serializers/repository.py
index 1492f4e47..6e1719378 100644
--- a/pulpcore/app/serializers/repository.py
+++ b/pulpcore/app/serializers/repository.py
@@ -42,12 +42,12 @@ class RepositorySerializer(ModelSerializer):
                                                 'description')

-class RemoteSerializer(MasterModelSerializer):
+class RemoteSerializerMixin:
     """ 
-    Every remote defined by a plugin should have a Remote serializer that inherits from this
-    class. Please import from `pulpcore.plugin.serializers` rather than from this module directly.
+    A mixin that defines all of the fields used by the RemoteSerializer.
+
+    This allows reuse with the RepositorySyncURLSerializer.
     """ 
-    _href = DetailIdentityField()
     name = serializers.CharField(
         help_text=_('A unique name for this remote.'),
         validators=[UniqueValidator(queryset=models.Remote.objects.all())],
@@ -114,6 +114,14 @@ class RemoteSerializer(MasterModelSerializer):
         default=models.Remote.IMMEDIATE
     )

+
+class RemoteSerializer(MasterModelSerializer, RemoteSerializerMixin):
+    """ 
+    Every remote defined by a plugin should have a Remote serializer that inherits from this
+    class. Please import from `pulpcore.plugin.serializers` rather than from this module directly.
+    """ 
+    _href = DetailIdentityField()
+
     class Meta:
         abstract = True
         model = models.Remote
@@ -124,7 +132,7 @@ class RemoteSerializer(MasterModelSerializer):
         )

-class RepositorySyncURLSerializer(serializers.Serializer):
+class RepositorySyncURLSerializer(serializers.Serializer, RemoteSerializerMixin):
     repository = serializers.HyperlinkedRelatedField(
         required=True,
         help_text=_('A URI of the repository to be synchronized.'),
@@ -143,6 +151,13 @@ class RepositorySyncURLSerializer(serializers.Serializer):
                     'the remote repository. If ``False``, sync will be additive only.')
     )

+    class Meta:
+        fields = (
+            'name', 'url', 'ssl_ca_certificate', 'ssl_client_certificate', 'ssl_client_key',
+            'ssl_validation', 'proxy_url', 'username', 'password', '_last_updated',
+            'download_concurrency', 'policy'
+        ) + ('repository', 'mirror')
+

 class PublisherSerializer(MasterModelSerializer):
     """ 
diff --git a/pulpcore/app/viewsets/repository.py b/pulpcore/app/viewsets/repository.py
index 4d81d0a63..f37fc8376 100644
--- a/pulpcore/app/viewsets/repository.py
+++ b/pulpcore/app/viewsets/repository.py
@@ -293,6 +293,19 @@ class RemoteViewSet(NamedModelViewSet,
     queryset = Remote.objects.all()
     filterset_class = RemoteFilter

+    def get_remote_with_one_time_options(self):
+        """ 
+        Returns the detail remote overridden with the one-time options.
+        """ 
+        import pydevd_pycharm
+        pydevd_pycharm.settrace('localhost', port=29438, stdoutToServer=True, stderrToServer=True)
+        remote = super().get_object()
+        serializer = self.get_serializer(remote, data=self.request.data)
+        serializer.is_valid(raise_exception=True)
+        print(serializer.validated_data)
+        return remote
+
+

 class PublisherFilter(BaseFilterSet):
     """ 

Please register to edit this issue

Also available in: Atom PDF