Skip to content

Add more useful PulpExceptions for general tasks #7271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gerrod3 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add more useful PulpExceptions for general tasks #7271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/plugin_api/7270.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added more useful error exceptions for general tasks.
16 changes: 12 additions & 4 deletions pulp_file/app/tasks/synchronizing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
from gettext import gettext as _
from urllib.parse import quote, urlparse, urlunparse

import aiohttp.client_exceptions
from django.core.files import File

from pulpcore.plugin.exceptions import SyncError
from pulpcore.plugin.models import Artifact, ProgressReport, Remote, PublishedMetadata
from pulpcore.plugin.serializers import RepositoryVersionSerializer
from pulpcore.plugin.stages import (
Expand Down Expand Up @@ -38,14 +40,14 @@ def synchronize(remote_pk, repository_pk, mirror, url=None):
url (str): The url to synchronize. If omitted, the url of the remote is used.

Raises:
ValueError: If the remote does not specify a URL to sync.
SyncError: If the remote does not specify a URL to sync.

"""
remote = FileRemote.objects.get(pk=remote_pk)
repository = FileRepository.objects.get(pk=repository_pk)

if not remote.url:
raise ValueError(_("A remote must have a url specified to synchronize."))
raise SyncError(_("A remote must have a url specified to synchronize."))

first_stage = FileFirstStage(remote, url)
dv = DeclarativeVersion(first_stage, repository, mirror=mirror, acs=True)
Expand Down Expand Up @@ -103,15 +105,21 @@ async def run(self):
parsed_url = urlparse(self.url)
root_dir = os.path.dirname(parsed_url.path)
downloader = self.remote.get_downloader(url=self.url)
result = await downloader.run()
try:
result = await downloader.run()
except aiohttp.client_exceptions.ClientResponseError as e:
raise SyncError(_("Error downloading manifest file: {error}").format(error=e))
await pb.aincrement()
metadata_files.append((result.path, self.url.split("/")[-1]))

async with ProgressReport(
message="Parsing Metadata Lines", code="sync.parsing.metadata"
) as pb:
manifest = Manifest(result.path)
entries = list(manifest.read())
try:
entries = list(manifest.read())
except ValueError as e:
raise SyncError(_("Error parsing manifest file: {error}").format(error=e))

pb.total = len(entries)
await pb.asave()
Expand Down
8 changes: 8 additions & 0 deletions pulp_file/pytest_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,14 @@ def duplicate_filename_paths(write_3_iso_file_fixture_data_factory):
)


@pytest.fixture(scope="class")
def missing_file_path(file_fixtures_root):
file_fixtures_root.joinpath("missing_file").mkdir()
file = {"name": "missing_file/1.iso", "digest": "1234567890", "size": 100}
generate_manifest(file_fixtures_root.joinpath("missing_file/PULP_MANIFEST"), [file])
return "/missing_file/PULP_MANIFEST"


@pytest.fixture(scope="class")
def file_fixture_server_ssl_client_cert_req(
ssl_ctx_req_client_auth, file_fixtures_root, gen_fixture_server
Expand Down
33 changes: 31 additions & 2 deletions pulp_file/tests/functional/api/test_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ def test_invalid_url(file_bindings, file_repo, gen_object_with_cleanup, monitor_
remote = gen_object_with_cleanup(file_bindings.RemotesFileApi, remote_kwargs)

body = RepositorySyncURL(remote=remote.pulp_href)
with pytest.raises(PulpTaskError):
with pytest.raises(PulpTaskError) as e:
monitor_task(file_bindings.RepositoriesFileApi.sync(file_repo.pulp_href, body).task)
assert "[PLP0008] URL lookup failed." in e.value.task.error["description"]


@pytest.mark.parallel
Expand All @@ -90,8 +91,36 @@ def test_invalid_file(
"""Sync a repository using an invalid file repository."""
remote = file_remote_factory(manifest_path=invalid_manifest_path, policy="immediate")
body = RepositorySyncURL(remote=remote.pulp_href)
with pytest.raises(PulpTaskError):
with pytest.raises(PulpTaskError) as e:
monitor_task(file_bindings.RepositoriesFileApi.sync(file_repo.pulp_href, body).task)
assert "[PLP0003]" in e.value.task.error["description"]


@pytest.mark.parallel
def test_missing_file(
file_repo, file_bindings, missing_file_path, file_remote_factory, monitor_task
):
"""Sync a repository using a manifest file with a missing file."""
remote = file_remote_factory(manifest_path=missing_file_path, policy="immediate")
body = RepositorySyncURL(remote=remote.pulp_href)
with pytest.raises(PulpTaskError) as e:
monitor_task(file_bindings.RepositoriesFileApi.sync(file_repo.pulp_href, body).task)
assert "[PLP0013] Sync failed: Error downloading artifact" in e.value.task.error["description"]


@pytest.mark.parallel
def test_missing_manifest(
file_repo, file_bindings, basic_manifest_path, file_remote_factory, monitor_task
):
"""Sync a repository using a missing manifest file."""
remote = file_remote_factory(manifest_path="/MISSING_MANIFEST", policy="immediate")
body = RepositorySyncURL(remote=remote.pulp_href)
with pytest.raises(PulpTaskError) as e:
monitor_task(file_bindings.RepositoriesFileApi.sync(file_repo.pulp_href, body).task)
assert (
"[PLP0013] Sync failed: Error downloading manifest file"
in e.value.task.error["description"]
)


@pytest.mark.parallel
Expand Down
16 changes: 11 additions & 5 deletions pulpcore/app/models/content.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import asyncio
import datetime
import json
import logging
import os
import tempfile
import shutil
Expand Down Expand Up @@ -34,8 +35,11 @@
SizeValidationError,
MissingDigestValidationError,
UnsupportedDigestValidationError,
ExternalServiceError,
)

logger = logging.getLogger(__name__)

# All available digest fields ordered by algorithm strength.
_DIGEST_FIELDS = []
for alg in ("sha512", "sha384", "sha256", "sha224", "sha1", "md5"):
Expand Down Expand Up @@ -821,7 +825,7 @@ def sign(self, filename, env_vars=None):
env_vars (dict): dictionary of environment variables

Raises:
RuntimeError: If the return code of the script is not equal to 0.
ExternalServiceError: If the return code of the script is not equal to 0.

Returns:
A dictionary as validated by the validate() method.
Expand All @@ -834,12 +838,13 @@ def sign(self, filename, env_vars=None):
)

if completed_process.returncode != 0:
raise RuntimeError(str(completed_process.stderr))
logger.info("Signing service script failed: {}".format(str(completed_process.stderr)))
raise ExternalServiceError("Signing service script")

try:
return_value = json.loads(completed_process.stdout)
except json.JSONDecodeError:
raise RuntimeError("The signing service script did not return valid JSON!")
raise ExternalServiceError("Signing service script", "did not return valid JSON")

return return_value

Expand All @@ -855,12 +860,13 @@ async def asign(self, filename, env_vars=None):

stdout, stderr = await process.communicate()
if process.returncode != 0:
raise RuntimeError(str(stderr))
logger.info("Signing service script failed: {}".format(str(stderr)))
raise ExternalServiceError("Signing service script")

try:
return_value = json.loads(stdout)
except json.JSONDecodeError:
raise RuntimeError("The signing service script did not return valid JSON!")
raise ExternalServiceError("Signing service script", "did not return valid JSON")

return return_value

Expand Down
11 changes: 4 additions & 7 deletions pulpcore/app/models/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
from django.db import models, transaction
from django.db.models import F, Func, Q, Value
from django_lifecycle import AFTER_UPDATE, BEFORE_CREATE, BEFORE_DELETE, hook
from rest_framework.exceptions import APIException

from pulpcore.app.util import (
batch_qs,
Expand All @@ -28,7 +27,7 @@
)
from pulpcore.constants import ALL_KNOWN_CONTENT_CHECKSUMS, PROTECTED_REPO_VERSION_MESSAGE
from pulpcore.download.factory import DownloaderFactory
from pulpcore.exceptions import ResourceImmutableError
from pulpcore.exceptions import ResourceImmutableError, RepositoryVersionDeleteError

from pulpcore.cache import Cache

Expand Down Expand Up @@ -1249,7 +1248,7 @@ def _squash(self, repo_relations, next_version):
def check_protected(self):
"""Check if a repo version is protected before trying to delete it."""
if self in self.repository.protected_versions():
raise Exception(PROTECTED_REPO_VERSION_MESSAGE)
raise RepositoryVersionDeleteError(PROTECTED_REPO_VERSION_MESSAGE)

def delete(self, **kwargs):
"""
Expand All @@ -1263,7 +1262,7 @@ def delete(self, **kwargs):
"""
if self.complete:
if self.repository.versions.complete().count() <= 1:
raise APIException(_("Attempt to delete the last remaining version."))
raise RepositoryVersionDeleteError()
if settings.CACHE_ENABLED:
base_paths = self.distribution_set.values_list("base_path", flat=True)
if base_paths:
Expand Down Expand Up @@ -1335,9 +1334,7 @@ def __enter__(self):
RepositoryVersion: self
"""
if self.complete:
raise RuntimeError(
_("This Repository version is complete. It cannot be modified further.")
)
raise ResourceImmutableError(self)
repository = self.repository.cast()
repository.initialize_new_version(self)
return self
Expand Down
21 changes: 16 additions & 5 deletions pulpcore/app/tasks/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
from django.db import transaction
from rest_framework.exceptions import ValidationError as DRFValidationError

from pulpcore.app.apps import get_plugin_config
from pulpcore.app.models import CreatedResource
from pulpcore.app.loggers import deprecation_logger
from pulpcore.plugin.models import MasterModel
from pulpcore.exceptions import ValidationError

from asgiref.sync import sync_to_async

Expand All @@ -28,7 +30,7 @@ def general_create(app_label, serializer_name, *args, **kwargs):
Create a model instance.

Raises:
ValidationError: If the serializer is not valid
SerializerValidationError: If the serializer is not valid

"""
data = kwargs.pop("data", None)
Expand All @@ -38,7 +40,10 @@ def general_create(app_label, serializer_name, *args, **kwargs):
serializer_class = get_plugin_config(app_label).named_serializers[serializer_name]
serializer = serializer_class(data=data, context=context)

serializer.is_valid(raise_exception=True)
try:
serializer.is_valid(raise_exception=True)
except DRFValidationError as e:
raise ValidationError(e.detail)
instance = serializer.save()
if isinstance(instance, MasterModel):
instance = instance.cast()
Expand All @@ -65,7 +70,7 @@ def general_update(instance_id, app_label, serializer_name, *args, **kwargs):
their values are updated as such.

Raises:
[rest_framework.exceptions.ValidationError][]: When serializer instance can't be saved
SerializerValidationError: When serializer instance can't be saved
due to validation error. This theoretically should never occur since validation is
performed before the task is dispatched.
"""
Expand All @@ -80,7 +85,10 @@ def general_update(instance_id, app_label, serializer_name, *args, **kwargs):
if isinstance(instance, MasterModel):
instance = instance.cast()
serializer = serializer_class(instance, data=data, partial=partial)
serializer.is_valid(raise_exception=True)
try:
serializer.is_valid(raise_exception=True)
except DRFValidationError as e:
raise ValidationError(e.detail)
serializer.save()


Expand Down Expand Up @@ -141,7 +149,10 @@ async def ageneral_update(instance_id, app_label, serializer_name, *args, **kwar
if isinstance(instance, MasterModel):
instance = await instance.acast()
serializer = serializer_class(instance, data=data, partial=partial, context=context)
await sync_to_async(serializer.is_valid)(raise_exception=True)
try:
await sync_to_async(serializer.is_valid)(raise_exception=True)
except DRFValidationError as e:
raise ValidationError(e.detail)
await sync_to_async(serializer.save)()
return await sync_to_async(lambda: serializer.data)()

Expand Down
19 changes: 12 additions & 7 deletions pulpcore/app/tasks/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
export_content,
)
from pulpcore.constants import FS_EXPORT_METHODS
from pulpcore.exceptions import ExportError
from rest_framework.exceptions import ValidationError as DRFValidationError

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -66,14 +68,14 @@ def _export_to_file_system(path, relative_paths_to_artifacts, method=FS_EXPORT_M
relative_paths_to_artifacts: A dict with {relative_path: artifact} mapping

Raises:
ValidationError: When path is not in the ALLOWED_EXPORT_PATHS setting
ExportError: When path is not in the ALLOWED_EXPORT_PATHS setting
"""
using_filesystem_storage = (
settings.STORAGES["default"]["BACKEND"] == "pulpcore.app.models.storage.FileSystem"
)

if method != FS_EXPORT_METHODS.WRITE and not using_filesystem_storage:
raise RuntimeError(_("Only write is supported for non-filesystem storage."))
raise ExportError(_("Only write is supported for non-filesystem storage."))

os.makedirs(path)
export_not_on_same_filesystem = (
Expand Down Expand Up @@ -101,7 +103,7 @@ def _export_to_file_system(path, relative_paths_to_artifacts, method=FS_EXPORT_M
for chunk in af.chunks(1024 * 1024):
f.write(chunk)
else:
raise RuntimeError(_("Unsupported export method '{}'.").format(method))
raise ExportError(_("Unsupported export method '{}'.").format(method))


def _export_publication_to_file_system(
Expand Down Expand Up @@ -218,7 +220,7 @@ def fs_publication_export(exporter_pk, publication_pk, start_repo_version_pk=Non
)

if not _export_location_is_clean(exporter.path):
raise RuntimeError(_("Cannot export to directories that contain existing data."))
raise ExportError(_("Cannot export to directories that contain existing data."))

_export_publication_to_file_system(
exporter.path, publication, start_repo_version=start_repo_version, method=exporter.method
Expand Down Expand Up @@ -369,12 +371,15 @@ def pulp_export(exporter_pk, params):
params (dict): request data

Raises:
ValidationError: When path is not in the ALLOWED_EXPORT_PATHS setting,
ExportError: When path is not in the ALLOWED_EXPORT_PATHS setting,
OR path exists and is not a directory
"""
pulp_exporter = PulpExporter.objects.get(pk=exporter_pk)
serializer = PulpExportSerializer(data=params, context={"exporter": pulp_exporter})
serializer.is_valid(raise_exception=True)
try:
serializer.is_valid(raise_exception=True)
except DRFValidationError as e:
raise ExportError(e.detail)
the_export = PulpExport.objects.create(exporter=pulp_exporter, params=params)
the_export.validated_versions = serializer.validated_data.get("versions", None)
the_export.validated_start_versions = serializer.validated_data.get("start_versions", None)
Expand Down Expand Up @@ -470,7 +475,7 @@ def _do_export(pulp_exporter, tar, the_export):
# an on_demand repo
content_artifacts = ContentArtifact.objects.filter(content__in=version.content)
if content_artifacts.filter(artifact=None).exists():
raise RuntimeError(_("Remote artifacts cannot be exported."))
raise ExportError(_("Remote artifacts cannot be exported."))

if do_incremental:
vers_artifacts = version.artifacts.difference(vers_match[version].artifacts)
Expand Down
Loading