Added fields controlling cnx-timeouts to Remote.

closes #7201

CHANGES/7201.feature

@@ -0,0 +1,5 @@
+Exposed ``aiohttp.ClientTimeout`` fields in ``Remote`` as ``connect_timeout``,
+``sock_connect_timeout``, ``sock_read_timeout``, and ``total_timeout``.
+
+This replaces the previous hard-coded 600 second timeout for sock_connect and sock_read,
+giving per-``Remote`` control of all four ``ClientTimeout`` fields to the user.

pulpcore/app/migrations/0051_timeoutfields.py

@@ -0,0 +1,34 @@
+# Generated by Django 2.2.17 on 2020-11-12 19:38
+
+import django.core.validators
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0050_namespace_access_policies'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='remote',
+            name='connect_timeout',
+            field=models.FloatField(null=True, validators=[django.core.validators.MinValueValidator(0.0, 'Timeout must be >= 0')]),
+        ),
+        migrations.AddField(
+            model_name='remote',
+            name='sock_connect_timeout',
+            field=models.FloatField(null=True, validators=[django.core.validators.MinValueValidator(0.0, 'Timeout must be >= 0')]),
+        ),
+        migrations.AddField(
+            model_name='remote',
+            name='sock_read_timeout',
+            field=models.FloatField(null=True, validators=[django.core.validators.MinValueValidator(0.0, 'Timeout must be >= 0')]),
+        ),
+        migrations.AddField(
+            model_name='remote',
+            name='total_timeout',
+            field=models.FloatField(null=True, validators=[django.core.validators.MinValueValidator(0.0, 'Timeout must be >= 0')]),
+        ),
+    ]

pulpcore/app/models/repository.py

@@ -7,6 +7,7 @@
 import logging
 
 import django
+from django.core.validators import MinValueValidator
 from django.db import models, transaction
 from django.urls import reverse
 
@@ -208,6 +209,10 @@ class Remote(MasterModel):
         download_concurrency (models.PositiveIntegerField): Total number of
             simultaneous connections.
         policy (models.TextField): The policy to use when downloading content.
+        total_timeout (models.FloatField): Value for aiohttp.ClientTimeout.total on connections
+        connect_timeout (models.FloatField): Value for aiohttp.ClientTimeout.connect
+        sock_connect_timeout (models.FloatField): Value for aiohttp.ClientTimeout.sock_connect
+        sock_read_timeout (models.FloatField): Value for aiohttp.ClientTimeout.sock_read
     """
 
     TYPE = "remote"
@@ -249,6 +254,19 @@ class Remote(MasterModel):
     download_concurrency = models.PositiveIntegerField(default=10)
     policy = models.TextField(choices=POLICY_CHOICES, default=IMMEDIATE)
 
+    total_timeout = models.FloatField(
+        null=True, validators=[MinValueValidator(0.0, "Timeout must be >= 0")]
+    )
+    connect_timeout = models.FloatField(
+        null=True, validators=[MinValueValidator(0.0, "Timeout must be >= 0")]
+    )
+    sock_connect_timeout = models.FloatField(
+        null=True, validators=[MinValueValidator(0.0, "Timeout must be >= 0")]
+    )
+    sock_read_timeout = models.FloatField(
+        null=True, validators=[MinValueValidator(0.0, "Timeout must be >= 0")]
+    )
+
     @property
     def download_factory(self):
         """

pulpcore/app/serializers/repository.py

@@ -113,6 +113,31 @@ class RemoteSerializer(ModelSerializer):
         default=models.Remote.IMMEDIATE,
     )
 
