From 05faf9f04c43e20a7d00841d1925c2ef5e0363dc Mon Sep 17 00:00:00 2001 From: Grant Gainey Date: Thu, 12 Nov 2020 16:19:40 -0500 Subject: [PATCH] Added fields controlling cnx-timeouts to Remote. closes #7201 --- CHANGES/7201.feature | 5 + pulpcore/app/migrations/0051_timeoutfields.py | 34 ++++++ pulpcore/app/models/repository.py | 18 +++ pulpcore/app/serializers/repository.py | 29 +++++ pulpcore/app/settings.py | 1 - pulpcore/download/factory.py | 11 +- .../api/using_plugin/test_crud_repos.py | 110 +++++++++++++++++- 7 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 CHANGES/7201.feature create mode 100644 pulpcore/app/migrations/0051_timeoutfields.py diff --git a/CHANGES/7201.feature b/CHANGES/7201.feature new file mode 100644 index 0000000000..fad7af60d7 --- /dev/null +++ b/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. diff --git a/pulpcore/app/migrations/0051_timeoutfields.py b/pulpcore/app/migrations/0051_timeoutfields.py new file mode 100644 index 0000000000..a11043582c --- /dev/null +++ b/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')]), + ), + ] diff --git a/pulpcore/app/models/repository.py b/pulpcore/app/models/repository.py index e200999858..0c9c9aa354 100644 --- a/pulpcore/app/models/repository.py +++ b/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): """ diff --git a/pulpcore/app/serializers/repository.py b/pulpcore/app/serializers/repository.py index ea01ec6350..c1cc60ab7b 100644 --- a/pulpcore/app/serializers/repository.py +++ b/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", ) diff --git a/pulpcore/app/settings.py b/pulpcore/app/settings.py index f0b9ff07fe..9875deca3a 100644 --- a/pulpcore/app/settings.py +++ b/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 diff --git a/pulpcore/download/factory.py b/pulpcore/download/factory.py index ca73d36297..181f15459c 100644 --- a/pulpcore/download/factory.py +++ b/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): diff --git a/pulpcore/tests/functional/api/using_plugin/test_crud_repos.py b/pulpcore/tests/functional/api/using_plugin/test_crud_repos.py index e3541aa74c..54c3c648d9 100644 --- a/pulpcore/tests/functional/api/using_plugin/test_crud_repos.py +++ b/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)