Add cooldown time for failed RemoteArtifact fetch

On a request for on-demand content in the content app, a corrupted Remote
that contains the wrong binary (for that content) prevented other Remotes
from being attempted on future requests.

Now the last failed Remotes are temporarily ignored and others may be picked.

Closes #5725

CHANGES/5725.bugfix

@@ -0,0 +1,4 @@
+On a request for on-demand content in the content app, a corrupted Remote that
+contains the wrong binary (for that content) prevented other Remotes from being
+attempted on future requests. Now the last failed Remotes are temporarily ignored
+and others may be picked.

pulp_file/pytest_plugin.py

@@ -83,11 +83,11 @@ def file_fixtures_root(tmp_path):
 
 @pytest.fixture
 def write_3_iso_file_fixture_data_factory(file_fixtures_root):
-    def _write_3_iso_file_fixture_data_factory(name, overwrite=False):
+    def _write_3_iso_file_fixture_data_factory(name, overwrite=False, seed=None):
         file_fixtures_root.joinpath(name).mkdir(exist_ok=overwrite)
-        file1 = generate_iso(file_fixtures_root.joinpath(f"{name}/1.iso"))
-        file2 = generate_iso(file_fixtures_root.joinpath(f"{name}/2.iso"))
-        file3 = generate_iso(file_fixtures_root.joinpath(f"{name}/3.iso"))
+        file1 = generate_iso(file_fixtures_root.joinpath(f"{name}/1.iso"), seed=seed)
+        file2 = generate_iso(file_fixtures_root.joinpath(f"{name}/2.iso"), seed=seed)
+        file3 = generate_iso(file_fixtures_root.joinpath(f"{name}/3.iso"), seed=seed)
         generate_manifest(
             file_fixtures_root.joinpath(f"{name}/PULP_MANIFEST"), [file1, file2, file3]
         )
@@ -364,3 +364,19 @@ def _wget_recursive_download_on_host(url, destination):
         )
 
     return _wget_recursive_download_on_host
+
+
+@pytest.fixture
+def generate_server_and_remote(
+    file_bindings, gen_fixture_server, file_fixtures_root, gen_object_with_cleanup
+):
+    def _generate_server_and_remote(*, manifest_path, policy):
+        server = gen_fixture_server(file_fixtures_root, None)
+        url = server.make_url(manifest_path)
+        remote = gen_object_with_cleanup(
+            file_bindings.RemotesFileApi,
+            {"name": str(uuid.uuid4()), "url": str(url), "policy": policy},
+        )
+        return server, remote
+
+    yield _generate_server_and_remote

pulp_file/tests/functional/api/test_acs.py

@@ -12,22 +12,6 @@
 )
 
 
-@pytest.fixture
-def generate_server_and_remote(
-    file_bindings, gen_fixture_server, file_fixtures_root, gen_object_with_cleanup
-):
-    def _generate_server_and_remote(*, manifest_path, policy):
-        server = gen_fixture_server(file_fixtures_root, None)
-        url = server.make_url(manifest_path)
-        remote = gen_object_with_cleanup(
-            file_bindings.RemotesFileApi,
-            {"name": str(uuid.uuid4()), "url": str(url), "policy": policy},
-        )
-        return server, remote
-
-    yield _generate_server_and_remote
-
-
 @pytest.mark.parallel
 def test_acs_validation_and_update(
     file_bindings,

pulpcore/app/migrations/0126_remoteartifact_failed_at.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.16 on 2024-11-27 15:06
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('core', '0125_openpgpdistribution_openpgpkeyring_openpgppublickey_and_more'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='remoteartifact',
+            name='failed_at',
+            field=models.DateTimeField(null=True),
+        ),
+    ]

pulpcore/app/models/content.py

@@ -703,6 +703,7 @@ class RemoteArtifact(BaseModel, QueryMixin):
         sha256 (models.CharField): The expected SHA-256 checksum of the file.
         sha384 (models.CharField): The expected SHA-384 checksum of the file.
         sha512 (models.CharField): The expected SHA-512 checksum of the file.
