From 291f12a432048257f8927d42478e86f747e8e7e7 Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov Date: Sun, 5 Nov 2023 22:25:17 +0000 Subject: [PATCH 1/7] Check the size of `build_path` Fixes #649. --- conda-store-server/conda_store_server/orm.py | 13 ++++++++++--- conda-store-server/tests/test_db_api.py | 10 ++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/conda-store-server/conda_store_server/orm.py b/conda-store-server/conda_store_server/orm.py index 5a9b11762..3420e85a8 100644 --- a/conda-store-server/conda_store_server/orm.py +++ b/conda-store-server/conda_store_server/orm.py @@ -190,15 +190,22 @@ def build_path(self, conda_store): """ store_directory = os.path.abspath(conda_store.store_directory) namespace = self.environment.namespace.name - name = self.specification.name - return ( + res = ( pathlib.Path( conda_store.build_directory.format( - store_directory=store_directory, namespace=namespace, name=name + store_directory=store_directory, + namespace=namespace, ) ) / self.build_key ) + # conda prefix must be less or equal to 255 chars + # https://github.com/conda-incubator/conda-store/issues/649 + if len(str(res)) > 255: + raise ValueError( + f"build_path too long: {res} must be <= 255 chars, got {len(str(res))}" + ) + return res def environment_path(self, conda_store): """Environment path is the path for the symlink to the build diff --git a/conda-store-server/tests/test_db_api.py b/conda-store-server/tests/test_db_api.py index b9574e7d6..75abeb4c4 100644 --- a/conda-store-server/tests/test_db_api.py +++ b/conda-store-server/tests/test_db_api.py @@ -134,3 +134,13 @@ def test_get_set_keyvaluestore(db): # test updating a prefix api.set_kvstore_key_values(db, "pytest", {"c": 999, "d": 999}, update=False) assert {**setting_1, **setting_2} == api.get_kvstore_key_values(db, "pytest") + + +def test_build_path_too_long(db, conda_store, simple_specification): + conda_store.store_directory = 'A' * 800 + build_id = conda_store.register_environment( + db, specification=simple_specification, namespace="pytest" + ) + build = api.get_build(db, build_id=build_id) + with pytest.raises(ValueError, match=r"build_path too long: .* must be <= 255 chars"): + build.build_path(conda_store) From 4df1c2ab535772769669ceec9ba7730ff0577398 Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov Date: Mon, 6 Nov 2023 21:40:35 +0000 Subject: [PATCH 2/7] Set status to FAILED in `build_conda_environment` This extends the scope of try because an exception can be thrown in `append_to_logs`. For the same reason, `set_build_failed` is called first in except blocks. Fixes #654. --- .../conda_store_server/build.py | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/conda-store-server/conda_store_server/build.py b/conda-store-server/conda_store_server/build.py index 84dfea72f..a296cce43 100644 --- a/conda-store-server/conda_store_server/build.py +++ b/conda-store-server/conda_store_server/build.py @@ -107,27 +107,27 @@ def build_conda_environment(db: Session, conda_store, build): symlink the build to a named environment """ - set_build_started(db, build) - append_to_logs( - db, - conda_store, - build, - f"starting build of conda environment {datetime.datetime.utcnow()} UTC\n", - ) + try: + set_build_started(db, build) + append_to_logs( + db, + conda_store, + build, + f"starting build of conda environment {datetime.datetime.utcnow()} UTC\n", + ) - settings = conda_store.get_settings( - db=db, - namespace=build.environment.namespace.name, - environment_name=build.environment.name, - ) + settings = conda_store.get_settings( + db=db, + namespace=build.environment.namespace.name, + environment_name=build.environment.name, + ) - conda_prefix = build.build_path(conda_store) - conda_prefix.parent.mkdir(parents=True, exist_ok=True) + conda_prefix = build.build_path(conda_store) + conda_prefix.parent.mkdir(parents=True, exist_ok=True) - environment_prefix = build.environment_path(conda_store) - environment_prefix.parent.mkdir(parents=True, exist_ok=True) + environment_prefix = build.environment_path(conda_store) + environment_prefix.parent.mkdir(parents=True, exist_ok=True) - try: with utils.timer(conda_store.log, f"building conda_prefix={conda_prefix}"): context = action.action_solve_lockfile( settings.conda_command, @@ -202,14 +202,14 @@ def build_conda_environment(db: Session, conda_store, build): set_build_completed(db, conda_store, build) except subprocess.CalledProcessError as e: + set_build_failed(db, build) conda_store.log.exception(e) append_to_logs(db, conda_store, build, e.output) - set_build_failed(db, build) raise e except Exception as e: + set_build_failed(db, build) conda_store.log.exception(e) append_to_logs(db, conda_store, build, traceback.format_exc()) - set_build_failed(db, build) raise e From 34ebf5b513ada57c18acd28efc13e197274678c6 Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov Date: Mon, 6 Nov 2023 21:43:48 +0000 Subject: [PATCH 3/7] Test build status on failure These tests check whether the status is set to FAILED when an exception occurs in a task. --- conda-store-server/tests/test_server.py | 54 ++++++++++++++++++++++ tests/test_api.py | 61 +++++++++++++++++++++++++ 2 files changed, 115 insertions(+) diff --git a/conda-store-server/tests/test_server.py b/conda-store-server/tests/test_server.py index 7040e338a..0bd585c38 100644 --- a/conda-store-server/tests/test_server.py +++ b/conda-store-server/tests/test_server.py @@ -1,4 +1,5 @@ import json +import time import pytest import yaml @@ -335,6 +336,59 @@ def test_create_specification_unauth(testclient): assert r.status == schema.APIStatus.ERROR +# Only testing size values that will always cause errors. Smaller values could +# cause errors as well, but would be flaky since the test conda-store state +# directory might have different lengths on different systems, for instance, +# due to different username lengths. +@pytest.mark.parametrize( + "size", + [ + # OSError: [Errno 36] File name too long + 255, + # OSError: [Errno 36] File name too long + 256, + ], +) +def test_create_specification_auth_env_name_too_long(testclient, celery_worker, authenticate, size): + namespace = "default" + environment_name = 'A' * size + + response = testclient.post( + "api/v1/specification", + json={ + "namespace": namespace, + "specification": json.dumps({"name": environment_name}), + }, + ) + response.raise_for_status() + + r = schema.APIPostSpecification.parse_obj(response.json()) + assert r.status == schema.APIStatus.OK + build_id = r.data.build_id + + # Try checking that the status is 'FAILED' + is_updated = False + for _ in range(5): + time.sleep(5) + + # check for the given build + response = testclient.get(f"api/v1/build/{build_id}") + response.raise_for_status() + + r = schema.APIGetBuild.parse_obj(response.json()) + assert r.status == schema.APIStatus.OK + assert r.data.specification.name == environment_name + if r.data.status == "QUEUED": + continue # checked too fast, try again + assert r.data.status == "FAILED" + is_updated = True + break + + # If we're here, the task didn't update the status on failure + if not is_updated: + assert False, f"failed to update status" + + def test_create_specification_auth(testclient, celery_worker, authenticate): namespace = "default" environment_name = "pytest" diff --git a/tests/test_api.py b/tests/test_api.py index 7e52ddfe4..473ce300b 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -10,6 +10,7 @@ """ import asyncio import json +import time import uuid from typing import List @@ -458,6 +459,66 @@ def test_create_specification_auth(testclient): assert r.data.namespace.name == namespace +# Only testing size values that will always cause errors. Smaller values could +# cause errors as well, but would be flaky since the test conda-store state +# directory might have different lengths on different systems, for instance, +# due to different username lengths. +@pytest.mark.parametrize( + "size", + [ + # minio.error.MinioException: S3 operation failed; code: + # XMinioInvalidObjectName, message: Object name contains unsupported + # characters. + # The error message is misleading: it's a size issue. + 255, + # SQL error: value too long for type character varying(255) + 256, + ], +) +def test_create_specification_auth_env_name_too_long(testclient, size): + namespace = "default" + environment_name = 'A' * size + + testclient.login() + response = testclient.post( + "api/v1/specification", + json={ + "namespace": namespace, + "specification": json.dumps({"name": environment_name}), + }, + ) + if size > 255: + assert response.status_code == 500 + return # error, nothing to do + response.raise_for_status() + + r = schema.APIPostSpecification.parse_obj(response.json()) + assert r.status == schema.APIStatus.OK + build_id = r.data.build_id + + # Try checking that the status is 'FAILED' + is_updated = False + for _ in range(5): + time.sleep(5) + + # check for the given build + response = testclient.get(f"api/v1/build/{build_id}") + response.raise_for_status() + + r = schema.APIGetBuild.parse_obj(response.json()) + assert r.status == schema.APIStatus.OK + assert r.data.specification.name == environment_name + if r.data.status == "QUEUED": + continue # checked too fast, try again + assert r.data.status == "FAILED" + is_updated = True + break + + # If we're here, the task didn't update the status on failure + if not is_updated: + assert False, f"failed to update status" + + def test_create_specification_auth_no_namespace_specified(testclient): namespace = "username" # same as login username environment_name = f"pytest-{uuid.uuid4()}" From 0a2881166b8d2bee87a5e7024ea1dfa900c3f2a6 Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov Date: Tue, 7 Nov 2023 12:26:07 +0000 Subject: [PATCH 4/7] Add `status_info` This is an additional way to communicate information about the status of a build, which will be exposed to the user. --- .../versions/d78e9889566a_add_status_info.py | 23 +++++++++++++++++++ .../conda_store_server/build.py | 19 +++++++++++++-- conda-store-server/conda_store_server/orm.py | 8 ++++--- .../conda_store_server/schema.py | 1 + .../conda_store_server/utils.py | 4 ++++ conda-store-server/tests/test_server.py | 1 + tests/test_api.py | 1 + 7 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 conda-store-server/conda_store_server/alembic/versions/d78e9889566a_add_status_info.py diff --git a/conda-store-server/conda_store_server/alembic/versions/d78e9889566a_add_status_info.py b/conda-store-server/conda_store_server/alembic/versions/d78e9889566a_add_status_info.py new file mode 100644 index 000000000..d91938a11 --- /dev/null +++ b/conda-store-server/conda_store_server/alembic/versions/d78e9889566a_add_status_info.py @@ -0,0 +1,23 @@ +"""add status_info + +Revision ID: d78e9889566a +Revises: b387747ca9b7 +Create Date: 2023-11-07 12:25:04.416192 + +""" +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "d78e9889566a" +down_revision = "b387747ca9b7" +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column("build", sa.Column("status_info", sa.UnicodeText(), nullable=True)) + + +def downgrade(): + op.drop_column("build", "status_info") diff --git a/conda-store-server/conda_store_server/build.py b/conda-store-server/conda_store_server/build.py index a296cce43..6475c8921 100644 --- a/conda-store-server/conda_store_server/build.py +++ b/conda-store-server/conda_store_server/build.py @@ -10,6 +10,7 @@ import yaml from conda_store_server import action, api, conda_utils, orm, schema, utils +from conda_store_server.utils import BuildPathError from sqlalchemy.orm import Session @@ -38,8 +39,11 @@ def set_build_started(db: Session, build: orm.Build): db.commit() -def set_build_failed(db: Session, build: orm.Build): +def set_build_failed( + db: Session, build: orm.Build, status_info: typing.Optional[str] = None +): build.status = schema.BuildStatus.FAILED + build.status_info = status_info build.ended_on = datetime.datetime.utcnow() db.commit() @@ -109,6 +113,9 @@ def build_conda_environment(db: Session, conda_store, build): """ try: set_build_started(db, build) + # Note: even append_to_logs can fail due to filename size limit, so + # check build_path length first + conda_prefix = build.build_path(conda_store) append_to_logs( db, conda_store, @@ -122,7 +129,6 @@ def build_conda_environment(db: Session, conda_store, build): environment_name=build.environment.name, ) - conda_prefix = build.build_path(conda_store) conda_prefix.parent.mkdir(parents=True, exist_ok=True) environment_prefix = build.environment_path(conda_store) @@ -201,11 +207,20 @@ def build_conda_environment(db: Session, conda_store, build): build.size = context.result["disk_usage"] set_build_completed(db, conda_store, build) + # Always mark build as failed first since other functions may throw an + # exception except subprocess.CalledProcessError as e: set_build_failed(db, build) conda_store.log.exception(e) append_to_logs(db, conda_store, build, e.output) raise e + except BuildPathError as e: + # Provide status_info, which will be exposed to the user, ONLY in this + # case because the error message doesn't expose sensitive information + set_build_failed(db, build, status_info=e.message) + conda_store.log.exception(e) + append_to_logs(db, conda_store, build, traceback.format_exc()) + raise e except Exception as e: set_build_failed(db, build) conda_store.log.exception(e) diff --git a/conda-store-server/conda_store_server/orm.py b/conda-store-server/conda_store_server/orm.py index 3420e85a8..30bd4d603 100644 --- a/conda-store-server/conda_store_server/orm.py +++ b/conda-store-server/conda_store_server/orm.py @@ -7,6 +7,7 @@ from conda_store_server import conda_utils, schema, utils from conda_store_server.environment import validate_environment +from conda_store_server.utils import BuildPathError from sqlalchemy import ( JSON, BigInteger, @@ -173,6 +174,9 @@ class Build(Base): package_builds = relationship("CondaPackageBuild", secondary=build_conda_package) status = Column(Enum(schema.BuildStatus), default=schema.BuildStatus.QUEUED) + # Additional status info that will be provided to the user. DO NOT put + # sensitive data here + status_info = Column(UnicodeText, default=None) size = Column(BigInteger, default=0) scheduled_on = Column(DateTime, default=datetime.datetime.utcnow) started_on = Column(DateTime, default=None) @@ -202,9 +206,7 @@ def build_path(self, conda_store): # conda prefix must be less or equal to 255 chars # https://github.com/conda-incubator/conda-store/issues/649 if len(str(res)) > 255: - raise ValueError( - f"build_path too long: {res} must be <= 255 chars, got {len(str(res))}" - ) + raise BuildPathError("build_path too long: must be <= 255 chars") return res def environment_path(self, conda_store): diff --git a/conda-store-server/conda_store_server/schema.py b/conda-store-server/conda_store_server/schema.py index b0f365bbe..f5ae191ef 100644 --- a/conda-store-server/conda_store_server/schema.py +++ b/conda-store-server/conda_store_server/schema.py @@ -157,6 +157,7 @@ class Build(BaseModel): specification: Optional[Specification] packages: Optional[List[CondaPackage]] status: BuildStatus + status_info: Optional[str] size: int scheduled_on: datetime.datetime started_on: Optional[datetime.datetime] diff --git a/conda-store-server/conda_store_server/utils.py b/conda-store-server/conda_store_server/utils.py index 26006f99f..386561599 100644 --- a/conda-store-server/conda_store_server/utils.py +++ b/conda-store-server/conda_store_server/utils.py @@ -15,6 +15,10 @@ def message(self): return self.args[0] +class BuildPathError(CondaStoreError): + pass + + def symlink(source, target): if os.path.islink(target): os.unlink(target) diff --git a/conda-store-server/tests/test_server.py b/conda-store-server/tests/test_server.py index 0bd585c38..149132d4b 100644 --- a/conda-store-server/tests/test_server.py +++ b/conda-store-server/tests/test_server.py @@ -381,6 +381,7 @@ def test_create_specification_auth_env_name_too_long(testclient, celery_worker, if r.data.status == "QUEUED": continue # checked too fast, try again assert r.data.status == "FAILED" + assert r.data.status_info == "build_path too long: must be <= 255 chars" is_updated = True break diff --git a/tests/test_api.py b/tests/test_api.py index 473ce300b..ef8e7e5b4 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -511,6 +511,7 @@ def test_create_specification_auth_env_name_too_long(testclient, size): if r.data.status == "QUEUED": continue # checked too fast, try again assert r.data.status == "FAILED" + assert r.data.status_info == "build_path too long: must be <= 255 chars" is_updated = True break From 63aeb15c42a58dcb0b3d485cfc330a54e62a018b Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov Date: Tue, 7 Nov 2023 16:12:21 +0000 Subject: [PATCH 5/7] Fix `build_path` test to account for recent changes --- conda-store-server/tests/test_db_api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/conda-store-server/tests/test_db_api.py b/conda-store-server/tests/test_db_api.py index 75abeb4c4..e50a50477 100644 --- a/conda-store-server/tests/test_db_api.py +++ b/conda-store-server/tests/test_db_api.py @@ -1,6 +1,7 @@ import pytest from conda_store_server import api from conda_store_server.orm import NamespaceRoleMapping +from conda_store_server.utils import BuildPathError def test_namespace_crud(db): @@ -142,5 +143,5 @@ def test_build_path_too_long(db, conda_store, simple_specification): db, specification=simple_specification, namespace="pytest" ) build = api.get_build(db, build_id=build_id) - with pytest.raises(ValueError, match=r"build_path too long: .* must be <= 255 chars"): + with pytest.raises(BuildPathError, match=r"build_path too long: must be <= 255 chars"): build.build_path(conda_store) From 34ac2dd4380dda1da3264e0482bafcd0e79976bf Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov Date: Tue, 7 Nov 2023 16:14:28 +0000 Subject: [PATCH 6/7] Update error message --- conda-store-server/conda_store_server/orm.py | 2 +- conda-store-server/tests/test_db_api.py | 2 +- conda-store-server/tests/test_server.py | 2 +- tests/test_api.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/conda-store-server/conda_store_server/orm.py b/conda-store-server/conda_store_server/orm.py index 30bd4d603..557451c5c 100644 --- a/conda-store-server/conda_store_server/orm.py +++ b/conda-store-server/conda_store_server/orm.py @@ -206,7 +206,7 @@ def build_path(self, conda_store): # conda prefix must be less or equal to 255 chars # https://github.com/conda-incubator/conda-store/issues/649 if len(str(res)) > 255: - raise BuildPathError("build_path too long: must be <= 255 chars") + raise BuildPathError("build_path too long: must be <= 255 characters") return res def environment_path(self, conda_store): diff --git a/conda-store-server/tests/test_db_api.py b/conda-store-server/tests/test_db_api.py index e50a50477..59fe6307b 100644 --- a/conda-store-server/tests/test_db_api.py +++ b/conda-store-server/tests/test_db_api.py @@ -143,5 +143,5 @@ def test_build_path_too_long(db, conda_store, simple_specification): db, specification=simple_specification, namespace="pytest" ) build = api.get_build(db, build_id=build_id) - with pytest.raises(BuildPathError, match=r"build_path too long: must be <= 255 chars"): + with pytest.raises(BuildPathError, match=r"build_path too long: must be <= 255 characters"): build.build_path(conda_store) diff --git a/conda-store-server/tests/test_server.py b/conda-store-server/tests/test_server.py index 149132d4b..252bdaca8 100644 --- a/conda-store-server/tests/test_server.py +++ b/conda-store-server/tests/test_server.py @@ -381,7 +381,7 @@ def test_create_specification_auth_env_name_too_long(testclient, celery_worker, if r.data.status == "QUEUED": continue # checked too fast, try again assert r.data.status == "FAILED" - assert r.data.status_info == "build_path too long: must be <= 255 chars" + assert r.data.status_info == "build_path too long: must be <= 255 characters" is_updated = True break diff --git a/tests/test_api.py b/tests/test_api.py index ef8e7e5b4..a3e59652c 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -511,7 +511,7 @@ def test_create_specification_auth_env_name_too_long(testclient, size): if r.data.status == "QUEUED": continue # checked too fast, try again assert r.data.status == "FAILED" - assert r.data.status_info == "build_path too long: must be <= 255 chars" + assert r.data.status_info == "build_path too long: must be <= 255 characters" is_updated = True break From e2a7618c14249f86c186812214c710f1f543a5a7 Mon Sep 17 00:00:00 2001 From: Nikita Karetnikov Date: Sun, 12 Nov 2023 18:47:29 +0000 Subject: [PATCH 7/7] Document build path length limits --- docs/administration.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/administration.md b/docs/administration.md index 1cd02e667..427adc26b 100644 --- a/docs/administration.md +++ b/docs/administration.md @@ -552,6 +552,25 @@ for several reasons: - worker was killed for other reasons e.g. forced restart - bugs in conda-store +### Build path length + +Conda packages are guaranteed to be [relocatable] as long as the environment +prefix length is <= 255 characters. In conda-store, the said prefix is specified +in `Build.build_path`. When building an environment, you might see an error like +this: + +``` +build_path too long: must be <= 255 characters +``` + +If so, try configuring the conda-store `CondaStore.store_directory` to be as +close to the filesystem root as possible. Additionally, 255 characters is also a +common limit for individual files on many filesystems. When creating +environments, try using shorter `namespace` and `environment` names since they +affect both the `build_path` length and the filename length. + +[relocatable]: https://docs.conda.io/projects/conda-build/en/latest/resources/make-relocatable.html + ### Long paths on Windows conda-store supports Windows in standalone mode. However, when creating