+    total_timeout = serializers.FloatField(
+        allow_null=True,
+        required=False,
+        help_text="aiohttp.ClientTimeout.total (q.v.) for download-connections.",
+        min_value=0.0,
+    )
+    connect_timeout = serializers.FloatField(
+        allow_null=True,
+        required=False,
+        help_text="aiohttp.ClientTimeout.connect (q.v.) for download-connections.",
+        min_value=0.0,
+    )
+    sock_connect_timeout = serializers.FloatField(
+        allow_null=True,
+        required=False,
+        help_text="aiohttp.ClientTimeout.sock_connect (q.v.) for download-connections.",
+        min_value=0.0,
+    )
+    sock_read_timeout = serializers.FloatField(
+        allow_null=True,
+        required=False,
+        help_text="aiohttp.ClientTimeout.sock_read (q.v.) for download-connections.",
+        min_value=0.0,
+    )
+
     def validate_url(self, value):
         """
         Check if the 'url' is a ``file://`` path, and if so, ensure it's an ALLOWED_IMPORT_PATH.
@@ -156,6 +181,10 @@ class Meta:
             "pulp_last_updated",
             "download_concurrency",
             "policy",
+            "total_timeout",
+            "connect_timeout",
+            "sock_connect_timeout",
+            "sock_read_timeout",
         )
 
 

pulpcore/app/settings.py

@@ -230,7 +230,6 @@
     },
 }
 
-
 # What kinds of checksums is this pulp-instance _allowed to use_ ?
 # NOTE : "sha256"" IS REQUIRED - Pulp will fail to start if it is not found in this set
 # NOTE: specifying checksums that are not listed under ALL_KNOWN_CONTENT_CHECKSUMS will fail

pulpcore/download/factory.py

@@ -15,7 +15,6 @@
 from .http import HttpDownloader
 from .file import FileDownloader
 
-
 PROTOCOL_MAP = {"http": HttpDownloader, "https": HttpDownloader, "file": FileDownloader}
 
 
@@ -118,8 +117,14 @@ def _make_aiohttp_session_from_remote(self):
         headers = {"User-Agent": user_agent()}
 
         conn = aiohttp.TCPConnector(**tcp_conn_opts)
-
-        timeout = aiohttp.ClientTimeout(total=None, sock_connect=600, sock_read=600)
+        total = self._remote.total_timeout
+        sock_connect = self._remote.sock_connect_timeout
+        sock_read = self._remote.sock_read_timeout
+        connect = self._remote.connect_timeout
+
+        timeout = aiohttp.ClientTimeout(
+            total=total, sock_connect=sock_connect, sock_read=sock_read, connect=connect
+        )
         return aiohttp.ClientSession(connector=conn, timeout=timeout, headers=headers)
 
     def build(self, url, **kwargs):

pulpcore/tests/functional/api/using_plugin/test_crud_repos.py

@@ -1,18 +1,31 @@
 # coding=utf-8
 """Tests that CRUD repositories."""
+import time
 import unittest
 from itertools import permutations
 from urllib.parse import urljoin
 
 from pulp_smash import api, config, utils
+from pulp_smash.pulp3.bindings import monitor_task
 from pulp_smash.pulp3.utils import gen_repo
+
 from requests.exceptions import HTTPError
 
-from pulpcore.tests.functional.api.using_plugin.constants import FILE_REMOTE_PATH, FILE_REPO_PATH
 from pulpcore.tests.functional.api.using_plugin.utils import gen_file_remote
+from pulpcore.tests.functional.api.using_plugin.constants import (
+    FILE_FIXTURE_MANIFEST_URL,
+    FILE_REMOTE_PATH,
+    FILE_REPO_PATH,
+)
 from pulpcore.tests.functional.utils import set_up_module as setUpModule  # noqa:F401
 from pulpcore.tests.functional.utils import skip_if
 
+from pulpcore.client.pulp_file.exceptions import ApiException
+from pulpcore.client.pulp_file import (
+    ApiClient as FileApiClient,
+    RemotesFileApi,
+)
+
 
 class CRUDRepoTestCase(unittest.TestCase):
     """CRUD repositories."""
@@ -192,3 +205,98 @@ def test_negative_create_repo_with_invalid_parameter(self):
         response = api.Client(self.cfg, api.echo_handler).post(FILE_REPO_PATH, gen_repo(foo="bar"))
         assert response.status_code == 400
         assert response.json()["foo"] == ["Unexpected field"]
+
+
+class CRUDRemoteTestCase(unittest.TestCase):
+    """CRUD remotes."""
+
+    @classmethod
+    def setUpClass(cls):
+        """Create class-wide variables."""
+        cls.cfg = config.get_config()
+
+    def setUp(self):
+        self.client = FileApiClient(self.cfg.get_bindings_config())
+        self.remotes_api = RemotesFileApi(self.client)
+        self.remote_attrs = {
+            "name": utils.uuid4(),
+            "url": FILE_FIXTURE_MANIFEST_URL,
+            "ca_cert": None,
+            "client_cert": None,
+            "client_key": None,
+            "tls_validation": False,
+            "proxy_url": None,
+            "username": "pulp",
+            "password": "pulp",
+            "download_concurrency": 10,
+            "policy": "on_demand",
+            "total_timeout": None,
+            "connect_timeout": None,
+            "sock_connect_timeout": None,
+            "sock_read_timeout": None,
+        }
+        self.remote = self.remotes_api.create(self.remote_attrs)
+
+    def _compare_results(self, data, received):
+        for k in data:
+            self.assertEqual(getattr(received, k), data[k])
+
+    def test_read(self):
+        # Compare initial-attrs vs remote created in setUp
+        self._compare_results(self.remote_attrs, self.remote)
+
+    def test_update(self):
+        data = {"download_concurrency": 66, "policy": "immediate"}
+        self.remotes_api.partial_update(self.remote.pulp_href, data)
+        time.sleep(1)  # without this, the read returns the pre-patch values
+        new_remote = self.remotes_api.read(self.remote.pulp_href)
+        self._compare_results(data, new_remote)
+
+    def test_timeout_attributes(self):
+        # Test valid timeout settings (float >= 0)
+        data = {
+            "total_timeout": 1.0,
+            "connect_timeout": 66.0,
+            "sock_connect_timeout": 0.0,
+            "sock_read_timeout": 3.1415926535,
+        }
+        self.remotes_api.partial_update(self.remote.pulp_href, data)
+        time.sleep(1)
+        new_remote = self.remotes_api.read(self.remote.pulp_href)
+        self._compare_results(data, new_remote)
+
+    def test_timeout_attributes_float_lt_zero(self):
+        # Test invalid float < 0
+        data = {
+            "total_timeout": -1.0,
+        }
+        with self.assertRaises(ApiException):
+            self.remotes_api.partial_update(self.remote.pulp_href, data)
+
+    def test_timeout_attributes_non_float(self):
+        # Test invalid non-float
+        data = {
+            "connect_timeout": "abc",
+        }
+        with self.assertRaises(ApiException):
+            self.remotes_api.partial_update(self.remote.pulp_href, data)
+
+    def test_timeout_attributes_reset_to_empty(self):
+        # Test reset to empty
+        data = {
+            "total_timeout": False,
+            "connect_timeout": None,
+            "sock_connect_timeout": False,
+            "sock_read_timeout": None,
+        }
+        response = self.remotes_api.partial_update(self.remote.pulp_href, data)
+        monitor_task(response.task)
+        new_remote = self.remotes_api.read(self.remote.pulp_href)
+        self._compare_results(data, new_remote)
+
+    def test_delete(self):
+        response = self.remotes_api.delete(self.remote.pulp_href)
+        monitor_task(response.task)
+        # verify the delete
+        with self.assertRaises(ApiException):
+            self.remotes_api.read(self.remote.pulp_href)