+        failed_at (models.DateTimeField): The datetime of last download attempt failure.
 
     Relations:
 
@@ -721,6 +722,7 @@ class RemoteArtifact(BaseModel, QueryMixin):
     sha256 = models.CharField(max_length=64, null=True, db_index=True)
     sha384 = models.CharField(max_length=96, null=True, db_index=True)
     sha512 = models.CharField(max_length=128, null=True, db_index=True)
+    failed_at = models.DateTimeField(null=True)
 
     content_artifact = models.ForeignKey(ContentArtifact, on_delete=models.CASCADE)
     remote = models.ForeignKey("Remote", on_delete=models.CASCADE)

pulpcore/app/settings.py

@@ -295,6 +295,11 @@
     "EXPIRES_TTL": 600,  # 10 minutes
 }
 
+# The time a RemoteArtifact will be ignored after failure.
+# In on-demand, if a fetching content from a remote failed due to corrupt data,
+# the corresponding RemoteArtifact will be ignored for that time (seconds).
+REMOTE_CONTENT_FETCH_FAILURE_COOLDOWN = 5 * 60  # 5 minutes
+
 SPECTACULAR_SETTINGS = {
     "SERVE_URLCONF": ROOT_URLCONF,
     "DEFAULT_GENERATOR_CLASS": "pulpcore.openapi.PulpSchemaGenerator",

pulpcore/content/handler.py

@@ -6,6 +6,7 @@
 import socket
 import struct
 from gettext import gettext as _
+from datetime import timedelta
 
 from aiohttp.client_exceptions import ClientResponseError, ClientConnectionError
 from aiohttp.web import FileResponse, StreamResponse, HTTPOk
@@ -22,6 +23,7 @@
 from asgiref.sync import sync_to_async
 
 import django
+from django.utils import timezone
 
 from pulpcore.constants import STORAGE_RESPONSE_MAP
 from pulpcore.responses import ArtifactResponse
@@ -820,22 +822,25 @@ async def _stream_content_artifact(self, request, response, content_artifact):
                 [pulpcore.plugin.models.ContentArtifact][] returned the binary data needed for
                 the client.
         """
-        # We should only retry with exceptions that happen before we receive any data
+        # We should only skip exceptions that happen before we receive any data
         # and start streaming, as we can't rollback data if something happens after that.
-        RETRYABLE_EXCEPTIONS = (
+        SKIPPABLE_EXCEPTIONS = (
             ClientResponseError,
             UnsupportedDigestValidationError,
             ClientConnectionError,
         )
 
-        remote_artifacts = content_artifact.remoteartifact_set.select_related(
-            "remote"
-        ).order_by_acs()
+        protection_time = settings.REMOTE_CONTENT_FETCH_FAILURE_COOLDOWN
+        remote_artifacts = (
+            content_artifact.remoteartifact_set.select_related("remote")
+            .order_by_acs()
+            .exclude(failed_at__gte=timezone.now() - timedelta(seconds=protection_time))
+        )
         async for remote_artifact in remote_artifacts:
             try:
                 response = await self._stream_remote_artifact(request, response, remote_artifact)
                 return response
-            except RETRYABLE_EXCEPTIONS as e:
+            except SKIPPABLE_EXCEPTIONS as e:
                 log.warning(
                     "Could not download remote artifact at '{}': {}".format(
                         remote_artifact.url, str(e)
@@ -1142,14 +1147,22 @@ async def finalize():
                 extra_data={"disable_retry_list": (DigestValidationError,)}
             )
         except DigestValidationError:
+            remote_artifact.failed_at = timezone.now()
+            await remote_artifact.asave()
             await downloader.session.close()
             close_tcp_connection(request.transport._sock)
+            REMOTE_CONTENT_FETCH_FAILURE_COOLDOWN = settings.REMOTE_CONTENT_FETCH_FAILURE_COOLDOWN
             raise RuntimeError(
-                f"We tried streaming {remote_artifact.url!r} to the client, but it "
-                "failed checkusm validation. "
-                "At this point, we cant recover from wrong data already sent, "
-                "so we are forcing the connection to close. "
-                "If this error persists, the remote server might be corrupted."
+                f"Pulp tried streaming {remote_artifact.url!r} to "
+                "the client, but it failed checksum validation.\n\n"
+                "We can't recover from wrong data already sent so we are:\n"
+                "- Forcing the connection to close.\n"
+                "- Marking this Remote to be ignored for "
+                f"{REMOTE_CONTENT_FETCH_FAILURE_COOLDOWN=}s.\n\n"
+                "If the Remote is known to be fixed, try resyncing the associated repository.\n"
+                "If the Remote is known to be permanently corrupted, try removing "
+                "affected Pulp Remote, adding a good one and resyncing.\n"
+                "If the problem persists, please contact the Pulp team."
             )
 
         if content_length := response.headers.get("Content-Length"):

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

@@ -1,15 +1,14 @@
 """Tests related to content delivery."""
 
-from aiohttp.client_exceptions import ClientResponseError, ClientPayloadError
 import hashlib
-import pytest
 import subprocess
+import uuid
 from urllib.parse import urljoin
 
-from pulpcore.client.pulp_file import (
-    RepositorySyncURL,
-)
+import pytest
+from aiohttp.client_exceptions import ClientPayloadError, ClientResponseError
 
+from pulpcore.client.pulp_file import RepositorySyncURL
 from pulpcore.tests.functional.utils import download_file, get_files_in_manifest
 
 
@@ -116,8 +115,13 @@ def test_remote_content_changed_with_on_demand(
 ):
     """
     GIVEN a remote synced on demand with fileA (e.g, digest=123),
-    WHEN on the remote server, fileA changed its content (e.g, digest=456),
-    THEN retrieving fileA from the content app will cause a connection-close/incomplete-response.
+    AND the remote server, fileA changed its content (e.g, digest=456),
+
+    WHEN the client first requests that content
+    THEN the content app will start a response but close the connection before finishing
+
+    WHEN the client requests that content again (within the RA cooldown interval)
+    THEN the content app will return a 404
     """
     # GIVEN
     basic_manifest_path = write_3_iso_file_fixture_data_factory("basic")
@@ -129,17 +133,108 @@ def test_remote_content_changed_with_on_demand(
     repo = file_bindings.RepositoriesFileApi.read(file_repo_with_auto_publish.pulp_href)
     distribution = file_distribution_factory(repository=repo.pulp_href)
     expected_file_list = list(get_files_in_manifest(remote.url))
-
-    # WHEN
     write_3_iso_file_fixture_data_factory("basic", overwrite=True)
 
-    # THEN
     get_url = urljoin(distribution.base_url, expected_file_list[0][0])
-    with pytest.raises(ClientPayloadError, match="Response payload is not completed"):
-        download_file(get_url)
 
-    # Assert again with curl just to be sure.
+    # WHEN (first request)
     result = subprocess.run(["curl", "-v", get_url], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+    # THEN
     assert result.returncode == 18
     assert b"* Closing connection 0" in result.stderr
     assert b"curl: (18) transfer closed with outstanding read data remaining" in result.stderr
+
+    # WHEN (second request)
+    result = subprocess.run(["curl", "-v", get_url], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+    # THEN
+    assert result.returncode == 0
+    assert b"< HTTP/1.1 404 Not Found" in result.stderr
+
+
+@pytest.mark.parallel
+def test_handling_remote_artifact_on_demand_streaming_failure(
+    write_3_iso_file_fixture_data_factory,
+    file_repo_with_auto_publish,
+    file_remote_factory,
+    file_bindings,
+    monitor_task,
+    monitor_task_group,
+    file_distribution_factory,
+    gen_object_with_cleanup,
+    generate_server_and_remote,
+):
+    """
+    GIVEN A content synced with on-demand which has 2 RemoteArtifacts (Remote + ACS).
+    AND Only the ACS RemoteArtifact (that has priority on the content-app) is corrupted
+
+    WHEN a client requests the content for the first time
+    THEN the client doesnt get any content
+
+    WHEN a client requests the content for the second time
+    THEN the client gets the right content
+    """
+
+    # Plumbing
+    def create_simple_remote(manifest_path):
+        remote = file_remote_factory(manifest_path=manifest_path, policy="on_demand")
+        body = RepositorySyncURL(remote=remote.pulp_href)
+        monitor_task(
+            file_bindings.RepositoriesFileApi.sync(file_repo_with_auto_publish.pulp_href, body).task
+        )
+        return remote
+
+    def create_acs_remote(manifest_path):
+        acs_server, acs_remote = generate_server_and_remote(
+            manifest_path=manifest_path, policy="on_demand"
+        )
+        acs = gen_object_with_cleanup(
+            file_bindings.AcsFileApi,
+            {"remote": acs_remote.pulp_href, "paths": [], "name": str(uuid.uuid4())},
+        )
+        monitor_task_group(file_bindings.AcsFileApi.refresh(acs.pulp_href).task_group)
+        return acs
+
+    def sync_publish_and_distribute(remote):
+        body = RepositorySyncURL(remote=remote.pulp_href)
+        monitor_task(
+            file_bindings.RepositoriesFileApi.sync(file_repo_with_auto_publish.pulp_href, body).task
+        )
+        repo = file_bindings.RepositoriesFileApi.read(file_repo_with_auto_publish.pulp_href)
+        distribution = file_distribution_factory(repository=repo.pulp_href)
+        return distribution
+
+    def refresh_acs(acs):
+        monitor_task_group(file_bindings.AcsFileApi.refresh(acs.pulp_href).task_group)
+        return acs
+
+    def get_original_content_info(remote):
+        expected_files = get_files_in_manifest(remote.url)
+        content_unit = list(expected_files)[0]
+        return content_unit[0], content_unit[1]
+
+    def download_from_distribution(content, distribution):
+        content_unit_url = urljoin(distribution.base_url, content_name)
+        downloaded_file = download_file(content_unit_url)
+        actual_checksum = hashlib.sha256(downloaded_file.body).hexdigest()
+        return actual_checksum
+
+    # GIVEN
+    basic_manifest_path = write_3_iso_file_fixture_data_factory("basic", seed=123)
+    acs_manifest_path = write_3_iso_file_fixture_data_factory("acs", seed=123)
+    remote = create_simple_remote(basic_manifest_path)
+    distribution = sync_publish_and_distribute(remote)
+    acs = create_acs_remote(acs_manifest_path)
+    refresh_acs(acs)
+    write_3_iso_file_fixture_data_factory("acs", overwrite=True)  # corrupt
+
+    # WHEN/THEN (first request)
+    content_name, expected_checksum = get_original_content_info(remote)
+
+    with pytest.raises(ClientPayloadError, match="Response payload is not completed"):
+        download_from_distribution(content_name, distribution)
+
+    # WHEN/THEN (second request)
+    actual_checksum = download_from_distribution(content_name, distribution)
+    assert actual_checksum == expected_checksum

pulpcore/tests/functional/utils.py

@@ -4,6 +4,7 @@
 import asyncio
 import hashlib
 import os
+import random
 
 from aiohttp import web
 from dataclasses import dataclass
@@ -103,10 +104,14 @@ async def _download_file(url, auth=None, headers=None):
             return MockDownload(body=await response.read(), response_obj=response)
 
 
-def generate_iso(full_path, size=1024, relative_path=None):
+def generate_iso(full_path, size=1024, relative_path=None, seed=None):
     """Generate a random file."""
     with open(full_path, "wb") as fout:
-        contents = os.urandom(size)
+        if seed:
+            random.seed(seed)
+            contents = random.randbytes(size)
+        else:
+            contents = os.urandom(size)
         fout.write(contents)
         fout.flush()
     digest = hashlib.sha256(contents).hexdigest()