Skip to content
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.

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

feat: handle migration errors from the template #2819

Merged
merged 8 commits into from Apr 14, 2022
4 changes: 4 additions & 0 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions renku/core/errors.py
Expand Up @@ -458,6 +458,10 @@ class InvalidTemplateError(TemplateError):
"""Raised when using a non-valid template."""


class TemplateMissingReferenceError(TemplateError):
"""Raised when using a non-valid template."""


class TemplateUpdateError(TemplateError):
"""Raised when a project couldn't be updated from its template."""

Expand Down
4 changes: 4 additions & 0 deletions renku/core/template/template.py
Expand Up @@ -420,6 +420,10 @@ def fetch(cls, source: Optional[str], reference: Optional[str]) -> "RepositoryTe
try:
repository = clone_repository(url=source, path=path, checkout_revision=reference, install_lfs=False)
except errors.GitError as e:
if "Cannot checkout reference" in str(e):
raise errors.TemplateMissingReferenceError(
f"Cannot find the reference {reference} in the template repository from {source}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference {reference} -> reference '{reference}' so that it's more visible what the reference is.

) from e
raise errors.InvalidTemplateError(f"Cannot clone template repository from {source}") from e

version = repository.head.commit.hexsha
Expand Down
10 changes: 9 additions & 1 deletion renku/core/template/usecase.py
Expand Up @@ -132,7 +132,15 @@ def update_template(
"break your project"
)

templates_source = fetch_templates_source(source=template_metadata.source, reference=template_metadata.reference)
try:
templates_source = fetch_templates_source(
source=template_metadata.source, reference=template_metadata.reference
)
except errors.TemplateMissingReferenceError as e:
message = f"{str(e)}; You can still manually update the template and set a difference reference."
raise errors.TemplateUpdateError(message)
except errors.InvalidTemplateError:
raise errors.TemplateUpdateError("Template cannot be fetched.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason to convert these errors to TemplateUpdateError? I believe we don't need to catch them here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to simplify catching/handling them in the service and in the CLI. For the latter, throwing a new error results in a clean error message, while not doing it means a long and confusing stack trace is printed (see screen below).
Is there a better way to handle this? Or maybe that should be done in the cli code?
Screenshot_20220413_134606

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It's fine to do it here.


update_available, latest_reference = templates_source.is_update_available(
id=template_metadata.id, reference=template_metadata.reference, version=template_metadata.version
Expand Down
43 changes: 43 additions & 0 deletions renku/ui/service/errors.py
Expand Up @@ -342,6 +342,29 @@ def __init__(self, exception=None):
super().__init__(exception=exception)


class UserProjectTemplateReferenceError(ServiceError):
"""The project's template original reference cannot be found anymore.

The reference has probably been removed, either on purpose or as a side effect of a
forced push.
"""

code = SVC_ERROR_USER + 141
userMessage = (
"The project's template original reference has been removed or overwritten."
" Manually changing it in a session may fix the problem."
" Further details: {message}."
)
devMessage = "Template reference is not available anymore. Details: {message}."

def __init__(self, exception):
super().__init__(
userMessage=self.userMessage.format(message=str(exception)),
devMessage=self.devMessage.format(message=str(exception)),
exception=exception,
)


class ProgramInvalidGenericFieldsError(ServiceError):
"""One or more fields are unexpected.

Expand Down Expand Up @@ -668,6 +691,26 @@ def __init__(self, exception=None):
super().__init__(exception=exception)


class IntermittentProjectTemplateUnavailable(ServiceError):
"""The reference template for the project is currently unavailable.

It may be a temporary issue in accessing the remote template, or it may have been deleted,
moved, or otherwise not-accessible.
"""

code = SVC_ERROR_INTERMITTENT + 140
userMessage = (
"The reference template for the project is currently unavailable."
" It may be a temporary problem, or the template may not be accessible anymore."
)
devMessage = (
"Error accessing the project template. This may be temporary, or the project may not be accessible anymore."
)

def __init__(self, exception=None):
super().__init__(exception=exception)


class IntermittentTimeoutError(ServiceError):
"""An operation timed out."""

Expand Down
9 changes: 7 additions & 2 deletions renku/ui/service/views/cache.py
Expand Up @@ -28,7 +28,11 @@
from renku.ui.service.gateways.gitlab_api_provider import GitlabAPIProvider
from renku.ui.service.views.api_versions import V0_9, V1_0, V1_1, VersionedBlueprint
from renku.ui.service.views.decorators import accepts_json, optional_identity, requires_cache, requires_identity
from renku.ui.service.views.error_handlers import handle_common_except, handle_migration_errors
from renku.ui.service.views.error_handlers import (
handle_common_except,
handle_migration_read_errors,
handle_migration_write_errors,
)
from renku.ui.service.views.v0_9.cache import add_v0_9_specific_endpoints
from renku.ui.service.views.v1_0.cache import add_v1_0_specific_endpoints

Expand Down Expand Up @@ -156,7 +160,7 @@ def list_projects_view(user_data, cache):

@cache_blueprint.route("/cache.migrate", methods=["POST"], provide_automatic_options=False, versions=[V1_1])
@handle_common_except
@handle_migration_errors
@handle_migration_write_errors
@accepts_json
@requires_cache
@requires_identity
Expand Down Expand Up @@ -187,6 +191,7 @@ def migrate_project_view(user_data, cache):
"/cache.migrations_check", methods=["GET"], provide_automatic_options=False, versions=[V1_0, V1_1]
)
@handle_common_except
@handle_migration_read_errors
@requires_cache
@optional_identity
def migration_check_project_view(user_data, cache):
Expand Down
25 changes: 23 additions & 2 deletions renku/ui/service/views/error_handlers.py
Expand Up @@ -36,6 +36,7 @@
MigrationRequired,
ParameterError,
RenkuException,
TemplateMissingReferenceError,
TemplateUpdateError,
UninitializedProject,
)
Expand All @@ -44,6 +45,7 @@
IntermittentDatasetExistsError,
IntermittentFileNotExistsError,
IntermittentProjectIdError,
IntermittentProjectTemplateUnavailable,
IntermittentRedisError,
IntermittentSettingExistsError,
IntermittentTimeoutError,
Expand All @@ -65,6 +67,7 @@
UserNonRenkuProjectError,
UserOutdatedProjectError,
UserProjectCreationError,
UserProjectTemplateReferenceError,
UserRepoBranchInvalidError,
UserRepoNoAccessError,
UserRepoUrlInvalidError,
Expand Down Expand Up @@ -375,8 +378,26 @@ def decorated_function(*args, **kwargs):
return decorated_function


def handle_migration_errors(f):
"""Wrapper which handles migrations exceptions."""
def handle_migration_read_errors(f):
"""Wrapper which handles migrations read exceptions."""
# noqa
@wraps(f)
def decorated_function(*args, **kwargs):
"""Represents decorated function."""
# NOTE: verify if this may better go in MigrationsCheckCtrl as try/except in to_response()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this comment. Is it a left-over TODO?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch! Yes, that is a leftover, thanks for spotting it!

try:
return f(*args, **kwargs)
except TemplateMissingReferenceError as e:
raise UserProjectTemplateReferenceError(e)
except (InvalidTemplateError, TemplateUpdateError) as e:
raise IntermittentProjectTemplateUnavailable(e)

return decorated_function


@handle_migration_read_errors
def handle_migration_write_errors(f):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to distinguish between migration read/write errors. AFAIK, the difference is that in read we only check for migration; so, the same errors can happen in both cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The write section handles a couple of extra errors, and those throw a more severe error (Program instead of Intermittent).
The point is that errors while reading are somewhat expected (e.g. template may be deleted, that's not a bug), while actually migrating the project should be invoked by the client only if the read errors didn't happen.
So a DockerfileUpdateError or a MigrationError caught on writing is likely to be a bug or severe error from which the user may not be able to recover in the UI.

"""Wrapper which handles migrations write exceptions."""
# noqa
@wraps(f)
def decorated_function(*args, **kwargs):
Expand Down
4 changes: 2 additions & 2 deletions renku/ui/service/views/v1_0/cache.py
Expand Up @@ -22,11 +22,11 @@
from renku.ui.service.serializers.v1_0.cache import ProjectMigrateResponseRPC_1_0
from renku.ui.service.views.api_versions import V0_9, V1_0
from renku.ui.service.views.decorators import accepts_json, requires_cache, requires_identity
from renku.ui.service.views.error_handlers import handle_common_except, handle_migration_errors
from renku.ui.service.views.error_handlers import handle_common_except, handle_migration_write_errors


@handle_common_except
@handle_migration_errors
@handle_migration_write_errors
@accepts_json
@requires_cache
@requires_identity
Expand Down
10 changes: 10 additions & 0 deletions tests/cli/test_template.py
Expand Up @@ -245,6 +245,16 @@ def test_template_update_latest_version(runner, client):
assert "Template is up-to-date" in result.output


@pytest.mark.integration
def test_template_update_missing_repo(runner, client_with_template):
"""Test update with a none-existing template repository fails with expected error."""
result = runner.invoke(cli, ["template", "update"])

assert 1 == result.exit_code
assert "Template cannot be fetched" in result.output
assert not client_with_template.repository.is_dirty()


@pytest.mark.integration
def test_template_update_dry_run(runner, client):
"""Test update dry-run doesn't make any changes."""
Expand Down
9 changes: 8 additions & 1 deletion tests/core/management/test_template.py
Expand Up @@ -56,7 +56,7 @@ def test_template_fetch_invalid_git_url():
@pytest.mark.vcr
def test_template_fetch_invalid_git_reference():
"""Test fetching a template from an invalid reference."""
with pytest.raises(errors.InvalidTemplateError):
with pytest.raises(errors.TemplateMissingReferenceError):
fetch_templates_source(source=TEMPLATES_URL, reference="invalid-ref")


Expand Down Expand Up @@ -85,6 +85,13 @@ def test_template_update_files(client_with_template, templates_source, client_da
assert file.read_text() != files_before[file]


def test_template_update_source_failure(client_with_template, client_database_injection_manager):
"""Test template update with broken template source."""
with client_database_injection_manager(client_with_template):
with pytest.raises(errors.TemplateUpdateError):
update_template(force=False, interactive=False, dry_run=False)


@pytest.mark.parametrize(
"action, content_type",
[
Expand Down
74 changes: 62 additions & 12 deletions tests/service/views/test_cache_views.py
Expand Up @@ -26,11 +26,17 @@

from renku.core.dataset.context import DatasetContext
from renku.domain_model.git import GitURL
from renku.domain_model.template import TemplateMetadata
from renku.infrastructure.gateway.dataset_gateway import DatasetGateway
from renku.infrastructure.repository import Repository
from renku.ui.service.errors import IntermittentFileExistsError, UserAnonymousError
from renku.ui.service.errors import (
IntermittentFileExistsError,
IntermittentProjectTemplateUnavailable,
UserAnonymousError,
UserProjectTemplateReferenceError,
)
from renku.ui.service.serializers.headers import JWT_TOKEN_SECRET
from tests.utils import retry_failed
from tests.utils import assert_rpc_response, retry_failed


@pytest.mark.service
Expand Down Expand Up @@ -681,18 +687,62 @@ def test_check_migrations_remote(svc_client, identity_headers, it_remote_repo_ur

@pytest.mark.service
@pytest.mark.integration
def test_check_no_migrations(svc_client_with_repo):
"""Check if migrations are not required."""
svc_client, headers, project_id, _ = svc_client_with_repo

response = svc_client.get("/cache.migrations_check", query_string=dict(project_id=project_id), headers=headers)
def test_mirgate_wrong_template_failure(svc_client_with_repo, template, monkeypatch):
"""Check if migrations gracefully fail when the project template is not available."""
import renku.core.template.usecase
from renku.core.template.template import fetch_templates_source

assert 200 == response.status_code
svc_client, headers, project_id, _ = svc_client_with_repo

assert not response.json["result"]["core_compatibility_status"]["migration_required"]
assert not response.json["result"]["template_status"]["newer_template_available"]
assert not response.json["result"]["dockerfile_renku_status"]["automated_dockerfile_update"]
assert response.json["result"]["project_supported"]
class DummyTemplateMetadata(TemplateMetadata):
def __init__(self, metadata, immutable_files):
super().__init__(metadata=metadata, immutable_files=immutable_files)

def set_fake_source(self, value):
"""Toggle source between fake and real"""
self.fake_source = value

@property
def source(self):
"""Template source."""
template_url = template["url"]
if self.fake_source:
return f"{template_url}FAKE_URL"
return template_url

@property
def reference(self):
"""Template reference."""
return "FAKE_REF"

def dummy_check_for_template_update(client):
metadata = DummyTemplateMetadata.from_client(client=client)
metadata.set_fake_source(fake_source)
templates_source = fetch_templates_source(source=metadata.source, reference=metadata.reference)
update_available, latest_reference = templates_source.is_update_available(
id=metadata.id, reference=metadata.reference, version=metadata.version
)
return update_available, metadata.allow_update, metadata.reference, latest_reference

# NOTE: fake URL and fake REF
fake_source = True
with monkeypatch.context() as monkey:
monkey.setattr(renku.command.migrate, "check_for_template_update", dummy_check_for_template_update)

response = svc_client.get("/cache.migrations_check", query_string=dict(project_id=project_id), headers=headers)

assert_rpc_response(response, "error")
assert IntermittentProjectTemplateUnavailable.code == response.json["error"]["code"]

# NOTE: valid URL but fake REF
fake_source = False
with monkeypatch.context() as monkey:
monkey.setattr(renku.command.migrate, "check_for_template_update", dummy_check_for_template_update)

response = svc_client.get("/cache.migrations_check", query_string=dict(project_id=project_id), headers=headers)

assert_rpc_response(response, "error")
assert UserProjectTemplateReferenceError.code == response.json["error"]["code"]


@pytest.mark.service
Expand Down